From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wu Fengguang <fengguang.wu@intel.com>
Subject: Re: [PATCH][BUG] Lack of mutex_lock in drop_pagecache_sb()
Date: Mon, 23 Mar 2009 18:38:46 +0800
Message-ID: <20090323103846.GA16577@localhost>
References: <20090318170237.8F6C.61FB500B@jp.fujitsu.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: linux-fsdevel@vger.kernel.org
To: Masasyoshi MIZUMA <m.mizuma@jp.fujitsu.com>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mga03.intel.com ([143.182.124.21]:40985 "EHLO mga03.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753610AbZCWKjE (ORCPT <rfc822;linux-fsdevel@vger.kernel.org>);
	Mon, 23 Mar 2009 06:39:04 -0400
Content-Disposition: inline
In-Reply-To: <20090318170237.8F6C.61FB500B@jp.fujitsu.com>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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).

Is this a real producible bug or a theory one?
IMHO the I_FREEING flag should avoid the race.

> ----------------------------------------------------------------------
> 
> 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.

Thanks,
Fengguang