From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751326AbdJCIq4 (ORCPT ); Tue, 3 Oct 2017 04:46:56 -0400 Received: from m12-13.163.com ([220.181.12.13]:45947 "EHLO m12-13.163.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751128AbdJCIqz (ORCPT ); Tue, 3 Oct 2017 04:46:55 -0400 Subject: Re: [BUG] fs/dcache: might_sleep is called under a spinlock To: Al Viro Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org References: <20171003031920.GI21978@ZenIV.linux.org.uk> From: Jia-Ju Bai Message-ID: Date: Tue, 3 Oct 2017 16:46:47 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <20171003031920.GI21978@ZenIV.linux.org.uk> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-CM-TRANSID: DcCowAAnSVp4TtNZe6jFLw--.26998S2 X-Coremail-Antispam: 1Uf129KBjvJXoWxuryrGr17Xw4UGF1kJFy3XFb_yoW5WrWUpF y7tas0qr95JFWI9w48AF409w1Sv3W29rW5JF1UW348Ar15WryIqFnFqF4j9FyFyrW3CF4I qF1jgrn5CrZxXaDanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDUYxBIdaVFxhVjvjDU0xZFpf9x07juuWLUUUUU= X-Originating-IP: [183.172.144.55] X-CM-SenderInfo: xedlyx5dmximizq6il2tof0z/1tbiHhhmelSIXTQVcgAAsI Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thanks for your detailed explanation :) I will improve my static analysis tool. Thanks, Jia-Ju Bai On 2017/10/3 11:19, Al Viro wrote: > On Tue, Oct 03, 2017 at 10:38:25AM +0800, Jia-Ju Bai wrote: >> According to fs/dcache.c, might_sleep is called under a spinlock, >> and the function call path is: >> d_prune_aliases (acquire the spinlock) >> dput >> might_sleep >> >> This bug is found by my static analysis tool and my code review. >> A possible fix is to remove might_sleep in dput. > ... or to fix your static analysis tool. First of all, that call > of dput() really *can* block and if we had inode->i_lock or dentry->d_lock > still held at that point we'd have a real bug. However, __dentry_kill() > there is called with dentry->d_inode == inode and inode->i_lock held, > so dentry->d_inode is stable until inode->i_lock is dropped. Said > __dentry_kill() contains > if (dentry->d_inode) > dentry_unlink_inode(dentry); > with inode->i_lock held until that point. dentry_unlink_inode() starts > with > struct inode *inode = dentry->d_inode; > bool hashed = !d_unhashed(dentry); > > if (hashed) > raw_write_seqcount_begin(&dentry->d_seq); > __d_clear_type_and_inode(dentry); > hlist_del_init(&dentry->d_u.d_alias); > if (hashed) > raw_write_seqcount_end(&dentry->d_seq); > spin_unlock(&dentry->d_lock); > spin_unlock(&inode->i_lock); > so > 1) inode in there is guaranteed to be equal to the argument of > d_prune_aliases() and > 2) both dentry->d_lock and inode->i_lock are dropped before > dentry_unlink_inode() returns. inode->i_lock is not regained in the > rest of __dentry_kill(); dentry->d_lock is regained and dropped before > __dentry_kill() returns. IOW, we are fine - dput() in d_prune_aliases() > is called without any spinlocks held. > > That, BTW, is the reason for > goto restart; > in there, instead of just continuing the loop - if we get to that point, > the list of aliases might have changed. > > Removing might_sleep() in dput() would've been wrong - it really might > sleep when called from that point. Here's how: we used to have two > links to the same file - foo/bar and baz/barf. baz/barf used to be > opened, then rm -rf baz happened and later we'd called d_prune_aliases() > on the inode of foo/bar. And as the loop had been executed on one CPU, > on another the opened file got closed, dropping the last reference to > dentry that used to be baz/barf. Note that its parent (the thing that > used to be dentry of baz) is unhashed and the only contributor to its > refcount is our dentry, so dput(parent) *does* drop the last remaining > reference, triggering the final iput() on inode of baz, along with > freeing on-disk inode, doing disk IO, etc. > > Again, it's not that we can't block in that dput() - it's that __dentry_kill() > drops all spinlocks.