2010-12-23 12:52:46

by Hillf Danton

[permalink] [raw]
Subject: [PATCH] fix freeing user_struct in user cache

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 <[email protected]>
---

--- 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);


2010-12-24 03:55:48

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] fix freeing user_struct in user cache

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 <[email protected]>
> ---
>
> --- 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

2010-12-24 14:24:06

by Hillf Danton

[permalink] [raw]
Subject: Re: [PATCH] fix freeing user_struct in user cache

On Fri, Dec 24, 2010 at 11:55 AM, Greg KH <[email protected]> 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 <[email protected]>
>> ---
>>
>> --- 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, <[email protected]> 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
>

2010-12-24 17:19:24

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] fix freeing user_struct in user cache

On Fri, Dec 24, 2010 at 10:24:02PM +0800, Hillf Danton wrote:
> On Fri, Dec 24, 2010 at 11:55 AM, Greg KH <[email protected]> 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 <[email protected]>
> >> ---
> >>
> >> --- 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, <[email protected]> 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

2010-12-24 23:55:48

by Serge E. Hallyn

[permalink] [raw]
Subject: Re: [PATCH] fix freeing user_struct in user cache

Quoting Hillf Danton ([email protected]):
> On Fri, Dec 24, 2010 at 11:55 AM, Greg KH <[email protected]> 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 <[email protected]>
> >> ---
> >>
> >> --- 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 <[email protected]>

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

thanks,
-serge

2010-12-25 13:56:47

by Hillf Danton

[permalink] [raw]
Subject: Re: [PATCH] fix freeing user_struct in user cache

On Sat, Dec 25, 2010 at 1:14 AM, Greg KH <[email protected]> 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
>