| 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 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 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
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);