fs/dcache.c: fix NULL pointer dereference in shrink_lock_dentry()
PATCH 1/1 fs/dcache.c: fix NULL pointer dereference in shrink_lock_dentry()
From: Eric Biggers <[email protected]>
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.
--- fs/dcache.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
fs/dcache.c +2 -1
@@ -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; }
Signature · 1 line
2.17.0.rc0.231.g781580f067-goog
Quoted 8 lines · Eric Biggers
From: Eric Biggers <[email protected]>
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 <[email protected]> in case we end up choosing this variant)
Al Viro
maintainerQuoted 28 lines · Matthew Wilcox
Quoted 8 lines · Eric Biggers
From: Eric Biggers <[email protected]>
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);(quoted diff) +3
-1- 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;
Quoted 2 lines
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...
Quoted 4 lines · Al Viro
Quoted 2 lines · Matthew Wilcox
That puts the comparison out-of-line rather than in the exit path that everybody uses.
Quoted 9 lines
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.