From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from bombadil.infradead.org ([198.137.202.133]:50676 "EHLO
        bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750727AbeCXEhh (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Sat, 24 Mar 2018 00:37:37 -0400
Date: Fri, 23 Mar 2018 21:37:35 -0700
From: Matthew Wilcox <willy@infradead.org>
To: Eric Biggers <ebiggers3@gmail.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
        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: <20180324043735.GB22733@bombadil.infradead.org>
References: <20180323230443.168482-1-ebiggers3@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180323230443.168482-1-ebiggers3@gmail.com>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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)