Re: fs: NULL deref in atime_needs_update

From: Dmitry Vyukov <dvyukov@google.com>
To: Al Viro <viro@zeniv.linux.org.uk>
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: Mon, 29 Feb 2016 13:34:13 +0100	[thread overview]
Message-ID: <CACT4Y+Z=SHjUNXpm7dahCgbVX=tgC6oitY_9mmXK7FJEYK275Q@mail.gmail.com> (raw)
In-Reply-To: <CACT4Y+ZOacjiiaWLKN-W4-Gc0W8a4jfxP=cZLq9Pkgtba=cbAg@mail.gmail.com>

On Mon, Feb 29, 2016 at 10:38 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Sun, Feb 28, 2016 at 9:01 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Sun, Feb 28, 2016 at 05:01:34PM +0000, Al Viro wrote:
>>
>>> 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...
>>
>> OK, as per David's suggestion, let's flip them around, bringing the
>> barrier in d_is_negative() between them.  Dmitry, could you try this on
>> top of mainline?  Again, it's until the first warning.

I am testing the new patch for several hours in several VMs now, I
would expect a WARNING to already fire. But no warnings fired so far.
I will keep it running. But meanwhile, do you an explanation of how:

-               *inode = d_backing_inode(dentry);
                negative = d_is_negative(dentry);
+               *inode = d_backing_inode(dentry);

can fix the bug? Explanation other than a missing rmw, because I am
running on x86 so I would not expect rmb to be the root cause (though,
it still may be necessary for other arches and to prevent possible
miscompilations). If you do have it and assuming that I will not see
warnings till tomorrow, then hopefully we can consider it as fixed!

It's not that I really understand what happens here, but looking at
the diff: is it the case that negative and inode can change under our
feet? If so, we still probably can get an inconsistent picture (i.e.
negative dentry but not NULL inode), can it be an issue? Is
non-negative->negative->non-negative->negative transition possible? If
so, we still probably can get the same crash regardless of order of
negative/inode loads.