From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-f170.google.com ([209.85.220.170]:33822 "EHLO mail-qk0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752015AbcGUDUY (ORCPT ); Wed, 20 Jul 2016 23:20:24 -0400 Received: by mail-qk0-f170.google.com with SMTP id o67so63212154qke.1 for ; Wed, 20 Jul 2016 20:20:24 -0700 (PDT) Subject: Re: [Bug] fs/dcache.c: NULL pointer dereference on dentry_string_cmp To: Al Viro , Feifei Xu References: <83724554-69c8-2b87-8e43-7ad252ec18c8@linux.vnet.ibm.com> <20160720055941.GJ2356@ZenIV.linux.org.uk> Cc: linux-fsdevel@vger.kernel.org, xuhilar@gmail.com, boqun.feng@gmail.com From: hejianet Message-ID: <57903F70.5030206@gmail.com> Date: Thu, 21 Jul 2016 11:20:16 +0800 MIME-Version: 1.0 In-Reply-To: <20160720055941.GJ2356@ZenIV.linux.org.uk> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 >