| Message ID | 20190523153436.19102-11-jgg@ziepe.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/hmm: Various revisions from a locking/code review | expand |
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 > --- a/mm/hmm.c > +++ 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 ? > > mutex_lock(&range->hmm->lock); > @@ -945,9 +944,13 @@ void hmm_range_unregister(struct hmm_range *range) > mutex_unlock(&range->hmm->lock); > > /* Drop reference taken by hmm_range_register() */ > - range->valid = false; > hmm_put(range->hmm); > - range->hmm = NULL; > + > + /* The range is now invalid, leave it poisoned. */ > + range->valid = false; > + range->start = ULONG_MAX; > + range->end = 0; > + memset(&range->hmm, POISON_INUSE, sizeof(range->hmm)); > } > EXPORT_SYMBOL(hmm_range_unregister); > > -- > 2.21.0 >
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);
diff --git a/mm/hmm.c b/mm/hmm.c index 6c3b7398672c29..02752d3ef2ed92 100644 --- a/mm/hmm.c +++ 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; mutex_lock(&range->hmm->lock); @@ -945,9 +944,13 @@ void hmm_range_unregister(struct hmm_range *range) mutex_unlock(&range->hmm->lock); /* Drop reference taken by hmm_range_register() */ - range->valid = false; hmm_put(range->hmm); - range->hmm = NULL; + + /* The range is now invalid, leave it poisoned. */ + range->valid = false; + range->start = ULONG_MAX; + range->end = 0; + memset(&range->hmm, POISON_INUSE, sizeof(range->hmm)); } EXPORT_SYMBOL(hmm_range_unregister);