[PATCH] cgroup: fix reverse unlock sequence in cgroup_get_sb

[PATCH] cgroup: fix reverse unlock sequence in cgroup_get_sb

[Xiaotian Feng]

In cgroup_get_sb, the lock sequence is:
	mutex_lock(&inode->i_mutex);
	mutex_lock(&cgroup->mutex);
so the last unlock sequence should be:
	mutex_unlock(&cgroup->mutex);
	mutex_unlock(&inode->i_mutex);

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
---
 kernel/cgroup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 3737a68..11ef162 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1140,8 +1140,8 @@ static int cgroup_get_sb(struct file_system_type *fs_type,
 		BUG_ON(root->number_of_cgroups != 1);
 
 		cgroup_populate_dir(root_cgrp);
-		mutex_unlock(&inode->i_mutex);
 		mutex_unlock(&cgroup_mutex);
+		mutex_unlock(&inode->i_mutex);
 	}
 
 	simple_set_mnt(mnt, sb);
-- 
1.6.2.5

Re: [PATCH] cgroup: fix reverse unlock sequence in cgroup_get_sb

[Balbir Singh]

* Xiaotian Feng <dfeng@redhat.com> [2009-07-21 18:25:26]:

> In cgroup_get_sb, the lock sequence is:
> 	mutex_lock(&inode->i_mutex);
> 	mutex_lock(&cgroup->mutex);
> so the last unlock sequence should be:
> 	mutex_unlock(&cgroup->mutex);
> 	mutex_unlock(&inode->i_mutex);
> 
> Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> ---
>  kernel/cgroup.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 3737a68..11ef162 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1140,8 +1140,8 @@ static int cgroup_get_sb(struct file_system_type *fs_type,
>  		BUG_ON(root->number_of_cgroups != 1);
> 
>  		cgroup_populate_dir(root_cgrp);
> -		mutex_unlock(&inode->i_mutex);
>  		mutex_unlock(&cgroup_mutex);
> +		mutex_unlock(&inode->i_mutex);
>  	}
>

Seems reasonable to me. You might also want to mention that elsewhere
the sequence is unlock cgroup_mutex followed by inode->i_mutex.

Acked-by: Balbir Singh <balbir@linux.vnet.ibm.com>
 
 
-- 
	Balbir

Re: [PATCH] cgroup: fix reverse unlock sequence in cgroup_get_sb

[Danny Feng]

On 07/21/2009 07:10 PM, Balbir Singh wrote:
> * Xiaotian Feng<dfeng@redhat.com>  [2009-07-21 18:25:26]:
>
>> In cgroup_get_sb, the lock sequence is:
>> 	mutex_lock(&inode->i_mutex);
>> 	mutex_lock(&cgroup->mutex);
>> so the last unlock sequence should be:
>> 	mutex_unlock(&cgroup->mutex);
>> 	mutex_unlock(&inode->i_mutex);
>>
>> Signed-off-by: Xiaotian Feng<dfeng@redhat.com>
>> ---
>>   kernel/cgroup.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>> index 3737a68..11ef162 100644
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>> @@ -1140,8 +1140,8 @@ static int cgroup_get_sb(struct file_system_type *fs_type,
>>   		BUG_ON(root->number_of_cgroups != 1);
>>
>>   		cgroup_populate_dir(root_cgrp);
>> -		mutex_unlock(&inode->i_mutex);
>>   		mutex_unlock(&cgroup_mutex);
>> +		mutex_unlock(&inode->i_mutex);
>>   	}
>>
>
> Seems reasonable to me. You might also want to mention that elsewhere
> the sequence is unlock cgroup_mutex followed by inode->i_mutex.
Yep, thank you very much:-)
>
> Acked-by: Balbir Singh<balbir@linux.vnet.ibm.com>
>
>

Re: [PATCH] cgroup: fix reverse unlock sequence in cgroup_get_sb

[Balbir Singh]

* Zefan Li <lizf.kernel@gmail.com> [2009-07-21 19:38:03]:

