[PATCH] Fix for bad lock balance in Containers

[PATCH] Fix for bad lock balance in Containers

[Dhaval Giani]

Hi,

I have been going through the containers code and trying it out. I tried
mounting the same hierarchy at two different points and I got a bad
locking balance warning.

=====================================
[ BUG: bad unlock balance detected! ]
-------------------------------------
mount/4467 is trying to release lock (&type->s_umount_key) at:
[<c017de3b>] vfs_kern_mount+0x5b/0x70
but there are no more locks to release!

other info that might help us debug this:
no locks held by mount/4467.

stack backtrace:
 [<c0105b9f>] show_trace_log_lvl+0x19/0x2e
 [<c0105bc6>] show_trace+0x12/0x14
 [<c0105cb1>] dump_stack+0x14/0x16
 [<c0142c1a>] print_unlock_inbalance_bug+0xcc/0xd6
 [<c0142c93>] check_unlock+0x6f/0x75
 [<c0142ee0>] __lock_release+0x1e/0x51
 [<c014312c>] lock_release+0x4c/0x64
 [<c013bd9d>] up_write+0x16/0x2b
 [<c017de3b>] vfs_kern_mount+0x5b/0x70
 [<c018fae7>] do_new_mount+0x85/0xe6
 [<c019013f>] do_mount+0x185/0x199
 [<c01903ab>] sys_mount+0x71/0xa6
 [<c0104d6a>] sysenter_past_esp+0x5f/0x99
 =======================

Going through the code, I realised this is because when the container is
already mounted at one point, when being remounted, it just does a
simple_set_mnt which does not update s_umount which causes the warning
to come. This also cause umount to hang the second time.

The correct method would be allocate the superblock using sget (or a
variant) which would call grab_super correctly and set these locks.
Seeing the code which is there, some part of grab_super is already done
in container_get_sb where the root->sb->s_active is updated. So I have
called down_write there as well to get the locking balance. However I
believe that this is not the correct approach.

There are a few questions I had with respect to the current code,

Why is the increment of s_active dependent on the return value of
simple_set_mnt? All the other functions which I have seen (fs ones),
grab_super (which increments s_active) is called followed by a
simple_set_mnt.

What should be the correct approach to get the locking balance? As far
as I can see, the correct method would be to call sget which would then
correctly handle everything. But that would require a test function. I
saw functionality similar to a test function in the beginning of
container_get_sb(). Should that be seperated and put in a seperate test
function so that sget can be called?

Another possible approach would be to call grab_super directly (since we
know the test function is going to return true), but this cannot be done
since grab_super is static right now. Or maybe we could duplicate
grab_super's functionality.

In the meantime a temporary fix.

Thanks and regards
Dhaval

Signed-off-by: Dhaval Giani <dhaval@linux.vnet.ibm.com>


--- linux-2.6.22-rc4/kernel/container.c	2007-06-13 15:38:32.000000000 +0530
+++ old/kernel/container.c	2007-06-25 00:55:03.000000000 +0530
@@ -995,6 +995,7 @@ static int container_get_sb(struct file_
 		BUG_ON(ret);
 	} else {
 		/* Reuse the existing superblock */
+		down_write(&(root->sb->umount));
 		ret = simple_set_mnt(mnt, root->sb);
 		if (!ret)
 			atomic_inc(&root->sb->s_active);

Re: [PATCH] Fix for bad lock balance in Containers

[Bharata B Rao]

On Tue, Jun 26, 2007 at 10:30:11AM +0530, Dhaval Giani wrote:
> 
> --- linux-2.6.22-rc4/kernel/container.c	2007-06-13 15:38:32.000000000 +0530
> +++ old/kernel/container.c	2007-06-25 00:55:03.000000000 +0530
> @@ -995,6 +995,7 @@ static int container_get_sb(struct file_
>  		BUG_ON(ret);
>  	} else {
>  		/* Reuse the existing superblock */
> +		down_write(&(root->sb->umount));
>  		ret = simple_set_mnt(mnt, root->sb);
>  		if (!ret)
>  			atomic_inc(&root->sb->s_active);

Do you want to change this to first take refererence on sb and then
do simple_set_mnt() ?

And btw, simple_set_mnt() always returns zero and looks like it can't fail.

Regards,
Bharata.

Re: [PATCH] Fix for bad lock balance in Containers

[Dhaval Giani]

On Tue, Jun 26, 2007 at 10:30:11AM +0530, Dhaval Giani wrote:
Hi,

There was a mistake in the patch. Thanks to Andrew Morton for pointing it out.
Sending out a fresh patch. Sorry for the mistake!
 
>  		BUG_ON(ret);
>  	} else {
>  		/* Reuse the existing superblock */
> +		down_write(&(root->sb->umount));

Should be down_write(&(root->sb->s_umount));

>  		ret = simple_set_mnt(mnt, root->sb);
>  		if (!ret)
>  			atomic_inc(&root->sb->s_active);

Signed-off-by: Dhaval Giani <dhaval@linux.vnet.ibm.com>


--- linux-2.6.22-rc4/kernel/container.c	2007-06-13 15:38:32.000000000 +0530
+++ old/kernel/container.c	2007-06-25 00:55:03.000000000 +0530
@@ -995,6 +995,7 @@ static int container_get_sb(struct file_
 		BUG_ON(ret);
 	} else {
 		/* Reuse the existing superblock */
+		down_write(&(root->sb->s_umount));
 		ret = simple_set_mnt(mnt, root->sb);
 		if (!ret)
 			atomic_inc(&root->sb->s_active);

Re: [PATCH] Fix for bad lock balance in Containers

[Paul Menage]

On 6/26/07, Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:
>
> There are a few questions I had with respect to the current code,
>
> Why is the increment of s_active dependent on the return value of
> simple_set_mnt?

I think it's because, as you observed, grab_super() is static and
hence not reachable from container.c. But I thought that the only side
effect of it that we needed (since we had a safe pointer to the
superblock not obtained via the superblock list) was the increment of
sb_active. (As it turns out I was wrong and we needed the s_umount
lock too).

Incrementing the sb_active count (if simple_set_mnt() failed) seemed
as though it would be wrong.

>
> What should be the correct approach to get the locking balance? As far
> as I can see, the correct method would be to call sget which would then
> correctly handle everything. But that would require a test function. I
> saw functionality similar to a test function in the beginning of
> container_get_sb(). Should that be seperated and put in a seperate test
> function so that sget can be called?

I don't quite remember why I did it this way originally, now. I've got
no objection to the code being cleaned up to fit the more common
approach if it's practical to do so, but it seems to me that your
current patch fix seems enough at least for now.

Thanks,

Paul