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

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

[Sasha Levin]

Hi All,

I've stumbled on the following oops when testing the trinity fuzzer using KVM tool, running on the latest -next kernel:

[ 3960.845373] INFO: task trinity:31661 blocked for more than 120 seconds.
[ 3960.846757] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 3960.850245] trinity         D ffff880008e89610  4952 31661   2197 0x00000004
[ 3960.855032]  ffff880008e39830 0000000000000086 ffff880008e39fd8 ffff880008e89610
[ 3960.859289]  00000000001d3980 ffff880008e39fd8 ffff880008e38000 00000000001d3980
[ 3960.862723]  00000000001d3980 00000000001d3980 ffff880008e39fd8 00000000001d3980
[ 3960.866083] Call Trace:
[ 3960.867332]  [<ffffffff825830ea>] schedule+0x3a/0x50
[ 3960.869477]  [<ffffffff82581285>] schedule_timeout+0x245/0x2c0
[ 3960.871996]  [<ffffffff811023ee>] ? mark_held_locks+0x6e/0x130
[ 3960.875599]  [<ffffffff810ffb62>] ? lock_release_holdtime+0xb2/0x160
[ 3960.879156]  [<ffffffff8258450b>] ? _raw_spin_unlock_irq+0x2b/0x70
[ 3960.881682]  [<ffffffff810de041>] ? get_parent_ip+0x11/0x50
[ 3960.883907]  [<ffffffff82583880>] wait_for_common+0x120/0x170
[ 3960.886190]  [<ffffffff810dc930>] ? try_to_wake_up+0x320/0x320
[ 3960.888570]  [<ffffffff82583978>] wait_for_completion+0x18/0x20
[ 3960.890968]  [<ffffffff810c5595>] call_usermodehelper_exec+0x1b5/0x1d0
[ 3960.893583]  [<ffffffff825837a4>] ? wait_for_common+0x44/0x170
[ 3960.895907]  [<ffffffff8182aa58>] kobject_uevent_env+0x4e8/0x580
[ 3960.898310]  [<ffffffff8182aafb>] kobject_uevent+0xb/0x10
[ 3960.900577]  [<ffffffff811b59a1>] sysfs_slab_add+0xb1/0x210
[ 3960.902830]  [<ffffffff811b74db>] kmem_cache_create+0xcb/0x2f0
[ 3960.905704]  [<ffffffff8216c72d>] nf_conntrack_init+0x9d/0x380
[ 3960.908797]  [<ffffffff8216d11f>] nf_conntrack_net_init+0xf/0x1a0
[ 3960.911065]  [<ffffffff821195a2>] ops_init+0x42/0x180
[ 3960.913192]  [<ffffffff8211974b>] setup_net+0x6b/0x100
[ 3960.915308]  [<ffffffff82119ba6>] copy_net_ns+0x86/0x110
[ 3960.917534]  [<ffffffff810d4049>] create_new_namespaces+0xd9/0x190
[ 3960.920048]  [<ffffffff810d4224>] copy_namespaces+0x84/0xc0
[ 3960.922326]  [<ffffffff810aa471>] copy_process+0xa21/0x1480
[ 3960.924328]  [<ffffffff810d3d9e>] ? up_read+0x1e/0x40
[ 3960.925327]  [<ffffffff810aaf83>] do_fork+0x73/0x340
[ 3960.926273]  [<ffffffff811026dd>] ? trace_hardirqs_on+0xd/0x10
[ 3960.927372]  [<ffffffff82581ea9>] ? mutex_unlock+0x9/0x10
[ 3960.928422]  [<ffffffff812c4d13>] ? ext4_sync_file+0xa3/0x340
[ 3960.929487]  [<ffffffff8258525d>] ? retint_swapgs+0x13/0x1b
[ 3960.930520]  [<ffffffff810554b3>] sys_clone+0x23/0x30
[ 3960.931475]  [<ffffffff82585ec3>] stub_clone+0x13/0x20
[ 3960.932490]  [<ffffffff82585b39>] ? system_call_fastpath+0x16/0x1b
[ 3960.933655] 2 locks held by trinity/31661:
[ 3960.934413]  #0:  (net_mutex){+.+.+.}, at: [<ffffffff82119b9e>] copy_net_ns+0x7e/0x110
[ 3960.936057]  #1:  (slub_lock){+.+.+.}, at: [<ffffffff811b7451>] kmem_cache_create+0x41/0x2f0
[ 3960.937810] Kernel panic - not syncing: hung_task: blocked tasks