> 2009/7/21, Balbir Singh <balbir@linux.vnet.ibm.com>:
> >
> > * Xiaotian Feng <dfeng@redhat.com> [2009-07-21 18:25:26]:
> >
> > > In cgroup_get_sb, the lock sequence is:
> > >       mutex_lock(&inode->i_mutex);
> > >       mutex_lock(&cgroup->mutex);
> > > so the last unlock sequence should be:
> > >       mutex_unlock(&cgroup->mutex);
> > >       mutex_unlock(&inode->i_mutex);
> > >
> > > Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> > > ---
> > >  kernel/cgroup.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> > > index 3737a68..11ef162 100644
> > > --- a/kernel/cgroup.c
> > > +++ b/kernel/cgroup.c
> > > @@ -1140,8 +1140,8 @@ static int cgroup_get_sb(struct file_system_type
> > *fs_type,
> > >               BUG_ON(root->number_of_cgroups != 1);
> > >
> > >               cgroup_populate_dir(root_cgrp);
> > > -             mutex_unlock(&inode->i_mutex);
> > >               mutex_unlock(&cgroup_mutex);
> > > +             mutex_unlock(&inode->i_mutex);
> > >       }
> > >
> >
> > Seems reasonable to me. You might also want to mention that elsewhere
> > the sequence is unlock cgroup_mutex followed by inode->i_mutex.
> >
> > Acked-by: Balbir Singh balbir@linux.vnet.ibm.com
> 
> 
> No, the unlock order is irrelevant. It's the lock order that matters. So
> this patch
> fixes nothing.
> 
> Xiaotian, you didn't run into deadlock, did you?
>


Li, Consider the following


lock(A)
lock(B)
unlock(A) 
unlock(B)

Tomorrow if a unsuspecting programmer does this

lock(A)
lock(B)
unlock(A) 

code block 

unlock(B)


What protects code block? lock B? Is that the intention?

-- 
	Balbir

Re: [PATCH] cgroup: fix reverse unlock sequence in cgroup_get_sb

[Paul Menage]

On Tue, Jul 21, 2009 at 5:01 AM, Balbir Singh<balbir@linux.vnet.ibm.com> wrote:
>
> lock(A)
> lock(B)
> unlock(A)
> unlock(B)
>
> Tomorrow if a unsuspecting programmer does this
>
> lock(A)
> lock(B)
> unlock(A)
>
> code block
>
> unlock(B)
>
>
> What protects code block? lock B? Is that the intention?
>

An "unsuspecting programmer" shouldn't be adding code to
multi-threaded routines without thoroughly understanding the locking.

I guess there's no harm in this patch, but as Li says, it doesn't
really change anything.

Paul

Re: [PATCH] cgroup: fix reverse unlock sequence in cgroup_get_sb

[Balbir Singh]

* menage@google.com <menage@google.com> [2009-07-21 08:34:51]:

> On Tue, Jul 21, 2009 at 5:01 AM, Balbir Singh<balbir@linux.vnet.ibm.com> wrote:
> >
> > lock(A)
> > lock(B)
> > unlock(A)
> > unlock(B)
> >
> > Tomorrow if a unsuspecting programmer does this
> >
> > lock(A)
> > lock(B)
> > unlock(A)
> >
> > code block
> >
> > unlock(B)
> >
> >
> > What protects code block? lock B? Is that the intention?
> >
> 
> An "unsuspecting programmer" shouldn't be adding code to
> multi-threaded routines without thoroughly understanding the locking.
> 

Agreed, but why leave behind places for people to do so. There is the
consistency factor as well, see below.


> I guess there's no harm in this patch, but as Li says, it doesn't
> really change anything.
>

Well all the other places do it right in the same routine. 

-- 
	Balbir

Re: [PATCH] cgroup: fix reverse unlock sequence in cgroup_get_sb

[Paul Menage]

On Tue, Jul 21, 2009 at 3:25 AM, Xiaotian Feng<dfeng@redhat.com> wrote:
> In cgroup_get_sb, the lock sequence is:
>        mutex_lock(&inode->i_mutex);
>        mutex_lock(&cgroup->mutex);
> so the last unlock sequence should be:

Make this "so for consistency the last ..." ?

Maybe make the patch title "Make unlock sequence in cgroup_get_sb
consistent" so someone looking through the change logs for fixes to
backport doesn't wrongly thing that this fixes any bug"?

