From mboxrd@z Thu Jan 1 00:00:00 1970 From: Masayoshi MIZUMA Subject: Re: [PATCH][BUG] Lack of mutex_lock in drop_pagecache_sb() Date: Tue, 24 Mar 2009 16:06:45 +0900 Message-ID: <20090324155655.2684.61FB500B@jp.fujitsu.com> References: <20090318170237.8F6C.61FB500B@jp.fujitsu.com> <20090323103846.GA16577@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit Cc: linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk To: Wu Fengguang Return-path: Received: from fgwmail7.fujitsu.co.jp ([192.51.44.37]:41403 "EHLO fgwmail7.fujitsu.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750978AbZCXHGH (ORCPT ); Tue, 24 Mar 2009 03:06:07 -0400 Received: from m1.gw.fujitsu.co.jp ([10.0.50.71]) by fgwmail7.fujitsu.co.jp (Fujitsu Gateway) with ESMTP id n2O762tj001170 for (envelope-from m.mizuma@jp.fujitsu.com); Tue, 24 Mar 2009 16:06:02 +0900 Received: from smail (m1 [127.0.0.1]) by outgoing.m1.gw.fujitsu.co.jp (Postfix) with ESMTP id 9EF8745DD76 for ; Tue, 24 Mar 2009 16:06:02 +0900 (JST) Received: from s1.gw.fujitsu.co.jp (s1.gw.fujitsu.co.jp [10.0.50.91]) by m1.gw.fujitsu.co.jp (Postfix) with ESMTP id 73AD345DD75 for ; Tue, 24 Mar 2009 16:06:02 +0900 (JST) Received: from s1.gw.fujitsu.co.jp (localhost.localdomain [127.0.0.1]) by s1.gw.fujitsu.co.jp (Postfix) with ESMTP id 736671DB8016 for ; Tue, 24 Mar 2009 16:06:02 +0900 (JST) Received: from m108.s.css.fujitsu.com (m108.s.css.fujitsu.com [10.249.87.108]) by s1.gw.fujitsu.co.jp (Postfix) with ESMTP id 244C7E08002 for ; Tue, 24 Mar 2009 16:06:02 +0900 (JST) In-Reply-To: <20090323103846.GA16577@localhost> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Hi, Fengguang On Mon, 23 Mar 2009 18:38:46 +0800 Wu Fengguang wrote: > Masasyoshi, > > On Wed, Mar 18, 2009 at 05:13:35PM +0900, Masasyoshi MIZUMA wrote: > > I create the patch which fixes lack of mutex_lock in drop_pagecache_sb(). > > Please check the bug and the patch (below). > Thank you for your comment, and I apologize to you for my lack of explanation. > Is this a real producible bug or a theory one? This is a real bug. > IMHO the I_FREEING flag should avoid the race. I supplement the explanation for this problem. clear_inode() is called by dispose_list(), and sets the inode's i_state to I_CLEAR. Therefore, the following conditional expression doesn't match for the inode: "if (inode->i_state & (I_FREEING|I_WILL_FREE)) continue;" As the result, this problem can happen. > > > ---------------------------------------------------------------------- > > > > When drop_pagecache_sb() frees inodes, it doesn't get mutex_lock of > > iprune_mutex. Therefore, if it races the process which frees inodes > > (ex. prune_icache()), OS panic may happen. > > > > An example of the panic flow is the following: > > ---------------------------------------------------------------------- > > [process A] | [process B] > > | | > > | shrink_icache_memory() | > > | | | > > | V | > > | prune_icache() | drop_pagecache() > > | mutex_lock(&iprune_mutex) | | > > | spin_lock(&inode_lock) | | > > | | | V > > | | | drop_pagecache_sb() > > | | | | > > inode->i_state |= I_FREEING; > > > | V | V > > | spin_unlock(&inode_lock) | spin_lock(&inode_lock) > > | | | | > > if (inode->i_state & (I_FREEING|I_WILL_FREE)) > continue; > > > | | | | > > | V | V > > | dispose_list() | __iget() > > | list_del() | | > > | | | | > > | V | V > > | spin_lock(&inode_lock) | list_move() <----- PANIC !! > > | | > > V | > > (time) > > ---------------------------------------------------------------------- > > If the inode which Process B do list_move() with is the same as the one which > > Process A did list_del() with, OS may panic. I applied your comment and then modified the panic flow figure. Please check below: ---------------------------------------------------------------------- [process A] | [process B] | | | shrink_icache_memory() | | | | | V | | prune_icache() | drop_pagecache() | mutex_lock(&iprune_mutex) | | | spin_lock(&inode_lock) | | | | | V | | | drop_pagecache_sb() | | | | | V | | | inode->i_state |= I_FREEING; | | | | | | | V | V | spin_unlock(&inode_lock) | spin_lock(&inode_lock) | | | | | | | | | V | | | dispose_list() | | | list_del() | | | | | | | V | | | clear_inode() | | | inode->i_state = I_CLEAR | | | | | | | | | V | | | if (inode->i_state & (I_FREEING|I_WILL_FREE)) | | | continue; <---- NOT MATCH | | | | | | | V | | | __iget() | | | | | V | V | spin_lock(&inode_lock) | list_move() <----- PANIC !! | | V | (time) ---------------------------------------------------------------------- Thanks, Masayoshi MIZUMA