I had two trinity processes running, and both were stuck at clone() syscalls:

clone(clone_flags=0xcc2820ff, newsp=0xf3f270[page_0xff], parent_tid=0xf3f270[page_0xff], child_tid=0x400000, regs=0xf41290[page_allocs])
clone(clone_flags=0x45706000, newsp=0xf3f270[page_0xff], parent_tid=0x7f9bf26f0000, child_tid=0xf3f270[page_0xff], regs=0xf41290[page_allocs])

This is the second time I got this oops, and both times it originated with clone calling up to the netfilter connection tracker code, so I don't think that it's coincidental that thats the origin.

-- 

Sasha.

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

[Eric Dumazet]

Le samedi 14 janvier 2012 à 18:30 +0200, Sasha Levin a écrit :
> Hi All,
> 
> I've stumbled on the following oops when testing the trinity fuzzer using KVM tool, running on the latest -next kernel:
> 
> [ 3960.845373] INFO: task trinity:31661 blocked for more than 120 seconds.
> [ 3960.846757] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> [ 3960.850245] trinity         D ffff880008e89610  4952 31661   2197 0x00000004
> [ 3960.855032]  ffff880008e39830 0000000000000086 ffff880008e39fd8 ffff880008e89610
> [ 3960.859289]  00000000001d3980 ffff880008e39fd8 ffff880008e38000 00000000001d3980
> [ 3960.862723]  00000000001d3980 00000000001d3980 ffff880008e39fd8 00000000001d3980
> [ 3960.866083] Call Trace:
> [ 3960.867332]  [<ffffffff825830ea>] schedule+0x3a/0x50
> [ 3960.869477]  [<ffffffff82581285>] schedule_timeout+0x245/0x2c0
> [ 3960.871996]  [<ffffffff811023ee>] ? mark_held_locks+0x6e/0x130
> [ 3960.875599]  [<ffffffff810ffb62>] ? lock_release_holdtime+0xb2/0x160
> [ 3960.879156]  [<ffffffff8258450b>] ? _raw_spin_unlock_irq+0x2b/0x70
> [ 3960.881682]  [<ffffffff810de041>] ? get_parent_ip+0x11/0x50
> [ 3960.883907]  [<ffffffff82583880>] wait_for_common+0x120/0x170
> [ 3960.886190]  [<ffffffff810dc930>] ? try_to_wake_up+0x320/0x320
> [ 3960.888570]  [<ffffffff82583978>] wait_for_completion+0x18/0x20
> [ 3960.890968]  [<ffffffff810c5595>] call_usermodehelper_exec+0x1b5/0x1d0
> [ 3960.893583]  [<ffffffff825837a4>] ? wait_for_common+0x44/0x170
> [ 3960.895907]  [<ffffffff8182aa58>] kobject_uevent_env+0x4e8/0x580
> [ 3960.898310]  [<ffffffff8182aafb>] kobject_uevent+0xb/0x10
> [ 3960.900577]  [<ffffffff811b59a1>] sysfs_slab_add+0xb1/0x210
> [ 3960.902830]  [<ffffffff811b74db>] kmem_cache_create+0xcb/0x2f0
> [ 3960.905704]  [<ffffffff8216c72d>] nf_conntrack_init+0x9d/0x380
> [ 3960.908797]  [<ffffffff8216d11f>] nf_conntrack_net_init+0xf/0x1a0
> [ 3960.911065]  [<ffffffff821195a2>] ops_init+0x42/0x180
> [ 3960.913192]  [<ffffffff8211974b>] setup_net+0x6b/0x100
> [ 3960.915308]  [<ffffffff82119ba6>] copy_net_ns+0x86/0x110
> [ 3960.917534]  [<ffffffff810d4049>] create_new_namespaces+0xd9/0x190
> [ 3960.920048]  [<ffffffff810d4224>] copy_namespaces+0x84/0xc0
> [ 3960.922326]  [<ffffffff810aa471>] copy_process+0xa21/0x1480
> [ 3960.924328]  [<ffffffff810d3d9e>] ? up_read+0x1e/0x40
> [ 3960.925327]  [<ffffffff810aaf83>] do_fork+0x73/0x340
> [ 3960.926273]  [<ffffffff811026dd>] ? trace_hardirqs_on+0xd/0x10
> [ 3960.927372]  [<ffffffff82581ea9>] ? mutex_unlock+0x9/0x10
> [ 3960.928422]  [<ffffffff812c4d13>] ? ext4_sync_file+0xa3/0x340
> [ 3960.929487]  [<ffffffff8258525d>] ? retint_swapgs+0x13/0x1b
> [ 3960.930520]  [<ffffffff810554b3>] sys_clone+0x23/0x30
> [ 3960.931475]  [<ffffffff82585ec3>] stub_clone+0x13/0x20
> [ 3960.932490]  [<ffffffff82585b39>] ? system_call_fastpath+0x16/0x1b
> [ 3960.933655] 2 locks held by trinity/31661:
> [ 3960.934413]  #0:  (net_mutex){+.+.+.}, at: [<ffffffff82119b9e>] copy_net_ns+0x7e/0x110
> [ 3960.936057]  #1:  (slub_lock){+.+.+.}, at: [<ffffffff811b7451>] kmem_cache_create+0x41/0x2f0
> [ 3960.937810] Kernel panic - not syncing: hung_task: blocked tasks
> 
> I had two trinity processes running, and both were stuck at clone() syscalls:
> 
> clone(clone_flags=0xcc2820ff, newsp=0xf3f270[page_0xff], parent_tid=0xf3f270[page_0xff], child_tid=0x400000, regs=0xf41290[page_allocs])
> clone(clone_flags=0x45706000, newsp=0xf3f270[page_0xff], parent_tid=0x7f9bf26f0000, child_tid=0xf3f270[page_0xff], regs=0xf41290[page_allocs])
> 
> This is the second time I got this oops, and both times it originated with clone calling up to the netfilter connection tracker code, so I don't think that it's coincidental that thats the origin.
> 

