[PATCH] fix freeing user_struct in user cache

[PATCH] fix freeing user_struct in user cache

[Hillf Danton]

When racing on adding into user cache, the new allocated from mm slab
is freed without putting user namespace.

Since the user namespace is already operated by getting, putting has
to be issued.

btw, it could be freed out of lock?

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/kernel/user.c	2010-11-01 19:54:12.000000000 +0800
+++ b/kernel/user.c	2010-12-23 20:42:00.000000000 +0800
@@ -158,6 +158,7 @@ struct user_struct *alloc_uid(struct use
 		spin_lock_irq(&uidhash_lock);
 		up = uid_hash_find(uid, hashent);
 		if (up) {
+			put_user_ns(ns);
 			key_put(new->uid_keyring);
 			key_put(new->session_keyring);
 			kmem_cache_free(uid_cachep, new);

Re: [PATCH] fix freeing user_struct in user cache

[Greg KH]

On Thu, Dec 23, 2010 at 08:52:34PM +0800, Hillf Danton wrote:
> When racing on adding into user cache, the new allocated from mm slab
> is freed without putting user namespace.
> 
> Since the user namespace is already operated by getting, putting has
> to be issued.
> 
> btw, it could be freed out of lock?
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
> 
> --- a/kernel/user.c	2010-11-01 19:54:12.000000000 +0800
> +++ b/kernel/user.c	2010-12-23 20:42:00.000000000 +0800
> @@ -158,6 +158,7 @@ struct user_struct *alloc_uid(struct use
>  		spin_lock_irq(&uidhash_lock);
>  		up = uid_hash_find(uid, hashent);
>  		if (up) {
> +			put_user_ns(ns);
>  			key_put(new->uid_keyring);
>  			key_put(new->session_keyring);
>  			kmem_cache_free(uid_cachep, new);

Hm, are you sure about this?  Also, why send this to me, did I last
touch this?

confused,

greg k-h

Re: [PATCH] fix freeing user_struct in user cache

[Hillf Danton]

On Fri, Dec 24, 2010 at 11:55 AM, Greg KH <gregkh@suse.de> wrote:
> On Thu, Dec 23, 2010 at 08:52:34PM +0800, Hillf Danton wrote:
>> When racing on adding into user cache, the new allocated from mm slab
>> is freed without putting user namespace.
>>
>> Since the user namespace is already operated by getting, putting has
>> to be issued.
>>
>> btw, it could be freed out of lock?
>>
>> Signed-off-by: Hillf Danton <dhillf@gmail.com>
>> ---
>>
>> --- a/kernel/user.c   2010-11-01 19:54:12.000000000 +0800
>> +++ b/kernel/user.c   2010-12-23 20:42:00.000000000 +0800
>> @@ -158,6 +158,7 @@ struct user_struct *alloc_uid(struct use
>>               spin_lock_irq(&uidhash_lock);
>>               up = uid_hash_find(uid, hashent);
>>               if (up) {
>> +                     put_user_ns(ns);
>>                       key_put(new->uid_keyring);
>>                       key_put(new->session_keyring);
>>                       kmem_cache_free(uid_cachep, new);
>
> Hm, are you sure about this?  Also, why send this to me, did I last
> touch this?
>

sure with no doubt.

I do not know if you touched that last, but I received the following message,

On Tue, Dec 21, 2010 at 3:42 AM,  <gregkh@suse.de> wrote:
>
> This is a note to let you know that I've just added the patch titled
>
>    bonding: Fix slave selection bug.
>
> to the 2.6.36-stable tree which can be found at:
>    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

so you were Cced since you charge patch delivered.

Cheers

Hillf

> confused,
>
> greg k-h
>

Re: [PATCH] fix freeing user_struct in user cache

[Greg KH]

On Fri, Dec 24, 2010 at 10:24:02PM +0800, Hillf Danton wrote:
> On Fri, Dec 24, 2010 at 11:55 AM, Greg KH <gregkh@suse.de> wrote:
> > On Thu, Dec 23, 2010 at 08:52:34PM +0800, Hillf Danton wrote:
> >> When racing on adding into user cache, the new allocated from mm slab
> >> is freed without putting user namespace.
> >>
> >> Since the user namespace is already operated by getting, putting has
> >> to be issued.
> >>
> >> btw, it could be freed out of lock?
> >>
> >> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> >> ---
> >>
> >> --- a/kernel/user.c   2010-11-01 19:54:12.000000000 +0800
> >> +++ b/kernel/user.c   2010-12-23 20:42:00.000000000 +0800
> >> @@ -158,6 +158,7 @@ struct user_struct *alloc_uid(struct use
> >>               spin_lock_irq(&uidhash_lock);
> >>               up = uid_hash_find(uid, hashent);
> >>               if (up) {
> >> +                     put_user_ns(ns);
> >>                       key_put(new->uid_keyring);
> >>                       key_put(new->session_keyring);
> >>                       kmem_cache_free(uid_cachep, new);
> >
> > Hm, are you sure about this?  Also, why send this to me, did I last
> > touch this?
> >
> 
> sure with no doubt.
> 
> I do not know if you touched that last, but I received the following message,
> 
> On Tue, Dec 21, 2010 at 3:42 AM,  <gregkh@suse.de> wrote:
> >
> > This is a note to let you know that I've just added the patch titled
> >
> >    bonding: Fix slave selection bug.
> >
> > to the 2.6.36-stable tree which can be found at:
> >    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> 
> so you were Cced since you charge patch delivered.

That was a stable patch, I send all of those out :)

Use scripts/get_maintainer.pl to determine the best person to send this
patch to (hint, it's not me.)

thanks,

greg k-h

Re: [PATCH] fix freeing user_struct in user cache

[Hillf Danton]

On Sat, Dec 25, 2010 at 1:14 AM, Greg KH <gregkh@suse.de> wrote:
> That was a stable patch, I send all of those out :)
>
> Use scripts/get_maintainer.pl to determine the best person to send this
> patch to (hint, it's not me.)

thanks, Greg, for sending it out, and merry Christmas.

Hillf

>
> thanks,
>
> greg k-h
>

[PATCH] fix freeing user_struct in user cache

[Hillf Danton]

When racing on adding into user cache, the new allocated from mm slab
is freed without putting user namespace.

Since the user namespace is already operated by getting, putting has
to be issued.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/kernel/user.c	2010-11-01 19:54:12.000000000 +0800
+++ b/kernel/user.c	2010-12-23 20:42:00.000000000 +0800
@@ -158,6 +158,7 @@ struct user_struct *alloc_uid(struct use
 		spin_lock_irq(&uidhash_lock);
 		up = uid_hash_find(uid, hashent);
 		if (up) {
+			put_user_ns(ns);
 			key_put(new->uid_keyring);
 			key_put(new->session_keyring);
 			kmem_cache_free(uid_cachep, new);

Re: [PATCH] fix freeing user_struct in user cache

[Serge E. Hallyn]

Quoting Hillf Danton (dhillf@gmail.com):
> When racing on adding into user cache, the new allocated from mm slab
> is freed without putting user namespace.
> 
> Since the user namespace is already operated by getting, putting has
> to be issued.
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>

which was previously

> Acked-by: Serge Hallyn <serge@hallyn.com>

thanks again, Hillf.

> ---
> 
> --- a/kernel/user.c	2010-11-01 19:54:12.000000000 +0800
> +++ b/kernel/user.c	2010-12-23 20:42:00.000000000 +0800
> @@ -158,6 +158,7 @@ struct user_struct *alloc_uid(struct use
>  		spin_lock_irq(&uidhash_lock);
>  		up = uid_hash_find(uid, hashent);
>  		if (up) {
> +			put_user_ns(ns);
>  			key_put(new->uid_keyring);
>  			key_put(new->session_keyring);
>  			kmem_cache_free(uid_cachep, new);

Re: [PATCH] fix freeing user_struct in user cache

[Serge E. Hallyn]

Quoting Hillf Danton (dhillf@gmail.com):
> On Fri, Dec 24, 2010 at 11:55 AM, Greg KH <gregkh@suse.de> wrote:
> > On Thu, Dec 23, 2010 at 08:52:34PM +0800, Hillf Danton wrote:
> >> When racing on adding into user cache, the new allocated from mm slab
> >> is freed without putting user namespace.
> >>
> >> Since the user namespace is already operated by getting, putting has
> >> to be issued.
> >>
> >> btw, it could be freed out of lock?
> >>
> >> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> >> ---
> >>
> >> --- a/kernel/user.c   2010-11-01 19:54:12.000000000 +0800
> >> +++ b/kernel/user.c   2010-12-23 20:42:00.000000000 +0800
> >> @@ -158,6 +158,7 @@ struct user_struct *alloc_uid(struct use
> >>               spin_lock_irq(&uidhash_lock);
> >>               up = uid_hash_find(uid, hashent);
> >>               if (up) {
> >> +                     put_user_ns(ns);
> >>                       key_put(new->uid_keyring);
> >>                       key_put(new->session_keyring);
> >>                       kmem_cache_free(uid_cachep, new);
> >
> > Hm, are you sure about this?  Also, why send this to me, did I last
> > touch this?
> >
> 
> sure with no doubt.

Good catch, thanks.

Acked-by: Serge Hallyn <serge@hallyn.com>

> I do not know if you touched that last, but I received the following message,

thanks,
-serge