Re: [PATCH v2 hmm 08/11] mm/hmm: Remove racy protection against double-unregistration

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Jun 06, 2019 at 08:29:10PM -0700, John Hubbard wrote:
> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> > From: Jason Gunthorpe <jgg@mellanox.com>
> > 
> > No other register/unregister kernel API attempts to provide this kind of
> > protection as it is inherently racy, so just drop it.
> > 
> > Callers should provide their own protection, it appears nouveau already
> > does, but just in case drop a debugging POISON.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> > Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
> >  mm/hmm.c | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/mm/hmm.c b/mm/hmm.c
> > index c702cd72651b53..6802de7080d172 100644
> > +++ b/mm/hmm.c
> > @@ -284,18 +284,13 @@ EXPORT_SYMBOL(hmm_mirror_register);
> >   */
> >  void hmm_mirror_unregister(struct hmm_mirror *mirror)
> >  {
> > -	struct hmm *hmm = READ_ONCE(mirror->hmm);
> > -
> > -	if (hmm == NULL)
> > -		return;
> > +	struct hmm *hmm = mirror->hmm;
> >  
> >  	down_write(&hmm->mirrors_sem);
> >  	list_del_init(&mirror->list);
> > -	/* To protect us against double unregister ... */
> > -	mirror->hmm = NULL;
> >  	up_write(&hmm->mirrors_sem);
> > -
> >  	hmm_put(hmm);
> > +	memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm));
> 
> I hadn't thought of POISON_* for these types of cases, it's a 
> good technique to remember.
> 
> I noticed that this is now done outside of the lock, but that
> follows directly from your commit description, so that all looks 
> correct.

Yes, the thing about POISON is that if you ever read it then you have
found a use after free bug - thus we should never need to write it
under a lock (just after a serializing lock)

Normally I wouldn't bother as kfree does poison as well, but since we
can't easily audit the patches yet to be submitted this seems safer
and will reliably cause those patches to explode with an oops in
testing.

Thanks,
Jason