Apparently SLUB calls sysfs_slab_add() from kmem_cache_create() while
still holding slub_lock.

So if the task launched needs to "cat /proc/slabinfo" or anything
needing slub_lock, its a deadlock.

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

[Eric Dumazet]

Le samedi 14 janvier 2012 à 18:10 +0100, Eric Dumazet a écrit :

> Apparently SLUB calls sysfs_slab_add() from kmem_cache_create() while
> still holding slub_lock.
> 
> So if the task launched needs to "cat /proc/slabinfo" or anything
> needing slub_lock, its a deadlock.
> 
> 

Bug added in commit 2bce64858442149784f6c88
(slub: Allow removal of slab caches during boot)

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

[Sasha Levin]

On Sat, 2012-01-14 at 18:10 +0100, Eric Dumazet wrote:
> Apparently SLUB calls sysfs_slab_add() from kmem_cache_create() while
> still holding slub_lock.
> 
> So if the task launched needs to "cat /proc/slabinfo" or anything
> needing slub_lock, its a deadlock.

I've made the following patch to test it, It doesn't look like it's the correct solution, but it verifies that the problem is there (it works well with the patch).

---------------

>From cc4874b491b8e5d9d1ea5bf2032413efdbddced8 Mon Sep 17 00:00:00 2001
From: Sasha Levin <levinsasha928@gmail.com>
Date: Sun, 15 Jan 2012 14:55:03 +0200
Subject: [PATCH] slab: Fix hang when creating sysfs entries

This patch fixes the hang which happens when we create a sysfs entry and call
back to userspace. If the usermode helper tries to do anything which involves
slub_lock we will hang since slub_lock is already held.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 mm/slub.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 4907563..6948327 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5283,7 +5283,9 @@ static int sysfs_slab_add(struct kmem_cache *s)
 		kobject_put(&s->kobj);
 		return err;
 	}
+	up_write(&slub_lock);
 	kobject_uevent(&s->kobj, KOBJ_ADD);
