Re: [RFC PATCH 10/11] mm/hmm: Poison hmm_range during unregister

[Jason Gunthorpe <jgg@mellanox.com>]

On Sat, Jun 08, 2019 at 01:43:12AM +0530, Souptick Joarder wrote:
> On Thu, May 23, 2019 at 9:05 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > From: Jason Gunthorpe <jgg@mellanox.com>
> >
> > Trying to misuse a range outside its lifetime is a kernel bug. Use WARN_ON
> > and poison bytes to detect this condition.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> 
> Acked-by: Souptick Joarder <jrdr.linux@gmail.com>
> 
> >  mm/hmm.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/hmm.c b/mm/hmm.c
> > index 6c3b7398672c29..02752d3ef2ed92 100644
> > +++ b/mm/hmm.c
> > @@ -936,8 +936,7 @@ EXPORT_SYMBOL(hmm_range_register);
> >   */
> >  void hmm_range_unregister(struct hmm_range *range)
> >  {
> > -       /* Sanity check this really should not happen. */
> > -       if (range->hmm == NULL || range->end <= range->start)
> > +       if (WARN_ON(range->end <= range->start))
> >                 return;
> 
> Does it make any sense to sanity check for range == NULL as well ?

The purpose of the sanity check is to make API misuse into a reliable
crash, so if range is NULL then it will already reliably crash due to
next lines. 

This approach is to help driver authors use the API properly.

However, looking closer, this will already crash reliably if we double
unregister as range->hmm->lock will instantly crash due the poison,
and the test no longer works right anyhow since v2 dropped the set of
the start/end values. I've deleted the check for v3:

Thanks,
Jason

From 461d880d1e898dc8e9ff6236b1730a5996df8738 Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgg@mellanox.com>
Date: Thu, 23 May 2019 11:40:24 -0300
Subject: [PATCH] mm/hmm: Poison hmm_range during unregister
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Trying to misuse a range outside its lifetime is a kernel bug. Use poison
bytes to help detect this condition. Double unregister will reliably crash.

Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
---
v2
- Keep range start/end valid after unregistration (Jerome)
v3
- Revise some comments (John)
- Remove start/end WARN_ON (Souptick)
---
 mm/hmm.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index d4ac179c899c4e..288fcd1ffca5b5 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -925,10 +925,6 @@ void hmm_range_unregister(struct hmm_range *range)
 {
 	struct hmm *hmm = range->hmm;
 
-	/* Sanity check this really should not happen. */
-	if (hmm == NULL || range->end <= range->start)
-		return;
-
 	mutex_lock(&hmm->lock);
 	list_del_rcu(&range->list);
 	mutex_unlock(&hmm->lock);
@@ -937,7 +933,14 @@ void hmm_range_unregister(struct hmm_range *range)
 	range->valid = false;
 	mmput(hmm->mm);
 	hmm_put(hmm);
-	range->hmm = NULL;
+
+	/*
+	 * The range is now invalid and the ref on the hmm is dropped, so
+         * poison the pointer.  Leave other fields in place, for the caller's
+         * use.
+         */
+	range->valid = false;
+	memset(&range->hmm, POISON_INUSE, sizeof(range->hmm));
 }
 EXPORT_SYMBOL(hmm_range_unregister);
 
-- 
2.21.0