[PATCH 1/1] 2.6.39-rc7+ fs: Fix spinlock recursion in get_active_super()

[PATCH 1/1] 2.6.39-rc7+ fs: Fix spinlock recursion in get_active_super()

[Tim Gardner]

>From c7d9161350188c8132210eea5c7f6edff94e6c9c Mon Sep 17 00:00:00 2001
From: Tim Gardner <tim.gardner@canonical.com>
Date: Wed, 18 May 2011 10:30:02 -0600
Subject: [PATCH] fs: Fix spinlock recursion in get_active_super()

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 fs/super.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 8a06881..e203e2d 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -503,8 +503,8 @@ struct super_block *get_active_super(struct block_device *bdev)
 	if (!bdev)
 		return NULL;
 
-restart:
 	spin_lock(&sb_lock);
+restart:
 	list_for_each_entry(sb, &super_blocks, s_list) {
 		if (list_empty(&sb->s_instances))
 			continue;
-- 
1.7.0.4

Re: [PATCH 1/1] 2.6.39-rc7+ fs: Fix spinlock recursion in get_active_super()

[Christoph Hellwig]

How can you reproduce that recursion?  Given that grab_super drops
sb_lock I can't see any way to hit it.

Re: [PATCH 1/1] 2.6.39-rc7+ fs: Fix spinlock recursion in get_active_super()

[Tim Gardner]

On 05/18/2011 12:15 PM, Christoph Hellwig wrote:
> How can you reproduce that recursion?  Given that grab_super drops
> sb_lock I can't see any way to hit it.
>

Dang, you're right. Consider it NAKd

rtg
-- 
Tim Gardner tim.gardner@canonical.com

Re: [PATCH 1/1] 2.6.39-rc7+ fs: Fix spinlock recursion in get_active_super()

[Al Viro]

On Wed, May 18, 2011 at 10:35:00AM -0600, Tim Gardner wrote:
> >From c7d9161350188c8132210eea5c7f6edff94e6c9c Mon Sep 17 00:00:00 2001
> From: Tim Gardner <tim.gardner@canonical.com>
> Date: Wed, 18 May 2011 10:30:02 -0600
> Subject: [PATCH] fs: Fix spinlock recursion in get_active_super()
> 
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
>  fs/super.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 8a06881..e203e2d 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -503,8 +503,8 @@ struct super_block *get_active_super(struct block_device *bdev)
>  	if (!bdev)
>  		return NULL;
>  
> -restart:
>  	spin_lock(&sb_lock);
> +restart:
>  	list_for_each_entry(sb, &super_blocks, s_list) {
>  		if (list_empty(&sb->s_instances))
>  			continue;

WTF?  Have you even tried that?  The *only* place that contains goto restart
is a few line below and it's
                        if (grab_super(sb)) /* drops sb_lock */
                                return sb;
                        else
                                goto restart;
See that comment in there?  Now let's see if it's true:

static int grab_super(struct super_block *s) __releases(sb_lock)
{
        if (atomic_inc_not_zero(&s->s_active)) {
                spin_unlock(&sb_lock);
                return 1;
        }
        /* it's going away */
        s->s_count++;
        spin_unlock(&sb_lock);
        /* wait for it to die */
        down_write(&s->s_umount);
        up_write(&s->s_umount);
        put_super(s);
        return 0;
}

Note spin_unlock on both paths.  Morever, note blocking operations on the
path that returns 0.  If we had somehow managed to get through that without
dropping sb_locked we'd be FUBAR for obvious reasons.

IOW, if your testing had *ever* hit that goto, you'd get instant trouble.
On the exit from get_active_super() you'd hit spin_unlock(&sb_lock), with
rather nasty consequences the next time somebody would try to get it...