+	down_write(&slub_lock);
 	if (!unmergeable) {
 		/* Setup first alias */
 		sysfs_slab_alias(s, s->name);
-- 
1.7.8.3


-- 

Sasha.

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

[Eric Dumazet]

Le dimanche 15 janvier 2012 à 14:59 +0200, Sasha Levin a écrit :
> On Sat, 2012-01-14 at 18:10 +0100, Eric Dumazet wrote:
> > Apparently SLUB calls sysfs_slab_add() from kmem_cache_create() while
> > still holding slub_lock.
> > 
> > So if the task launched needs to "cat /proc/slabinfo" or anything
> > needing slub_lock, its a deadlock.
> 
> I've made the following patch to test it, It doesn't look like it's
> the correct solution, but it verifies that the problem is there (it
> works well with the patch).
> 
> ---------------
> 
> From cc4874b491b8e5d9d1ea5bf2032413efdbddced8 Mon Sep 17 00:00:00 2001
> From: Sasha Levin <levinsasha928@gmail.com>
> Date: Sun, 15 Jan 2012 14:55:03 +0200
> Subject: [PATCH] slab: Fix hang when creating sysfs entries
> 
> This patch fixes the hang which happens when we create a sysfs entry and call
> back to userspace. If the usermode helper tries to do anything which involves
> slub_lock we will hang since slub_lock is already held.
> 
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
>  mm/slub.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 4907563..6948327 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5283,7 +5283,9 @@ static int sysfs_slab_add(struct kmem_cache *s)
>  		kobject_put(&s->kobj);
>  		return err;
>  	}
> +	up_write(&slub_lock);
>  	kobject_uevent(&s->kobj, KOBJ_ADD);
> +	down_write(&slub_lock);
>  	if (!unmergeable) {
>  		/* Setup first alias */
>  		sysfs_slab_alias(s, s->name);
> -- 
> 1.7.8.3
> 
> 

Oh well, that cannot be right. Dont send official patches if you already
know "it's not the correct solution", we already know where is the
problem.

It's _never_ right to release a lock for a short time without any
additional checks. [ If it was right, the lock would be not needed ]

As soon as the slub_lock is released, another thread can come and find
the new kmem_cache.

For example, it can destroy it, and your thread is going to access s
while it was already freed.

	sysfs_slab_alias(s, s->name); // crash

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

[Christoph Lameter]

On Sun, 15 Jan 2012, Eric Dumazet wrote:

> As soon as the slub_lock is released, another thread can come and find
> the new kmem_cache.

Slabs are not looked up by name. A pointer to kmem_cache is passed to slab
functions and that pointer is returned from kmem_cache_create(). The risk
is someone traversing the kmem_cach list which is only done from within slub.


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.

Signed-off-by: Christoph Lameter <cl@linux.com>

---
 mm/slub.c |    4 +++-
 1 file changed, 3 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:07:19.532010264 -0600
@@ -3929,13 +3929,15 @@ 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);
+				up_write(&slub_lock);
 				kfree(n);
 				kfree(s);
 				goto err;
 			}
-			up_write(&slub_lock);
 			return s;
 		}
 		kfree(n);

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

[Eric Dumazet]

Le mardi 17 janvier 2012 à 09:12 -0600, Christoph Lameter a écrit :
> On Sun, 15 Jan 2012, Eric Dumazet wrote:
> 
> > As soon as the slub_lock is released, another thread can come and find
> > the new kmem_cache.
> 
> Slabs are not looked up by name. A pointer to kmem_cache is passed to slab
> functions and that pointer is returned from kmem_cache_create(). The risk
> is someone traversing the kmem_cach list which is only done from within slub.
> 
> 
> 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.
> 
> Signed-off-by: Christoph Lameter <cl@linux.com>
> 
> ---
>  mm/slub.c |    4 +++-
>  1 file changed, 3 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:07:19.532010264 -0600
> @@ -3929,13 +3929,15 @@ 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);
> +				up_write(&slub_lock);
>  				kfree(n);
>  				kfree(s);
>  				goto err;
>  			}
> -			up_write(&slub_lock);
>  			return s;
>  		}
>  		kfree(n);



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>

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