>        mutex_unlock(&cgroup->mutex);
>        mutex_unlock(&inode->i_mutex);
>
> Signed-off-by: Xiaotian Feng <dfeng@redhat.com>

Acked-by: Paul Menage <menage@google.com>

Paul

Re: [PATCH] cgroup: fix reverse unlock sequence in cgroup_get_sb

[Li Zefan]

>>> Seems reasonable to me. You might also want to mention that elsewhere
>>> the sequence is unlock cgroup_mutex followed by inode->i_mutex.
>>>
>>> Acked-by: Balbir Singh balbir@linux.vnet.ibm.com
>>
>> No, the unlock order is irrelevant. It's the lock order that matters. So
>> this patch
>> fixes nothing.
>>
>> Xiaotian, you didn't run into deadlock, did you?
>>
> 
> 
> Li, Consider the following
> 
> 
> lock(A)
> lock(B)
> unlock(A) 
> unlock(B)
> 
> Tomorrow if a unsuspecting programmer does this
> 
> lock(A)
> lock(B)
> unlock(A) 
> 
> code block 
> 
> unlock(B)
> 
> 
> What protects code block? lock B? Is that the intention?
> 

I won't worry about that. If unlock order is a concern,
we should have taught lockdep to detect it.

Here's a reply from Linus on this issue:

	http://lkml.org/lkml/2008/10/8/150

Re: [PATCH] cgroup: fix reverse unlock sequence in cgroup_get_sb

[Danny Feng]

On 07/22/2009 08:53 AM, Li Zefan wrote:
>>>> Seems reasonable to me. You might also want to mention that elsewhere
>>>> the sequence is unlock cgroup_mutex followed by inode->i_mutex.
>>>>
>>>> Acked-by: Balbir Singh balbir@linux.vnet.ibm.com
>>>>          
>>> No, the unlock order is irrelevant. It's the lock order that matters. So
>>> this patch
>>> fixes nothing.
>>>
>>> Xiaotian, you didn't run into deadlock, did you?
>>>
>>>        
>> Li, Consider the following
>>
>>
>> lock(A)
>> lock(B)
>> unlock(A)
>> unlock(B)
>>
>> Tomorrow if a unsuspecting programmer does this
>>
>> lock(A)
>> lock(B)
>> unlock(A)
>>
>> code block
>>
>> unlock(B)
>>
>>
>> What protects code block? lock B? Is that the intention?
>>
>>      
>
> I won't worry about that. If unlock order is a concern,
> we should have taught lockdep to detect it.
>
> Here's a reply from Linus on this issue:
>
> 	http://lkml.org/lkml/2008/10/8/150
>    
OK, this patch is trivial. Just for consistency with previous unlock 
sequence:-)

Re: [PATCH] cgroup: fix reverse unlock sequence in cgroup_get_sb

[Danny Feng]

