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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