[Christoph Lameter]

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

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

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

[Eric Dumazet]

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>

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

[Christoph Lameter]

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.

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

[Christoph Lameter]

Another version that drops the slub lock for both invocations of sysfs
functions from kmem_cache_create. The invocation from slab_sysfs_init
is not a problem since user space is not active at that point.


Subject: slub: Do not take the slub lock while calling into sysfs

This patch avoids holding the slub_lock during kmem_cache_create()
when calling sysfs. It is possible because kmem_cache_create()
allocates the kmem_cache object and therefore is the only one context
that can access the newly created object. It is therefore possible
to drop the slub_lock early. We defer the adding of the new kmem_cache
to the end of processing because the new kmem_cache structure would
be reachable otherwise via scans over slabs. This allows sysfs_slab_add()
to run without holding any locks.

The case is different if we are creating an alias instead of a new
kmem_cache structure. In that case we can also drop the slub lock
early because we have taken a refcount on the kmem_cache structure.
It therefore cannot vanish from under us.
But if the sysfs_slab_alias() call fails we can no longer simply
decrement the refcount since the other references may have gone
away in the meantime. Call kmem_cache_destroy() to cause the
refcount to be decremented and the kmem_cache structure to be
freed if all references are gone.

Signed-off-by: Christoph Lameter <cl@linux.com>


---
 mm/slub.c |   25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c	2012-01-17 09:53:26.599505365 -0600
+++ linux-2.6/mm/slub.c	2012-01-17 09:59:57.131497273 -0600
@@ -3912,13 +3912,14 @@ struct kmem_cache *kmem_cache_create(con
 		s->objsize = max(s->objsize, (int)size);
 		s->inuse = max_t(int, s->inuse, ALIGN(size, sizeof(void *)));

+		up_write(&slub_lock);
 		if (sysfs_slab_alias(s, name)) {
-			s->refcount--;
+			kmem_cache_destroy(s);
 			goto err;
 		}
-		up_write(&slub_lock);
 		return s;
 	}
+	up_write(&slub_lock);

 	n = kstrdup(name, GFP_KERNEL);
 	if (!n)
