| Message ID | 20190614004450.20252-9-jgg@ziepe.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/hmm: Various revisions from a locking/code review | expand |
On Thu, Jun 13, 2019 at 09:44:46PM -0300, 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. I don't even think we even need to bother with the POISON, normal list debugging will already catch a double unregistration anyway. Otherwise looks fine: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Sat, Jun 15, 2019 at 07:16:12AM -0700, Christoph Hellwig wrote: > On Thu, Jun 13, 2019 at 09:44:46PM -0300, 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. > > I don't even think we even need to bother with the POISON, normal list > debugging will already catch a double unregistration anyway. mirror->hmm isn't a list so list debugging won't help. My concern when I wrote this was that one of the in flight patches I can't see might be depending on this double-unregister-is-safe behavior, so I wanted them to crash reliably. It is a really overly conservative thing to do.. Thanks, Jason
On Tue, Jun 18, 2019 at 10:13:24AM -0300, Jason Gunthorpe wrote: > > I don't even think we even need to bother with the POISON, normal list > > debugging will already catch a double unregistration anyway. > > mirror->hmm isn't a list so list debugging won't help. > > My concern when I wrote this was that one of the in flight patches I > can't see might be depending on this double-unregister-is-safe > behavior, so I wanted them to crash reliably. > > It is a really overly conservative thing to do.. mirror->list is a list, and if we do a list_del on it during the second unregistration it will trip up on the list poisoning.
On Tue, Jun 18, 2019 at 06:27:22AM -0700, Christoph Hellwig wrote: > On Tue, Jun 18, 2019 at 10:13:24AM -0300, Jason Gunthorpe wrote: > > > I don't even think we even need to bother with the POISON, normal list > > > debugging will already catch a double unregistration anyway. > > > > mirror->hmm isn't a list so list debugging won't help. > > > > My concern when I wrote this was that one of the in flight patches I > > can't see might be depending on this double-unregister-is-safe > > behavior, so I wanted them to crash reliably. > > > > It is a really overly conservative thing to do.. > > mirror->list is a list, and if we do a list_del on it during the > second unregistration it will trip up on the list poisoning. With the previous loose coupling of the mirror and the range some code might rance to try to create a range without a mirror, which will now reliably crash with the poison. It isn't so much the double unregister that worries me, but racing unregister with range functions. Jason
On Tue, Jun 18, 2019 at 03:57:57PM -0300, Jason Gunthorpe wrote: > With the previous loose coupling of the mirror and the range some code > might rance to try to create a range without a mirror, which will now > reliably crash with the poison. > > It isn't so much the double unregister that worries me, but racing > unregister with range functions. Oh well. It was just a nitpick for the highly unusual code patterns in the two unregister routines, probably not worth fighting over even if I still don't see the point.
diff --git a/mm/hmm.c b/mm/hmm.c index c0f622f86223c2..e3e0a811a3a774 100644 --- a/mm/hmm.c +++ b/mm/hmm.c @@ -283,18 +283,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);