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

[Feifei Xu <xufeifei@linux.vnet.ibm.com>]

On 2016/7/20 13:59, Al Viro wrote:
>> [387421.143529] CPU: 69 PID: 39485 Comm: rsync Tainted: G W
>> ------------   3.10.0-327.18.2.el7.ppc64le #1
> Wait a sec, what is that doing on l-k?  It's an RH kernel, and nowhere
> near the current one, at that...  Anyway,
> 	a) can you reproduce it with the mainline?
> 	b) can you reproduce it on more current RH kernel?
I am building the 4.6 kernel from mainline to try to reproduce it.
And also will try more current RH kernel-3.10.0-327.22.2.el7.
Will update later.
> 	c) how hard it is to reproduce?  You've mentioned "crashes", which
> sounds like there had been more than one...
I have no test case to reproduce it. But it crashes regularly. And it 
happens on 3 physical
machines (powerpc).
> Another question: which filesystem type had that been?  If you have that
> crashdump, dentry->d_sb->s_type->name should give that information...
   It is xfs.   http://paste.ubuntu.com/20278867/
> 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.
>
> Nothing should be doing wholesale assignments to ->d_name.  Nothing that
> had &dentry->d_name passed as an argument should modify its ->name field
> (most of such parameters are declared const struct qstr *, and at least in
> mainline all of them could be constified that way; I hadn't scanned the
> kernel you'd been using yet - it's several hundred calls to RTFS through ;-/)
> Nothing outside of fs/dcache.c should modify ->d_name directly either (and
> that I'd verified both for mainline and for 3.10.0-327.el7).
>
> And in fs/dcache.c we have very few places modifying that sucker.  Namely,
> swap_names() and copy_name() in mainline and switch_name() in 3.10.0-327.el7.
> Assigned values are either ->d_name.name of another dentry or ->d_iname of
> this dentry.  The latter is never NULL (it's an array embedded into struct
> dentry) and the former would have to have become NULL at some earlier point.
>
> Buggered barriers might be a possibility, but those would probably not leak
> all the way into crashdump; I rather doubt that they are buggered, anyway,
> since we have store non-NULL into ->d_name.name/lwsync/store a mangled address
> of dentry into hash chain vs. fetch a value from hash chain, demangle it into
> dentry address, load dentry->d_name.name and observe NULL there.  With
> another lwsync thrown in between the last two steps, actually, but even
> without that the lwsync in writer + address dependency in reader should've
> been enough.
>
> [1] struct dentry is never an object with static or automatic storage duration
> or a member of any kind of compound object and the only place where a pointer
> to any other kind of object is converted into pointer to struct dentry is
> dentry = kmem_cache_alloc(dentry_cache, GFP_KERNEL); in __d_alloc().  IOW,
> all instances of struct dentry must come from that function, period.
>
Thanks
Fiona