@@ -3928,27 +3929,23 @@ struct kmem_cache *kmem_cache_create(con
 	if (s) {
 		if (kmem_cache_open(s, n,
 				size, align, flags, ctor)) {
-			list_add(&s->list, &slab_caches);
-			if (sysfs_slab_add(s)) {
-				list_del(&s->list);
-				kfree(n);
-				kfree(s);
-				goto err;
+
+			if (sysfs_slab_add(s) == 0) {
+				down_write(&slub_lock);
+				list_add(&s->list, &slab_caches);
+				up_write(&slub_lock);
+				return s;
 			}
-			up_write(&slub_lock);
-			return s;
 		}
 		kfree(n);
 		kfree(s);
 	}
 err:
-	up_write(&slub_lock);

 	if (flags & SLAB_PANIC)
 		panic("Cannot create slabcache %s\n", name);
-	else
-		s = NULL;
-	return s;
+
+	return NULL;
 }
 EXPORT_SYMBOL(kmem_cache_create);

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

[Eric W. Biederman]

Christoph Lameter <cl@linux.com> writes:

> Another version that drops the slub lock for both invocations of sysfs
> functions from kmem_cache_create. The invocation from slab_sysfs_init
> is not a problem since user space is not active at that point.
>
>
> Subject: slub: Do not take the slub lock while calling into sysfs
>
> This patch avoids holding the slub_lock during kmem_cache_create()
> when calling sysfs. It is possible because kmem_cache_create()
> allocates the kmem_cache object and therefore is the only one context
> that can access the newly created object. It is therefore possible
> to drop the slub_lock early. We defer the adding of the new kmem_cache
> to the end of processing because the new kmem_cache structure would
> be reachable otherwise via scans over slabs. This allows sysfs_slab_add()
> to run without holding any locks.
>
> The case is different if we are creating an alias instead of a new
> kmem_cache structure. In that case we can also drop the slub lock
> early because we have taken a refcount on the kmem_cache structure.
> It therefore cannot vanish from under us.
> But if the sysfs_slab_alias() call fails we can no longer simply
> decrement the refcount since the other references may have gone
> away in the meantime. Call kmem_cache_destroy() to cause the
> refcount to be decremented and the kmem_cache structure to be
> freed if all references are gone.
>
> Signed-off-by: Christoph Lameter <cl@linux.com>

I am dense.  Is the deadlock here that you are fixing slub calling sysfs
with the slub_lock held but sysfs then calling kmem_cache_zalloc?

I don't see what sysfs is doing in the creation path that would cause
a deadlock except for using slab.

> ---
>  mm/slub.c |   25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
>
> Index: linux-2.6/mm/slub.c
> ===================================================================
> --- linux-2.6.orig/mm/slub.c	2012-01-17 09:53:26.599505365 -0600
> +++ linux-2.6/mm/slub.c	2012-01-17 09:59:57.131497273 -0600
> @@ -3912,13 +3912,14 @@ struct kmem_cache *kmem_cache_create(con
>  		s->objsize = max(s->objsize, (int)size);
>  		s->inuse = max_t(int, s->inuse, ALIGN(size, sizeof(void *)));
>
> +		up_write(&slub_lock);
>  		if (sysfs_slab_alias(s, name)) {
> -			s->refcount--;
> +			kmem_cache_destroy(s);
>  			goto err;
>  		}
> -		up_write(&slub_lock);
>  		return s;
>  	}
> +	up_write(&slub_lock);
>
>  	n = kstrdup(name, GFP_KERNEL);
>  	if (!n)
> @@ -3928,27 +3929,23 @@ struct kmem_cache *kmem_cache_create(con
>  	if (s) {
>  		if (kmem_cache_open(s, n,
>  				size, align, flags, ctor)) {
> -			list_add(&s->list, &slab_caches);
> -			if (sysfs_slab_add(s)) {
> -				list_del(&s->list);
> -				kfree(n);
> -				kfree(s);
> -				goto err;
> +
> +			if (sysfs_slab_add(s) == 0) {
> +				down_write(&slub_lock);
> +				list_add(&s->list, &slab_caches);
> +				up_write(&slub_lock);
> +				return s;
>  			}
> -			up_write(&slub_lock);
> -			return s;
>  		}
>  		kfree(n);
>  		kfree(s);
>  	}
>  err:
> -	up_write(&slub_lock);
>
>  	if (flags & SLAB_PANIC)
>  		panic("Cannot create slabcache %s\n", name);
> -	else
> -		s = NULL;
> -	return s;
> +
> +	return NULL;
>  }
>  EXPORT_SYMBOL(kmem_cache_create);
>
> --
> 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

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

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> Christoph Lameter <cl@linux.com> writes:
>
>> Another version that drops the slub lock for both invocations of sysfs
>> functions from kmem_cache_create. The invocation from slab_sysfs_init
>> is not a problem since user space is not active at that point.
>>
>>
>> Subject: slub: Do not take the slub lock while calling into sysfs
>>
>> This patch avoids holding the slub_lock during kmem_cache_create()
>> when calling sysfs. It is possible because kmem_cache_create()
>> allocates the kmem_cache object and therefore is the only one context
>> that can access the newly created object. It is therefore possible
>> to drop the slub_lock early. We defer the adding of the new kmem_cache
>> to the end of processing because the new kmem_cache structure would
>> be reachable otherwise via scans over slabs. This allows sysfs_slab_add()
>> to run without holding any locks.
>>
>> The case is different if we are creating an alias instead of a new
>> kmem_cache structure. In that case we can also drop the slub lock
>> early because we have taken a refcount on the kmem_cache structure.
>> It therefore cannot vanish from under us.
>> But if the sysfs_slab_alias() call fails we can no longer simply
>> decrement the refcount since the other references may have gone
>> away in the meantime. Call kmem_cache_destroy() to cause the
>> refcount to be decremented and the kmem_cache structure to be
>> freed if all references are gone.
>>
>> Signed-off-by: Christoph Lameter <cl@linux.com>
>
> I am dense.  Is the deadlock here that you are fixing slub calling sysfs
> with the slub_lock held but sysfs then calling kmem_cache_zalloc?
>
> I don't see what sysfs is doing in the creation path that would cause
> a deadlock except for using slab.

Oh.  I see.  The problem is calling kobject_uevent (which happens to
live in slabs sysfs_slab_add) with a lock held.  And kobject_uevent
makes a blocking call to userspace.

No locks held seems to be a good policy on that one.

Eric

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

[Christoph Lameter]

On Thu, 19 Jan 2012, Eric W. Biederman wrote:

> Oh.  I see.  The problem is calling kobject_uevent (which happens to
> live in slabs sysfs_slab_add) with a lock held.  And kobject_uevent
> makes a blocking call to userspace.
>
> No locks held seems to be a good policy on that one.

Well we can just remove that call to kobject_uevent instead then. Does it
do anything useful? Cannot remember why we put that in there.

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

[Eric W. Biederman]

Christoph Lameter <cl@linux.com> writes:

> On Thu, 19 Jan 2012, Eric W. Biederman wrote:
>
>> Oh.  I see.  The problem is calling kobject_uevent (which happens to
>> live in slabs sysfs_slab_add) with a lock held.  And kobject_uevent
>> makes a blocking call to userspace.
>>
>> No locks held seems to be a good policy on that one.
>
> Well we can just remove that call to kobject_uevent instead then. Does it
> do anything useful? Cannot remember why we put that in there.

Empirically it sounds like something is listening for it and doing cat
/proc/slabinfo.  Something like that would have to occur for their to be
a deadlock that was observed.

On the flip side removing from sysfs with locks held must be done
carefully, and as a default I would recommend not to hold locks over
removing things from sysfs.  As removal blocks waiting for all of the
callers into sysfs those sysfs attributes to complete.

It looks like you are ok on the removal because none of the sysfs
attributes appear to take the slub_lock, just /proc/slabinfo.  But
it does look like playing with fire.

Eric

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

[Christoph Lameter]

On Thu, 19 Jan 2012, Eric W. Biederman wrote:

> On the flip side removing from sysfs with locks held must be done
> carefully, and as a default I would recommend not to hold locks over
> removing things from sysfs.  As removal blocks waiting for all of the
> callers into sysfs those sysfs attributes to complete.
>
> It looks like you are ok on the removal because none of the sysfs
> attributes appear to take the slub_lock, just /proc/slabinfo.  But
> it does look like playing with fire.

Ok then I guess my last patch is needed to make sysfs operations safe.

It may be good to audit the kernel for locks being held while calling
sysfs functions. Isnt there a lockdep check that ensures that no locks are
held?

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

[Eric W. Biederman]

Christoph Lameter <cl@linux.com> writes:

> On Thu, 19 Jan 2012, Eric W. Biederman wrote:
>
>> On the flip side removing from sysfs with locks held must be done
>> carefully, and as a default I would recommend not to hold locks over
>> removing things from sysfs.  As removal blocks waiting for all of the
>> callers into sysfs those sysfs attributes to complete.
>>
>> It looks like you are ok on the removal because none of the sysfs
>> attributes appear to take the slub_lock, just /proc/slabinfo.  But
>> it does look like playing with fire.
>
> Ok then I guess my last patch is needed to make sysfs operations safe.
>
> It may be good to audit the kernel for locks being held while calling
> sysfs functions. Isnt there a lockdep check that ensures that no locks are
> held?

I don't see a no locks are held check but call_usermodehelper in the
blocking case could certainly use one.

For the sysfs remove case lockdep should work.

Eric

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

[Pekka Enberg]

On Fri, Jan 20, 2012 at 4:49 PM, Christoph Lameter <cl@linux.com> wrote:
> Ok then I guess my last patch is needed to make sysfs operations safe.

Hmm. So is the latter patch needed or not?

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

[Pekka Enberg]

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

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

[Eric W. Biederman]

Pekka Enberg <penberg@kernel.org> writes:

> On Fri, Jan 20, 2012 at 4:49 PM, Christoph Lameter <cl@linux.com> wrote:
>> Ok then I guess my last patch is needed to make sysfs operations safe.
>
> Hmm. So is the latter patch needed or not?

Imperfect changelog but yes.

Eric