[PATCH vfs/for-next] fs/dcache.c: fix NULL pointer dereference in shrink_lock_dentry()

[PATCH vfs/for-next] fs/dcache.c: fix NULL pointer dereference in shrink_lock_dentry()

[Eric Biggers]

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.

Fixes: 121a8e083486 ("get rid of trylock loop in locking dentries on shrink list")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/dcache.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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;
 }
 
-- 
2.17.0.rc0.231.g781580f067-goog

Re: [PATCH vfs/for-next] fs/dcache.c: fix NULL pointer dereference in shrink_lock_dentry()

[Matthew Wilcox]

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)

Re: [PATCH vfs/for-next] fs/dcache.c: fix NULL pointer dereference in shrink_lock_dentry()

[Al Viro]

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...

Re: [PATCH vfs/for-next] fs/dcache.c: fix NULL pointer dereference in shrink_lock_dentry()

[Matthew Wilcox]

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.