linux-kernel - Re: [PATCH] fs: Work around NFS wreckage

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 13 Jan 2011 15:12:50 +0100
From:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Nick Piggin <npiggin@...nel.dk>,
	LKML <linux-kernel@...r.kernel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-fsdevel@...r.kernel.org,
	"Ramirez Luna, Omar" <omar.ramirez@...com>
Subject: Re: [PATCH] fs: Work around NFS wreckage

Hello,

On Thu, Jan 13, 2011 at 02:54:30PM +0100, Thomas Gleixner wrote:
> The dcache scalability work broke NFS root filesystems.
> 
> "cd /" results in the following problem:
> 
>     link_path_walk("/",...);
> 	jumps to return_reval
> 	need_reval_dot() returns true for NFS
> 	d_revalidate()
> 		dentry->d_op->d_revalidate(dentry, nd);
> 		 	returns -ECHILD due to nd->flags & LOOKUP_RCU
> 		nameidata_dentry_drop_rcu()
> 			spin_lock(&parent->d_lock);
> 			spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
> 
> 			This deadlocks because dentry == parent
> 
> This problem exists for any filesystem which implements d_revalidate.
> 
> Uwe bisected is down to commit 34286d6(fs: rcu-walk aware d_revalidate
> method), but reverting that patch causes different wreckage to show up.
> 
> Check for parent equal dentry and skip the nested lock to avoid the
> deadlock. I'm sure this is the wrong fix, but at least it "works" :)
> 
> Reported-by: Uwe Kleine-Koenig <u.kleine-koenig@...gutronix.de>
> Reported-by: "Ramirez Luna, Omar" <omar.ramirez@...com>
> Not-Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
>  fs/namei.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> Index: linux-2.6/fs/namei.c
> ===================================================================
> --- linux-2.6.orig/fs/namei.c
> +++ linux-2.6/fs/namei.c
> @@ -487,6 +487,8 @@ static int nameidata_dentry_drop_rcu(str
>  			goto err_root;
>  	}
>  	spin_lock(&parent->d_lock);
> +	if (parent == dentry)
> +		goto same;
>  	spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
>  	if (!__d_rcu_to_refcount(dentry, nd->seq))
>  		goto err;
> @@ -499,6 +501,8 @@ static int nameidata_dentry_drop_rcu(str
>  	BUG_ON(!parent->d_count);
>  	parent->d_count++;
>  	spin_unlock(&dentry->d_lock);
> +
> +same:
>  	spin_unlock(&parent->d_lock);
>  	if (nd->root.mnt) {
>  		path_get(&nd->root);
> 
Note there is a different patch available in the thread here:

	http://thread.gmane.org/gmane.linux.kernel/1087013/focus=1087048

The differences are that it tests for IS_ROOT(dentry) instead of parent ==
dentry (which looks more reasonable IMVHO) and that it increases
parent->d_count even if the test triggered.
(And it doesn't skip the BUG_ONs which hopefully doesn't make a
difference.)

Note I really have no glue about the code below fs/, but I wonder if
the toplevel directories of mounts need some treatment here, too.  (But
I expect that they don't.  So I ask just in case ...)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/