From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from zeniv.linux.org.uk ([195.92.253.2]:53524 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750800AbcGUETE (ORCPT ); Thu, 21 Jul 2016 00:19:04 -0400 Date: Thu, 21 Jul 2016 05:18:57 +0100 From: Al Viro To: hejianet Cc: Feifei Xu , linux-fsdevel@vger.kernel.org, xuhilar@gmail.com, boqun.feng@gmail.com Subject: Re: [Bug] fs/dcache.c: NULL pointer dereference on dentry_string_cmp Message-ID: <20160721041857.GK2356@ZenIV.linux.org.uk> References: <83724554-69c8-2b87-8e43-7ad252ec18c8@linux.vnet.ibm.com> <20160720055941.GJ2356@ZenIV.linux.org.uk> <57903F70.5030206@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <57903F70.5030206@gmail.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Jul 21, 2016 at 11:20:16AM +0800, hejianet wrote: > > 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? Hash insertion does smp_store_release(). Hash chain traversal - smp_read_barrier_depends(). On ppc the former is lwsync, while the latter is no-op, so it boils down to store dentry->d_name.name lwsync store mangled address of dentry into hash chain vs. fetch mangled address of dentry demangle it fetch dentry->d_name.name which should be enough - lwsync paired with address dependency gives the ordering. IOW, it's not about the barriers in __d_alloc(), it's those in hlist_bl_add_head_rcu() and hlist_bl_for_each_entry_rcu(). And it couldn't be a missing barrier anyway - crash dump shows that sucker with NULL ->d_name.name.