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

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

On Thu, Jul 14, 2016 at 09:25:41PM +0800, Feifei Xu wrote:
> Hi,
> 
> I met crashes on ppc64le machine.
> 
> Call trace: lookup_fast( ) -> __d_lookup_rcu( ) -> dentry_cmp( ) ->
> dentry_string_cmp ( )
> From the symbolized trace and disassembly code, when doing
> dentry_string_cmp(),
> dentry.d_name->name is NULL , this dereference triggered crash.
> 
> The dentry's data when crash happens: http://paste.ubuntu.com/19340635/.
> And the analysis of the crash vmcore here if you're interested:
> http://paste.ubuntu.com/19359665/
> 
> Can we add check before at the begging of dentry_string_cmp() as below?
> Or maybe we should not silently ignore the NULL pointer.
> static inline int dentry_string_cmp(const unsigned char *cs, const unsigned
> char *ct, unsigned tcount)
>  {
>         do {
> +             if (unlikely(!cs || !ct ))
> +                     return 1;
>                 if (*cs != *ct)
>                         return 1;
>                 cs++;

No, we can not.  Any time you get NULL there is a bug, plain and simple.
Your crash dump shows an impossible state - dentry->d_name.name equal to
NULL.  This should never happen, period.  What's more, you have that
NULL ->d_name.name in crash dump, so it's not a missing barrier somewhere...

> [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?
	c) how hard it is to reproduce?  You've mentioned "crashes", which
sounds like there had been more than one...

Another question: which filesystem type had that been?  If you have that
crashdump, dentry->d_sb->s_type->name should give that information...

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.