| Message ID | 20190606184438.31646-9-jgg@ziepe.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Various revisions from a locking/code review | expand |
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 > --- a/mm/hmm.c > +++ 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. > } > EXPORT_SYMBOL(hmm_mirror_unregister); > > Reviewed-by: John Hubbard <jhubbard@nvidia.com> thanks,
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
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> Reviewed-by: Ralph Campbell <rcampbell@nvidia.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 > --- a/mm/hmm.c > +++ 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)); > } > EXPORT_SYMBOL(hmm_mirror_unregister); > >
diff --git a/mm/hmm.c b/mm/hmm.c index c702cd72651b53..6802de7080d172 100644 --- a/mm/hmm.c +++ 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)); } EXPORT_SYMBOL(hmm_mirror_unregister);