Jason Gunthorpe
2019-Jul-23 17:17 UTC
[Nouveau] [PATCH 4/6] nouveau: unlock mmap_sem on all errors from nouveau_range_fault
On Tue, Jul 23, 2019 at 06:30:48PM +0200, Christoph Hellwig wrote:> On Tue, Jul 23, 2019 at 03:18:28PM +0000, Jason Gunthorpe wrote: > > Hum.. > > > > The caller does this: > > > > again: > > ret = nouveau_range_fault(&svmm->mirror, &range); > > if (ret == 0) { > > mutex_lock(&svmm->mutex); > > if (!nouveau_range_done(&range)) { > > mutex_unlock(&svmm->mutex); > > goto again; > > > > And we can't call nouveau_range_fault() -> hmm_range_fault() without > > holding the mmap_sem, so we can't allow nouveau_range_fault to unlock > > it. > > Goto again can only happen if nouveau_range_fault was successful, > in which case we did not drop mmap_sem.Oh, right we switch from success = number of pages to success =0.. However the reason this looks so weird to me is that the locking pattern isn't being followed, any result of hmm_range_fault should be ignored until we enter the svmm->mutex and check if there was a colliding invalidation. So the 'goto again' *should* be possible even if range_fault failed. But that is not for this patch..> > ret = hmm_range_fault(range, true); > > if (ret <= 0) { > > if (ret == 0) > > ret = -EBUSY; > > - up_read(&range->vma->vm_mm->mmap_sem); > > hmm_range_unregister(range); > > This would hold mmap_sem over hmm_range_unregister, which can lead > to deadlock if we call exit_mmap and then acquire mmap_sem again.That reminds me, this code is also leaking hmm_range_unregister() in the success path, right? I think the right way to structure this is to move the goto again and related into the nouveau_range_fault() so the whole retry algorithm is sensibly self contained. Jason
Christoph Hellwig
2019-Jul-23 17:23 UTC
[Nouveau] [PATCH 4/6] nouveau: unlock mmap_sem on all errors from nouveau_range_fault
On Tue, Jul 23, 2019 at 02:17:31PM -0300, Jason Gunthorpe wrote:> That reminds me, this code is also leaking hmm_range_unregister() in > the success path, right?No, that is done by hmm_vma_range_done / nouveau_range_done for the success path.> > I think the right way to structure this is to move the goto again and > related into the nouveau_range_fault() so the whole retry algorithm is > sensibly self contained.Then we'd take svmm->mutex inside the helper and let the caller unlock that. Either way it is a bit of a mess, and I'd rather prefer if someone has the hardware would do a grand rewrite of this path eventually. Alternatively if no one signs up to mainain this code we should eventually drop it given the staging status.
Jason Gunthorpe
2019-Jul-23 17:30 UTC
[Nouveau] [PATCH 4/6] nouveau: unlock mmap_sem on all errors from nouveau_range_fault
On Tue, Jul 23, 2019 at 07:23:35PM +0200, Christoph Hellwig wrote:> On Tue, Jul 23, 2019 at 02:17:31PM -0300, Jason Gunthorpe wrote: > > That reminds me, this code is also leaking hmm_range_unregister() in > > the success path, right? > > No, that is done by hmm_vma_range_done / nouveau_range_done for the > success path... which is done with the mmap_sem held :(> > I think the right way to structure this is to move the goto again and > > related into the nouveau_range_fault() so the whole retry algorithm is > > sensibly self contained. > > Then we'd take svmm->mutex inside the helper and let the caller > unlock that. Either way it is a bit of a mess, and I'd rather prefer > if someone has the hardware would do a grand rewrite of this path > eventually. Alternatively if no one signs up to mainain this code > we should eventually drop it given the staging status.I tend to agree with the sentiment, it just makes me sad that all the examples we have of these APIs are so troubled. Jason
Seemingly Similar Threads
- [PATCH 4/6] nouveau: unlock mmap_sem on all errors from nouveau_range_fault
- [PATCH 4/6] nouveau: unlock mmap_sem on all errors from nouveau_range_fault
- [PATCH 4/6] nouveau: unlock mmap_sem on all errors from nouveau_range_fault
- [PATCH 2/6] mm: move hmm_vma_range_done and hmm_vma_fault to nouveau
- [PATCH 2/6] mm: move hmm_vma_range_done and hmm_vma_fault to nouveau