linux-kernel - Re: [PATCH RESEND 1/1] vfs: Really check for inode ptr in lookup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191102172447.GU20975@paulmck-ThinkPad-P72>
Date:   Sat, 2 Nov 2019 10:24:47 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        wugyuan@...ibm.com, jlayton@...nel.org, hsiangkao@....com,
        Jan Kara <jack@...e.cz>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Ritesh Harjani <riteshh@...ux.ibm.com>
Subject: Re: [PATCH RESEND 1/1] vfs: Really check for inode ptr in lookup_fast

On Sat, Nov 02, 2019 at 06:17:06AM +0000, Al Viro wrote:
> On Fri, Nov 01, 2019 at 11:46:22PM +0000, Al Viro wrote:
> > on anything except alpha that would be pretty much automatic and
> > on alpha we get the things along the lines of
> > 
> > 	f = fdt[n]
> > 	mb
> > 	d = f->f_path.dentry
> > 	i = d->d_inode
> > 	assert(i != NULL)
> > vs.
> > 	see that d->d_inode is non-NULL
> > 	f->f_path.dentry = d
> > 	mb
> > 	fdt[n] = f
> > 
> > IOW, the barriers that make it safe to fetch the fields of struct file
> > (rcu_dereference_raw() in __fcheck_files() vs. smp_store_release()
> > in __fd_install() in the above) should *hopefully* take care of all
> > stores visible by the time of do_dentry_open().  Sure, alpha cache
> > coherency is insane, but AFAICS it's not _that_ insane.
> > 
> > Question to folks familiar with alpha memory model:
> > 
> > A = 0, B = NULL, C = NULL
> > CPU1:
> > 	A = 1
> > 
> > CPU2:
> > 	r1 = A
> > 	if (r1) {
> > 		B = &A
> > 		mb
> > 		C = &B
> > 	}
> > 
> > CPU3:
> > 	r2 = C;
> > 	mb
> > 	if (r2) {	// &B
> > 		r3 = *r2	// &A
> > 		r4 = *r3	// 1
> > 		assert(r4 == 1)
> > 	}
> > 
> > is the above safe on alpha?
> 
> Hmm...  After digging through alpha manuals, it should be -
> 
> U1: W A, 1
> 
> V1: R A, 1

Assuming a compare and branch here ...

> V2: W B, &A
> V3: MB
> V4: W C, &B
> 
> W1: R C, &B
> W2: MB

... and here ...

> W3: R B, &A
> W4: R A, 0
> 
> is rejected since
> 	U1 BEFORE V1 (storage and visibility)
> 	V1 BEFORE V3 BEFORE V4 (processor issue order constraints)
> 	V4 BEFORE W1 (storage and visibility)
> 	W1 BEFORE W2 BEFORE W4 (processor issue order constraints)
> and W4 BEFORE U1 (storage and visibility), which is impossible
> due to BEFORE being acyclic and transitive.
> 
> I might very well be missing something, though...  Paul, could you
> take a look and tell if the above makes sense?

...  then yes, agreed.  Alpha does respect control dependencies to
stores, and you supplied the required mb for the last task that has
a control dependency only to loads.

I have to ask...  Are you seeing failures on Alpha?

							Thanx, Paul