Re: fs: NULL deref in atime_needs_update

From: Al Viro <viro@ZenIV.linux.org.uk>
To: Dmitry Vyukov <dvyukov@google.com>
Cc: "Ian Kent" <raven@themaw.net>, "Mickaël Salaün" <mic@digikod.net>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	syzkaller <syzkaller@googlegroups.com>,
	"Kostya Serebryany" <kcc@google.com>,
	"Alexander Potapenko" <glider@google.com>,
	"Sasha Levin" <sasha.levin@oracle.com>,
	"Linus Torvalds" <torvalds@linux-foundation.org>,
	"David Howells" <dhowells@redhat.com>
Subject: Re: fs: NULL deref in atime_needs_update
Date: Sun, 28 Feb 2016 17:01:34 +0000	[thread overview]
Message-ID: <20160228170133.GM17997@ZenIV.linux.org.uk> (raw)
In-Reply-To: <CACT4Y+YztEDaepwKnzOnh=0+BN1NdpoCVOE-FLXquvKfNgndMg@mail.gmail.com>

[dhowells Cc'd]

On Sun, Feb 28, 2016 at 05:04:19PM +0100, Dmitry Vyukov wrote:

> 
> [ 1422.292356] ------------[ cut here ]------------
> [ 1422.292841] WARNING: CPU: 0 PID: 32603 at fs/namei.c:1587
> lookup_fast+0x3fa/0x450()

Huh?  So you have
                dentry = __d_lookup_rcu(parent, &nd->last, &seq);
returning non-NULL dentry, then
                *inode = d_backing_inode(dentry);
                negative = d_is_negative(dentry);
                if (read_seqcount_retry(&dentry->d_seq, seq))
                        return -ECHILD;
followed by by *inode == NULL and negative == true?

Nuts...  OK, that removes vfsmounts from consideration, but...  How the
fuck is that possible?  We have
		smp_rmb();
                seq = &dentry->d_seq->sequence & ~1;
see that ->d_name and ->d_parent match what we are looking for,
then
		*inode = dentry->d_inode;
		type = READ_ONCE(dentry->d_flags);
		smp_rmb();
		negative = (type & DCACHE_ENTRY_TYPE) == DCACHE_MISS_TYPE;
		smp_rmb();
		if (dentry->d_seq->sequence != seq)
			sod off
and observe *inode == NULL && !negative

Erm...  What's to order ->d_inode and ->d_flags fetches there?  David?
Looks like the barrier in d_is_negative() is on the wrong side of fetch.
Confused...