Re: [RFC PATCH 01/11] mm/hmm: Fix use after free with struct hmm in the mmu notifiers

[Jason Gunthorpe <jgg@mellanox.com>]

On Thu, Jun 06, 2019 at 04:54:41PM -0700, Ira Weiny wrote:
> On Thu, May 23, 2019 at 12:34:26PM -0300, Jason Gunthorpe wrote:
> > From: Jason Gunthorpe <jgg@mellanox.com>
> > 
> > mmu_notifier_unregister_no_release() is not a fence and the mmu_notifier
> > system will continue to reference hmm->mn until the srcu grace period
> > expires.
> > 
> > Resulting in use after free races like this:
> > 
> >          CPU0                                     CPU1
> >                                                __mmu_notifier_invalidate_range_start()
> >                                                  srcu_read_lock
> >                                                  hlist_for_each ()
> >                                                    // mn == hmm->mn
> > hmm_mirror_unregister()
> >   hmm_put()
> >     hmm_free()
> >       mmu_notifier_unregister_no_release()
> >          hlist_del_init_rcu(hmm-mn->list)
> > 			                           mn->ops->invalidate_range_start(mn, range);
> > 					             mm_get_hmm()
> >       mm->hmm = NULL;
> >       kfree(hmm)
> >                                                      mutex_lock(&hmm->lock);
> > 
> > Use SRCU to kfree the hmm memory so that the notifiers can rely on hmm
> > existing. Get the now-safe hmm struct through container_of and directly
> > check kref_get_unless_zero to lock it against free.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> >  include/linux/hmm.h |  1 +
> >  mm/hmm.c            | 25 +++++++++++++++++++------
> >  2 files changed, 20 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> > index 51ec27a8466816..8b91c90d3b88cb 100644
> > +++ b/include/linux/hmm.h
> > @@ -102,6 +102,7 @@ struct hmm {
> >  	struct mmu_notifier	mmu_notifier;
> >  	struct rw_semaphore	mirrors_sem;
> >  	wait_queue_head_t	wq;
> > +	struct rcu_head		rcu;
> >  	long			notifiers;
> >  	bool			dead;
> >  };
> > diff --git a/mm/hmm.c b/mm/hmm.c
> > index 816c2356f2449f..824e7e160d8167 100644
> > +++ b/mm/hmm.c
> > @@ -113,6 +113,11 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> >  	return NULL;
> >  }
> >  
> > +static void hmm_fee_rcu(struct rcu_head *rcu)
> 
> NIT: "free"
> 
> Other than that looks good.
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Fixed in v2, thanks

Jason