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

[hejianet <hejianet@gmail.com>]

On 7/20/16 1:59 PM, Al Viro wrote:
> 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.
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?
B.R.
Justin
>
> 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.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>