[-- Attachment #1: Type: text/plain, Size: 805 bytes --]

On 07/22/2009 12:03 AM, Paul Menage wrote:
> On Tue, Jul 21, 2009 at 3:25 AM, Xiaotian Feng<dfeng@redhat.com>  wrote:
>> In cgroup_get_sb, the lock sequence is:
>>         mutex_lock(&inode->i_mutex);
>>         mutex_lock(&cgroup->mutex);
>> so the last unlock sequence should be:
>
> Make this "so for consistency the last ..." ?
>
> Maybe make the patch title "Make unlock sequence in cgroup_get_sb
> consistent" so someone looking through the change logs for fixes to
> backport doesn't wrongly thing that this fixes any bug"?
>

Yep, this is a trivial patch.  Modified following your suggestion, thank 
you.

>>         mutex_unlock(&cgroup->mutex);
>>         mutex_unlock(&inode->i_mutex);
>>
>> Signed-off-by: Xiaotian Feng<dfeng@redhat.com>
>
> Acked-by: Paul Menage<menage@google.com>
>
> Paul


[-- Attachment #2: 0001-cgroup-make-unlock-sequence-in-cgroup_get_sb-consistent.patch --]
[-- Type: text/x-patch, Size: 852 bytes --]

>From ff96b0dc4a5f06a0e5b7f8dfa5df2b93e993302c Mon Sep 17 00:00:00 2001
From: Xiaotian Feng <dfeng@redhat.com>
Date: Tue, 21 Jul 2009 18:06:43 +0800
Subject: [PATCH] cgroup: make unlock sequence in cgroup_get_sb consistent

Make the last unlock sequence consistent with previous unlock sequeue.

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
---
 kernel/cgroup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 3737a68..11ef162 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1140,8 +1140,8 @@ static int cgroup_get_sb(struct file_system_type *fs_type,
 		BUG_ON(root->number_of_cgroups != 1);
 
 		cgroup_populate_dir(root_cgrp);
-		mutex_unlock(&inode->i_mutex);
 		mutex_unlock(&cgroup_mutex);
+		mutex_unlock(&inode->i_mutex);
 	}
 
 	simple_set_mnt(mnt, sb);
-- 
1.6.2.5

Re: [PATCH] cgroup: fix reverse unlock sequence in cgroup_get_sb

[Li Zefan]

Danny Feng wrote:
> On 07/22/2009 12:03 AM, Paul Menage wrote:
>> On Tue, Jul 21, 2009 at 3:25 AM, Xiaotian Feng<dfeng@redhat.com>  wrote:
>>> In cgroup_get_sb, the lock sequence is:
>>>         mutex_lock(&inode->i_mutex);
>>>         mutex_lock(&cgroup->mutex);
>>> so the last unlock sequence should be:
>>
>> Make this "so for consistency the last ..." ?
>>
>> Maybe make the patch title "Make unlock sequence in cgroup_get_sb
>> consistent" so someone looking through the change logs for fixes to
>> backport doesn't wrongly thing that this fixes any bug"?
>>
> 
> Yep, this is a trivial patch.  Modified following your suggestion, thank
> you.
> 

As far as it's not declared as a fix, I has no objection to this
patch.

Please always inline the patch in the mail body. And when resending
the patch, add the acked-by you collected in it:

Acked-by: Balbir ...
Acked-by: Paul ...
Signed-off-by: Xiaotian ...

You may resend the patch to Andrew Morton, who picks up cgroup
patches, otherwise the patch may be overlooked.

>>>         mutex_unlock(&cgroup->mutex);
>>>         mutex_unlock(&inode->i_mutex);
>>>
>>> Signed-off-by: Xiaotian Feng<dfeng@redhat.com>
>>
>> Acked-by: Paul Menage<menage@google.com>
>>
>> Paul
> 

Re: [PATCH] cgroup: fix reverse unlock sequence in cgroup_get_sb

[Danny Feng]

On 07/22/2009 10:18 AM, Li Zefan wrote:
> Danny Feng wrote:
>> On 07/22/2009 12:03 AM, Paul Menage wrote:
>>> On Tue, Jul 21, 2009 at 3:25 AM, Xiaotian Feng<dfeng@redhat.com>   wrote:
>>>> In cgroup_get_sb, the lock sequence is:
>>>>          mutex_lock(&inode->i_mutex);
>>>>          mutex_lock(&cgroup->mutex);
>>>> so the last unlock sequence should be:
>>> Make this "so for consistency the last ..." ?
>>>
>>> Maybe make the patch title "Make unlock sequence in cgroup_get_sb
>>> consistent" so someone looking through the change logs for fixes to
>>> backport doesn't wrongly thing that this fixes any bug"?
>>>
>> Yep, this is a trivial patch.  Modified following your suggestion, thank
>> you.
>>
>
> As far as it's not declared as a fix, I has no objection to this
> patch.
>
> Please always inline the patch in the mail body. And when resending
> the patch, add the acked-by you collected in it:
>
> Acked-by: Balbir ...
> Acked-by: Paul ...
> Signed-off-by: Xiaotian ...
>
> You may resend the patch to Andrew Morton, who picks up cgroup
> patches, otherwise the patch may be overlooked.
>
Got it, thank you very much.
>>>>          mutex_unlock(&cgroup->mutex);
>>>>          mutex_unlock(&inode->i_mutex);
>>>>
>>>> Signed-off-by: Xiaotian Feng<dfeng@redhat.com>
>>> Acked-by: Paul Menage<menage@google.com>
>>>
>>> Paul
>