Re: [Bug] fs/dcache.c: NULL pointer dereference on dentry_string_cmp

[Al Viro <viro@ZenIV.linux.org.uk>]

On Thu, Jul 21, 2016 at 11:20:16AM +0800, hejianet wrote:

> > I don't see any likely candidates for that bug - not in mainline, not in
> > the kernel you'd been running there.  Basically, once we have obtained
> > a pointer to dentry (which should happen only in __d_alloc()[1]), it should
> > never have NULL in its ->d_name.name.  Any path through __d_alloc() that
> > doesn't end up returning NULL will pass through
> >          dname[name->len] = 0;
> > 
> >          /* Make sure we always see the terminating NUL character */
> >          smp_wmb();
> >          dentry->d_name.name = dname;
> > which obviously can't end up with dentry->d_name.name == NULL - dname would
> > have to be NULL as well, and that would oops in the first of the quoted lines.
> Maybe not
> This barrier is to guarantee that in dentry_cmp,
> if dentry->d_name.name is equal to dname (not NULL), then dname[name->len]
> should be equal to 0(dname is terminated with NULL).
> This barrier will NOT guarantee that dentry->d_name.name != NULL in dentry_cmp.
> So maybe we need to add a if statement to avoid this race condition?
> 
> Is there anything wrong with my descriptions?

	Hash insertion does smp_store_release().  Hash chain traversal -
smp_read_barrier_depends().  On ppc the former is lwsync, while the latter
is no-op, so it boils down to
	store dentry->d_name.name
	lwsync
	store mangled address of dentry into hash chain
vs.
	fetch mangled address of dentry
	demangle it
	fetch dentry->d_name.name
which should be enough - lwsync paired with address dependency gives the
ordering.  IOW, it's not about the barriers in __d_alloc(), it's those in
hlist_bl_add_head_rcu() and hlist_bl_for_each_entry_rcu().

	And it couldn't be a missing barrier anyway - crash dump shows that
sucker with NULL ->d_name.name.