From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from zeniv.linux.org.uk ([195.92.253.2]:56582 "EHLO
        ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750743AbeCXEu6 (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Sat, 24 Mar 2018 00:50:58 -0400
Date: Sat, 24 Mar 2018 04:50:54 +0000
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Matthew Wilcox <willy@infradead.org>
Cc: Eric Biggers <ebiggers3@gmail.com>, linux-fsdevel@vger.kernel.org,
        John Ogness <john.ogness@linutronix.de>,
        Eric Biggers <ebiggers@google.com>
Subject: Re: [PATCH vfs/for-next] fs/dcache.c: fix NULL pointer dereference
 in shrink_lock_dentry()
Message-ID: <20180324045054.GL30522@ZenIV.linux.org.uk>
References: <20180323230443.168482-1-ebiggers3@gmail.com>
 <20180324043735.GB22733@bombadil.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180324043735.GB22733@bombadil.infradead.org>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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