Re: [PATCH 5/5] mm/hmm: Fix mm stale reference use in hmm_free()

[John Hubbard]

On 5/22/19 6:25 PM, Jason Gunthorpe wrote:
> On Wed, May 22, 2019 at 05:54:17PM -0700, Ralph Campbell wrote:
> > On 5/22/19 4:36 PM, Jason Gunthorpe wrote:
> > > On Mon, May 06, 2019 at 04:35:14PM -0700, rcampbell@xxxxxxxxxx wrote:
> > > > From: Ralph Campbell <rcampbell@xxxxxxxxxx>
> > > >
> > > > The last reference to struct hmm may be released long after the mm_struct
> > > > is destroyed because the struct hmm_mirror memory may be part of a
> > > > device driver open file private data pointer. The file descriptor close
> > > > is usually after the mm_struct is destroyed in do_exit(). This is a good
> > > > reason for making struct hmm a kref_t object [1] since its lifetime spans
> > > > the life time of mm_struct and struct hmm_mirror.
> > >
> > > > The fix is to not use hmm->mm in hmm_free() and to clear mm->hmm and
> > > > hmm->mm pointers in hmm_destroy() when the mm_struct is
> > > > destroyed.
> > >
> > > I think the right way to fix this is to have the struct hmm hold a
> > > mmgrab() on the mm so its memory cannot go away until all of the hmm
> > > users release the struct hmm, hmm_ranges/etc
> > >
> > > Then we can properly use mmget_not_zero() instead of the racy/abnormal
> > > 'if (hmm->xmm == NULL || hmm->dead)' pattern (see the other
> > > thread). Actually looking at this, all these tests look very
> > > questionable. If we hold the mmget() for the duration of the range
> > > object, as Jerome suggested, then they all get deleted.
> > >
> > > That just leaves mmu_notifier_unregister_no_relase() as the remaining
> > > user of hmm->mm (everyone else is trying to do range->mm) - and it
> > > looks like it currently tries to call
> > > mmu_notifier_unregister_no_release on a NULL hmm->mm and crashes :(
> > >
> > > Holding the mmgrab fixes this as we can safely call
> > > mmu_notifier_unregister_no_relase() post exit_mmap on a grab'd mm.
> > >
> > > Also we can delete the hmm_mm_destroy() intrustion into fork.c as it
> > > can't be called when the mmgrab is active.
> > >
> > > This is the basic pattern we used in ODP when working with mmu
> > > notifiers, I don't know why hmm would need to be different.

+1 for the mmgrab() approach. I have never been able to see how these
various checks can protect anything, and refcounting it into place definitely
sounds like the right answer.


thanks,
--
John Hubbard
NVIDIA