Re: fs: NULL deref in atime_needs_update

[Al Viro]

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