| Message ID | 20180323230443.168482-1-ebiggers3@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 23, 2018 at 04:04:43PM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > We can reach 'out:' with a negative dentry, e.g. if there is contention > on ->d_parent->d_lock and another task concurrently gets a reference to > the negative dentry. In that case 'inode' will be NULL, so we must not > try to unlock 'inode'. > > This bug was found by xfstest generic/429. hmm ... I'd rather see: if (unlikely(parent != dentry->d_parent)) { spin_unlock(&parent->d_lock); spin_lock(&dentry->d_lock); - goto out; + if (inode) + goto out; + return false; } spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED); if (likely(!dentry->d_lockref.count)) return true; spin_unlock(&parent->d_lock); out: spin_unlock(&inode->i_lock); return false; That puts the comparison out-of-line rather than in the exit path that everybody uses. (Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com> in case we end up choosing this variant)
On Fri, Mar 23, 2018 at 09:37:35PM -0700, Matthew Wilcox wrote: > On Fri, Mar 23, 2018 at 04:04:43PM -0700, Eric Biggers wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > We can reach 'out:' with a negative dentry, e.g. if there is contention > > on ->d_parent->d_lock and another task concurrently gets a reference to > > the negative dentry. In that case 'inode' will be NULL, so we must not > > try to unlock 'inode'. > > > > This bug was found by xfstest generic/429. > > hmm ... I'd rather see: > > if (unlikely(parent != dentry->d_parent)) { > spin_unlock(&parent->d_lock); > spin_lock(&dentry->d_lock); > - goto out; > + if (inode) > + goto out; > + return false; > } > spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED); > if (likely(!dentry->d_lockref.count)) > return true; > spin_unlock(&parent->d_lock); > out: > spin_unlock(&inode->i_lock); > return false; > > That puts the comparison out-of-line rather than in the exit path that > everybody uses. That was my first reaction as well, but... we can get there without parent changing - just a negative dentry that got grabbed by somebody else just as we'd been getting its ->d_lock. Moreover, this is _not_ the exit path everyone takes - all paths reaching it go though an unlikely branch. The common ones are actually "got all locks, everything's stable, nobody has grabbed any references" (return true a couple of lines prior) or "the sucker has grown references while it sat on the shrink list" as the second (considerably more rare) option (the very first return false in that function). To get to out: we need the damn thing touched by somebody else while we were *inside* shrink_lock_dentry(). And it's a narrow window (note that we do not have trylock loops there anymore). So Eric's patch is the right solution; I've folded it in, with credits in updated commit message. Will push tonight or tomorrow...
On Sat, Mar 24, 2018 at 04:50:54AM +0000, Al Viro wrote: > On Fri, Mar 23, 2018 at 09:37:35PM -0700, Matthew Wilcox wrote: > > That puts the comparison out-of-line rather than in the exit path that > > everybody uses. > > That was my first reaction as well, but... we can get there without parent > changing - just a negative dentry that got grabbed by somebody else just > as we'd been getting its ->d_lock. Moreover, this is _not_ the exit path > everyone takes - all paths reaching it go though an unlikely branch. > The common ones are actually "got all locks, everything's stable, nobody > has grabbed any references" (return true a couple of lines prior) or > "the sucker has grown references while it sat on the shrink list" as the > second (considerably more rare) option (the very first return false in > that function). Quite right. I just woke up and my brain had figured that out while I was sleeping. Thanks.
diff --git a/fs/dcache.c b/fs/dcache.c index 0c78ef4bb5e7..c159a4b304cf 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -1028,7 +1028,8 @@ static bool shrink_lock_dentry(struct dentry *dentry) return true; spin_unlock(&parent->d_lock); out: - spin_unlock(&inode->i_lock); + if (inode) + spin_unlock(&inode->i_lock); return false; }