Patchwork Hung task when calling clone() due to netfilter/slab

login
register
mail settings
Submitter Christoph Lameter
Date Jan. 17, 2012, 3:27 p.m.
Message ID <alpine.DEB.2.00.1201170927020.4800@router.home>
Download mbox | patch
Permalink /patch/136493/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Christoph Lameter - Jan. 17, 2012, 3:27 p.m.
On Tue, 17 Jan 2012, Eric Dumazet wrote:

> Buggy patch, since "goto err;" is going to up_write(&slub_lock) again.
>
> Also Christoph, you forgot to add the very much needed :
>
> Reported-by: Sasha Levin <levinsasha928@gmail.com>


Subject: slub: Do not hold slub_lock when calling sysfs_slab_add()

sysfs_slab_add() calls various sysfs functions that actually may
end up in userspace doing all sorts of things.

Release the slub_lock after adding the kmem_cache structure to the list.
At that point the address of the kmem_cache is not known so we are
guaranteed exlusive access to the following modifications to the
kmem_cache structure.

If the sysfs_slab_add fails then reacquire the slub_lock to
remove the kmem_cache structure from the list.

Reported-by: Sasha Levin <levinsasha928@gmail.com>
Signed-off-by: Christoph Lameter <cl@linux.com>

---
 mm/slub.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Jan. 17, 2012, 3:30 p.m.
Le mardi 17 janvier 2012 à 09:27 -0600, Christoph Lameter a écrit :

> Subject: slub: Do not hold slub_lock when calling sysfs_slab_add()
> 
> sysfs_slab_add() calls various sysfs functions that actually may
> end up in userspace doing all sorts of things.
> 
> Release the slub_lock after adding the kmem_cache structure to the list.
> At that point the address of the kmem_cache is not known so we are
> guaranteed exlusive access to the following modifications to the
> kmem_cache structure.
> 
> If the sysfs_slab_add fails then reacquire the slub_lock to
> remove the kmem_cache structure from the list.
> 
> Reported-by: Sasha Levin <levinsasha928@gmail.com>
> Signed-off-by: Christoph Lameter <cl@linux.com>
> 
> ---
>  mm/slub.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/mm/slub.c
> ===================================================================
> --- linux-2.6.orig/mm/slub.c	2012-01-17 03:07:11.140010438 -0600
> +++ linux-2.6/mm/slub.c	2012-01-17 03:26:06.799986908 -0600
> @@ -3929,13 +3929,14 @@ struct kmem_cache *kmem_cache_create(con
>  		if (kmem_cache_open(s, n,
>  				size, align, flags, ctor)) {
>  			list_add(&s->list, &slab_caches);
> +			up_write(&slub_lock);
>  			if (sysfs_slab_add(s)) {
> +				down_write(&slub_lock);
>  				list_del(&s->list);
>  				kfree(n);
>  				kfree(s);
>  				goto err;
>  			}
> -			up_write(&slub_lock);
>  			return s;
>  		}
>  		kfree(n);

Thanks !

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Lameter - Jan. 17, 2012, 4:04 p.m.
On Tue, 17 Jan 2012, Eric Dumazet wrote:

> Thanks !
>
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

That may not be the end of it. Slub also calls sysfs from sysfs_add_alias
while holding slub_lock.

If sysfs allows user space stuff to run then you cannot really hold any
locks. How is one supposed to sync adding pointers to sysfs structures in
subsystems? Drop all locks and then recheck the memory structures after
the sysfs function returns? Awkward.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pekka Enberg - Feb. 1, 2012, 8:07 a.m.
On Tue, Jan 17, 2012 at 5:30 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mardi 17 janvier 2012 à 09:27 -0600, Christoph Lameter a écrit :
>
>> Subject: slub: Do not hold slub_lock when calling sysfs_slab_add()
>>
>> sysfs_slab_add() calls various sysfs functions that actually may
>> end up in userspace doing all sorts of things.
>>
>> Release the slub_lock after adding the kmem_cache structure to the list.
>> At that point the address of the kmem_cache is not known so we are
>> guaranteed exlusive access to the following modifications to the
>> kmem_cache structure.
>>
>> If the sysfs_slab_add fails then reacquire the slub_lock to
>> remove the kmem_cache structure from the list.
>>
>> Reported-by: Sasha Levin <levinsasha928@gmail.com>
>> Signed-off-by: Christoph Lameter <cl@linux.com>
>>
>> ---
>>  mm/slub.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> Index: linux-2.6/mm/slub.c
>> ===================================================================
>> --- linux-2.6.orig/mm/slub.c  2012-01-17 03:07:11.140010438 -0600
>> +++ linux-2.6/mm/slub.c       2012-01-17 03:26:06.799986908 -0600
>> @@ -3929,13 +3929,14 @@ struct kmem_cache *kmem_cache_create(con
>>               if (kmem_cache_open(s, n,
>>                               size, align, flags, ctor)) {
>>                       list_add(&s->list, &slab_caches);
>> +                     up_write(&slub_lock);
>>                       if (sysfs_slab_add(s)) {
>> +                             down_write(&slub_lock);
>>                               list_del(&s->list);
>>                               kfree(n);
>>                               kfree(s);
>>                               goto err;
>>                       }
>> -                     up_write(&slub_lock);
>>                       return s;
>>               }
>>               kfree(n);
>
> Thanks !
>
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

I'm planning to queue this for v3.4 and tagging it for -stable for
v3.3. Do we need this for older versions as well?

                        Pekka
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c	2012-01-17 03:07:11.140010438 -0600
+++ linux-2.6/mm/slub.c	2012-01-17 03:26:06.799986908 -0600
@@ -3929,13 +3929,14 @@  struct kmem_cache *kmem_cache_create(con
 		if (kmem_cache_open(s, n,
 				size, align, flags, ctor)) {
 			list_add(&s->list, &slab_caches);
+			up_write(&slub_lock);
 			if (sysfs_slab_add(s)) {
+				down_write(&slub_lock);
 				list_del(&s->list);
 				kfree(n);
 				kfree(s);
 				goto err;
 			}
-			up_write(&slub_lock);
 			return s;
 		}
 		kfree(n);