From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from zeniv.linux.org.uk ([195.92.253.2]:59015 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753709AbcA3VMj (ORCPT ); Sat, 30 Jan 2016 16:12:39 -0500 Date: Sat, 30 Jan 2016 21:12:38 +0000 From: Al Viro To: Santosh Madiraju Cc: linux-fsdevel@vger.kernel.org Subject: Re: [PATCH 2/2] fs: fix bug where dentry freed earlier, might be getting freed again. Message-ID: <20160130211237.GG17997@ZenIV.linux.org.uk> References: <1454187106-10827-1-git-send-email-madiraju.santosh@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1454187106-10827-1-git-send-email-madiraju.santosh@gmail.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Sat, Jan 30, 2016 at 12:51:46PM -0800, Santosh Madiraju wrote: > From: madiraju > > While shrinking the dentry list in shrink_dentry_list function, > if DCACHE_MAY_FREE flag is set, it frees the dentry, and it again > tries to do it in dentry_kill. [snip] > diff --git a/fs/dcache.c b/fs/dcache.c > index 92d5140..7aa2252 100644 > --- a/fs/dcache.c > +++ b/fs/dcache.c > @@ -986,8 +986,8 @@ static void shrink_dentry_list(struct list_head *list) > spin_unlock(&parent->d_lock); > continue; > } > - > - __dentry_kill(dentry); > + if (dentry) > + __dentry_kill(dentry); Considering the fact that this call of __dentry_kill() is immediately preceded by inode = dentry->d_inode; if (inode && unlikely(!spin_trylock(&inode->i_lock))) { d_shrink_add(dentry, list); spin_unlock(&dentry->d_lock); if (parent) spin_unlock(&parent->d_lock); continue; } it is flat-out impossible to get to that call with dentry equal to NULL - we would've oopsed on attempt to fetch dentry->d_inode, not to mention that the value in dentry ultimately comes from dentry = list_entry(list->prev, struct dentry, d_lru); which _never_ yields NULL. And that access of dentry->d_inode is not the only place in between where we would've oopsed with dentry being NULL, while we are at it. And description also makes no sense whatsoever - if we run into DCACHE_MAY_FREE (which can't be set without DCACHE_DENTRY_KILLED having been set first), we won't even reach that __dentry_kill() - we'll run into if (unlikely(dentry->d_flags & DCACHE_DENTRY_KILLED)) { bool can_free = dentry->d_flags & DCACHE_MAY_FREE; spin_unlock(&dentry->d_lock); if (parent) spin_unlock(&parent->d_lock); if (can_free) dentry_free(dentry); continue; } and that's it as far as this dentry is concerned. We'd done d_shrink_del(dentry) - that's the very first thing we do and we do it unconditionally. What's more, making no sense is about the only thing description and patch have in common. NAK.