* [PATCH 5.13] mm, slub: move slub_debug static key enabling outside slab_mutex
@ 2021-05-04 12:00 Vlastimil Babka
2021-05-04 12:01 ` Vlastimil Babka
0 siblings, 1 reply; 4+ messages in thread
From: Vlastimil Babka @ 2021-05-04 12:00 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, David Rientjes, Joonsoo Kim,
Christoph Lameter, Pekka Enberg, Vlastimil Babka,
Paul E . McKenney
Paul E. McKenney reported [1] that commit 1f0723a4c0df ("mm, slub: enable
slub_debug static key when creating cache with explicit debug flags") results
in the lockdep complaint:
======================================================
WARNING: possible circular locking dependency detected
5.12.0+ #15 Not tainted
------------------------------------------------------
rcu_torture_sta/109 is trying to acquire lock:
ffffffff96063cd0 (cpu_hotplug_lock){++++}-{0:0}, at: static_key_enable+0x9/0x20
but task is already holding lock:
ffffffff96173c28 (slab_mutex){+.+.}-{3:3}, at: kmem_cache_create_usercopy+0x2d/0x250
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (slab_mutex){+.+.}-{3:3}:
lock_acquire+0xb9/0x3a0
__mutex_lock+0x8d/0x920
slub_cpu_dead+0x15/0xf0
cpuhp_invoke_callback+0x17a/0x7c0
cpuhp_invoke_callback_range+0x3b/0x80
_cpu_down+0xdf/0x2a0
cpu_down+0x2c/0x50
device_offline+0x82/0xb0
remove_cpu+0x1a/0x30
torture_offline+0x80/0x140
torture_onoff+0x147/0x260
kthread+0x10a/0x140
ret_from_fork+0x22/0x30
-> #0 (cpu_hotplug_lock){++++}-{0:0}:
check_prev_add+0x8f/0xbf0
__lock_acquire+0x13f0/0x1d80
lock_acquire+0xb9/0x3a0
cpus_read_lock+0x21/0xa0
static_key_enable+0x9/0x20
__kmem_cache_create+0x38d/0x430
kmem_cache_create_usercopy+0x146/0x250
kmem_cache_create+0xd/0x10
rcu_torture_stats+0x79/0x280
kthread+0x10a/0x140
ret_from_fork+0x22/0x30
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(slab_mutex);
lock(cpu_hotplug_lock);
lock(slab_mutex);
lock(cpu_hotplug_lock);
*** DEADLOCK ***
1 lock held by rcu_torture_sta/109:
#0: ffffffff96173c28 (slab_mutex){+.+.}-{3:3}, at: kmem_cache_create_usercopy+0x2d/0x250
stack backtrace:
CPU: 3 PID: 109 Comm: rcu_torture_sta Not tainted 5.12.0+ #15
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-1ubuntu1.1 04/01/2014
Call Trace:
dump_stack+0x6d/0x89
check_noncircular+0xfe/0x110
? lock_is_held_type+0x98/0x110
check_prev_add+0x8f/0xbf0
__lock_acquire+0x13f0/0x1d80
lock_acquire+0xb9/0x3a0
? static_key_enable+0x9/0x20
? mark_held_locks+0x49/0x70
cpus_read_lock+0x21/0xa0
? static_key_enable+0x9/0x20
static_key_enable+0x9/0x20
__kmem_cache_create+0x38d/0x430
kmem_cache_create_usercopy+0x146/0x250
? rcu_torture_stats_print+0xd0/0xd0
kmem_cache_create+0xd/0x10
rcu_torture_stats+0x79/0x280
? rcu_torture_stats_print+0xd0/0xd0
kthread+0x10a/0x140
? kthread_park+0x80/0x80
ret_from_fork+0x22/0x30
This is because there's one order of locking from the hotplug callbacks:
lock(cpu_hotplug_lock); // from hotplug machinery itself
lock(slab_mutex); // in e.g. slab_mem_going_offline_callback()
And commit 1f0723a4c0df made the reverse sequence possible:
lock(slab_mutex); // in kmem_cache_create_usercopy()
lock(cpu_hotplug_lock); // kmem_cache_open() -> static_key_enable()
The simplest fix is to move static_key_enable() to a place before slab_mutex is
taken. That means kmem_cache_create_usercopy() in mm/slab_common.c which is not
ideal for SLUB-specific code, but the #ifdef CONFIG_SLUB_DEBUG makes it
at least self-contained and obvious.
[1] https://lore.kernel.org/lkml/20210502171827.GA3670492@paulmck-ThinkPad-P17-Gen-1/
Reported-and-tested-by: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
mm/slab_common.c | 10 ++++++++++
mm/slub.c | 9 ---------
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/mm/slab_common.c b/mm/slab_common.c
index f8833d3e5d47..a4a571428c51 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -318,6 +318,16 @@ kmem_cache_create_usercopy(const char *name,
const char *cache_name;
int err;
+#ifdef CONFIG_SLUB_DEBUG
+ /*
+ * If no slub_debug was enabled globally, the static key is not yet
+ * enabled by setup_slub_debug(). Enable it if the cache is being
+ * created with any of the debugging flags passed explicitly.
+ */
+ if (flags & SLAB_DEBUG_FLAGS)
+ static_branch_enable(&slub_debug_enabled);
+#endif
+
mutex_lock(&slab_mutex);
err = kmem_cache_sanity_check(name, size);
diff --git a/mm/slub.c b/mm/slub.c
index 68123b21e65f..ec87ae218d45 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3828,15 +3828,6 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
static int kmem_cache_open(struct kmem_cache *s, slab_flags_t flags)
{
-#ifdef CONFIG_SLUB_DEBUG
- /*
- * If no slub_debug was enabled globally, the static key is not yet
- * enabled by setup_slub_debug(). Enable it if the cache is being
- * created with any of the debugging flags passed explicitly.
- */
- if (flags & SLAB_DEBUG_FLAGS)
- static_branch_enable(&slub_debug_enabled);
-#endif
s->flags = kmem_cache_flags(s->size, flags, s->name);
#ifdef CONFIG_SLAB_FREELIST_HARDENED
s->random = get_random_long();
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 5.13] mm, slub: move slub_debug static key enabling outside slab_mutex
2021-05-04 12:00 [PATCH 5.13] mm, slub: move slub_debug static key enabling outside slab_mutex Vlastimil Babka
@ 2021-05-04 12:01 ` Vlastimil Babka
2021-05-05 2:52 ` David Rientjes
0 siblings, 1 reply; 4+ messages in thread
From: Vlastimil Babka @ 2021-05-04 12:01 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, David Rientjes, Joonsoo Kim,
Christoph Lameter, Pekka Enberg, Paul E . McKenney
On 5/4/21 2:00 PM, Vlastimil Babka wrote:
> Paul E. McKenney reported [1] that commit 1f0723a4c0df ("mm, slub: enable
> slub_debug static key when creating cache with explicit debug flags") results
> in the lockdep complaint:
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.12.0+ #15 Not tainted
> ------------------------------------------------------
> rcu_torture_sta/109 is trying to acquire lock:
> ffffffff96063cd0 (cpu_hotplug_lock){++++}-{0:0}, at: static_key_enable+0x9/0x20
>
> but task is already holding lock:
> ffffffff96173c28 (slab_mutex){+.+.}-{3:3}, at: kmem_cache_create_usercopy+0x2d/0x250
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (slab_mutex){+.+.}-{3:3}:
> lock_acquire+0xb9/0x3a0
> __mutex_lock+0x8d/0x920
> slub_cpu_dead+0x15/0xf0
> cpuhp_invoke_callback+0x17a/0x7c0
> cpuhp_invoke_callback_range+0x3b/0x80
> _cpu_down+0xdf/0x2a0
> cpu_down+0x2c/0x50
> device_offline+0x82/0xb0
> remove_cpu+0x1a/0x30
> torture_offline+0x80/0x140
> torture_onoff+0x147/0x260
> kthread+0x10a/0x140
> ret_from_fork+0x22/0x30
>
> -> #0 (cpu_hotplug_lock){++++}-{0:0}:
> check_prev_add+0x8f/0xbf0
> __lock_acquire+0x13f0/0x1d80
> lock_acquire+0xb9/0x3a0
> cpus_read_lock+0x21/0xa0
> static_key_enable+0x9/0x20
> __kmem_cache_create+0x38d/0x430
> kmem_cache_create_usercopy+0x146/0x250
> kmem_cache_create+0xd/0x10
> rcu_torture_stats+0x79/0x280
> kthread+0x10a/0x140
> ret_from_fork+0x22/0x30
>
> other info that might help us debug this:
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(slab_mutex);
> lock(cpu_hotplug_lock);
> lock(slab_mutex);
> lock(cpu_hotplug_lock);
>
> *** DEADLOCK ***
>
> 1 lock held by rcu_torture_sta/109:
> #0: ffffffff96173c28 (slab_mutex){+.+.}-{3:3}, at: kmem_cache_create_usercopy+0x2d/0x250
>
> stack backtrace:
> CPU: 3 PID: 109 Comm: rcu_torture_sta Not tainted 5.12.0+ #15
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-1ubuntu1.1 04/01/2014
> Call Trace:
> dump_stack+0x6d/0x89
> check_noncircular+0xfe/0x110
> ? lock_is_held_type+0x98/0x110
> check_prev_add+0x8f/0xbf0
> __lock_acquire+0x13f0/0x1d80
> lock_acquire+0xb9/0x3a0
> ? static_key_enable+0x9/0x20
> ? mark_held_locks+0x49/0x70
> cpus_read_lock+0x21/0xa0
> ? static_key_enable+0x9/0x20
> static_key_enable+0x9/0x20
> __kmem_cache_create+0x38d/0x430
> kmem_cache_create_usercopy+0x146/0x250
> ? rcu_torture_stats_print+0xd0/0xd0
> kmem_cache_create+0xd/0x10
> rcu_torture_stats+0x79/0x280
> ? rcu_torture_stats_print+0xd0/0xd0
> kthread+0x10a/0x140
> ? kthread_park+0x80/0x80
> ret_from_fork+0x22/0x30
>
> This is because there's one order of locking from the hotplug callbacks:
>
> lock(cpu_hotplug_lock); // from hotplug machinery itself
> lock(slab_mutex); // in e.g. slab_mem_going_offline_callback()
>
> And commit 1f0723a4c0df made the reverse sequence possible:
> lock(slab_mutex); // in kmem_cache_create_usercopy()
> lock(cpu_hotplug_lock); // kmem_cache_open() -> static_key_enable()
>
> The simplest fix is to move static_key_enable() to a place before slab_mutex is
> taken. That means kmem_cache_create_usercopy() in mm/slab_common.c which is not
> ideal for SLUB-specific code, but the #ifdef CONFIG_SLUB_DEBUG makes it
> at least self-contained and obvious.
>
> [1] https://lore.kernel.org/lkml/20210502171827.GA3670492@paulmck-ThinkPad-P17-Gen-1/
>
> Reported-and-tested-by: Paul E. McKenney <paulmck@kernel.org>
Forgot:
Fixes: 1f0723a4c0df ("mm, slub: enable slub_debug static key when creating cache
with explicit debug flags")
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 5.13] mm, slub: move slub_debug static key enabling outside slab_mutex
2021-05-04 12:01 ` Vlastimil Babka
@ 2021-05-05 2:52 ` David Rientjes
0 siblings, 0 replies; 4+ messages in thread
From: David Rientjes @ 2021-05-05 2:52 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, linux-mm, linux-kernel, Joonsoo Kim,
Christoph Lameter, Pekka Enberg, Paul E . McKenney
On Tue, 4 May 2021, Vlastimil Babka wrote:
> On 5/4/21 2:00 PM, Vlastimil Babka wrote:
> > Paul E. McKenney reported [1] that commit 1f0723a4c0df ("mm, slub: enable
> > slub_debug static key when creating cache with explicit debug flags") results
> > in the lockdep complaint:
> >
> > ======================================================
> > WARNING: possible circular locking dependency detected
> > 5.12.0+ #15 Not tainted
> > ------------------------------------------------------
> > rcu_torture_sta/109 is trying to acquire lock:
> > ffffffff96063cd0 (cpu_hotplug_lock){++++}-{0:0}, at: static_key_enable+0x9/0x20
> >
> > but task is already holding lock:
> > ffffffff96173c28 (slab_mutex){+.+.}-{3:3}, at: kmem_cache_create_usercopy+0x2d/0x250
> >
> > which lock already depends on the new lock.
> >
> > the existing dependency chain (in reverse order) is:
> >
> > -> #1 (slab_mutex){+.+.}-{3:3}:
> > lock_acquire+0xb9/0x3a0
> > __mutex_lock+0x8d/0x920
> > slub_cpu_dead+0x15/0xf0
> > cpuhp_invoke_callback+0x17a/0x7c0
> > cpuhp_invoke_callback_range+0x3b/0x80
> > _cpu_down+0xdf/0x2a0
> > cpu_down+0x2c/0x50
> > device_offline+0x82/0xb0
> > remove_cpu+0x1a/0x30
> > torture_offline+0x80/0x140
> > torture_onoff+0x147/0x260
> > kthread+0x10a/0x140
> > ret_from_fork+0x22/0x30
> >
> > -> #0 (cpu_hotplug_lock){++++}-{0:0}:
> > check_prev_add+0x8f/0xbf0
> > __lock_acquire+0x13f0/0x1d80
> > lock_acquire+0xb9/0x3a0
> > cpus_read_lock+0x21/0xa0
> > static_key_enable+0x9/0x20
> > __kmem_cache_create+0x38d/0x430
> > kmem_cache_create_usercopy+0x146/0x250
> > kmem_cache_create+0xd/0x10
> > rcu_torture_stats+0x79/0x280
> > kthread+0x10a/0x140
> > ret_from_fork+0x22/0x30
> >
> > other info that might help us debug this:
> >
> > Possible unsafe locking scenario:
> >
> > CPU0 CPU1
> > ---- ----
> > lock(slab_mutex);
> > lock(cpu_hotplug_lock);
> > lock(slab_mutex);
> > lock(cpu_hotplug_lock);
> >
> > *** DEADLOCK ***
> >
> > 1 lock held by rcu_torture_sta/109:
> > #0: ffffffff96173c28 (slab_mutex){+.+.}-{3:3}, at: kmem_cache_create_usercopy+0x2d/0x250
> >
> > stack backtrace:
> > CPU: 3 PID: 109 Comm: rcu_torture_sta Not tainted 5.12.0+ #15
> > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-1ubuntu1.1 04/01/2014
> > Call Trace:
> > dump_stack+0x6d/0x89
> > check_noncircular+0xfe/0x110
> > ? lock_is_held_type+0x98/0x110
> > check_prev_add+0x8f/0xbf0
> > __lock_acquire+0x13f0/0x1d80
> > lock_acquire+0xb9/0x3a0
> > ? static_key_enable+0x9/0x20
> > ? mark_held_locks+0x49/0x70
> > cpus_read_lock+0x21/0xa0
> > ? static_key_enable+0x9/0x20
> > static_key_enable+0x9/0x20
> > __kmem_cache_create+0x38d/0x430
> > kmem_cache_create_usercopy+0x146/0x250
> > ? rcu_torture_stats_print+0xd0/0xd0
> > kmem_cache_create+0xd/0x10
> > rcu_torture_stats+0x79/0x280
> > ? rcu_torture_stats_print+0xd0/0xd0
> > kthread+0x10a/0x140
> > ? kthread_park+0x80/0x80
> > ret_from_fork+0x22/0x30
> >
> > This is because there's one order of locking from the hotplug callbacks:
> >
> > lock(cpu_hotplug_lock); // from hotplug machinery itself
> > lock(slab_mutex); // in e.g. slab_mem_going_offline_callback()
> >
> > And commit 1f0723a4c0df made the reverse sequence possible:
> > lock(slab_mutex); // in kmem_cache_create_usercopy()
> > lock(cpu_hotplug_lock); // kmem_cache_open() -> static_key_enable()
> >
> > The simplest fix is to move static_key_enable() to a place before slab_mutex is
> > taken. That means kmem_cache_create_usercopy() in mm/slab_common.c which is not
> > ideal for SLUB-specific code, but the #ifdef CONFIG_SLUB_DEBUG makes it
> > at least self-contained and obvious.
> >
> > [1] https://lore.kernel.org/lkml/20210502171827.GA3670492@paulmck-ThinkPad-P17-Gen-1/
> >
> > Reported-and-tested-by: Paul E. McKenney <paulmck@kernel.org>
>
> Forgot:
> Fixes: 1f0723a4c0df ("mm, slub: enable slub_debug static key when creating cache
> with explicit debug flags")
>
Acked-by: David Rientjes <rientjes@google.com>
Nice concise fix!
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 5.13] mm, slub: move slub_debug static key enabling outside slab_mutex
@ 2021-05-05 2:52 ` David Rientjes
0 siblings, 0 replies; 4+ messages in thread
From: David Rientjes @ 2021-05-05 2:52 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, linux-mm, linux-kernel, Joonsoo Kim,
Christoph Lameter, Pekka Enberg, Paul E . McKenney
On Tue, 4 May 2021, Vlastimil Babka wrote:
> On 5/4/21 2:00 PM, Vlastimil Babka wrote:
> > Paul E. McKenney reported [1] that commit 1f0723a4c0df ("mm, slub: enable
> > slub_debug static key when creating cache with explicit debug flags") results
> > in the lockdep complaint:
> >
> > ======================================================
> > WARNING: possible circular locking dependency detected
> > 5.12.0+ #15 Not tainted
> > ------------------------------------------------------
> > rcu_torture_sta/109 is trying to acquire lock:
> > ffffffff96063cd0 (cpu_hotplug_lock){++++}-{0:0}, at: static_key_enable+0x9/0x20
> >
> > but task is already holding lock:
> > ffffffff96173c28 (slab_mutex){+.+.}-{3:3}, at: kmem_cache_create_usercopy+0x2d/0x250
> >
> > which lock already depends on the new lock.
> >
> > the existing dependency chain (in reverse order) is:
> >
> > -> #1 (slab_mutex){+.+.}-{3:3}:
> > lock_acquire+0xb9/0x3a0
> > __mutex_lock+0x8d/0x920
> > slub_cpu_dead+0x15/0xf0
> > cpuhp_invoke_callback+0x17a/0x7c0
> > cpuhp_invoke_callback_range+0x3b/0x80
> > _cpu_down+0xdf/0x2a0
> > cpu_down+0x2c/0x50
> > device_offline+0x82/0xb0
> > remove_cpu+0x1a/0x30
> > torture_offline+0x80/0x140
> > torture_onoff+0x147/0x260
> > kthread+0x10a/0x140
> > ret_from_fork+0x22/0x30
> >
> > -> #0 (cpu_hotplug_lock){++++}-{0:0}:
> > check_prev_add+0x8f/0xbf0
> > __lock_acquire+0x13f0/0x1d80
> > lock_acquire+0xb9/0x3a0
> > cpus_read_lock+0x21/0xa0
> > static_key_enable+0x9/0x20
> > __kmem_cache_create+0x38d/0x430
> > kmem_cache_create_usercopy+0x146/0x250
> > kmem_cache_create+0xd/0x10
> > rcu_torture_stats+0x79/0x280
> > kthread+0x10a/0x140
> > ret_from_fork+0x22/0x30
> >
> > other info that might help us debug this:
> >
> > Possible unsafe locking scenario:
> >
> > CPU0 CPU1
> > ---- ----
> > lock(slab_mutex);
> > lock(cpu_hotplug_lock);
> > lock(slab_mutex);
> > lock(cpu_hotplug_lock);
> >
> > *** DEADLOCK ***
> >
> > 1 lock held by rcu_torture_sta/109:
> > #0: ffffffff96173c28 (slab_mutex){+.+.}-{3:3}, at: kmem_cache_create_usercopy+0x2d/0x250
> >
> > stack backtrace:
> > CPU: 3 PID: 109 Comm: rcu_torture_sta Not tainted 5.12.0+ #15
> > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-1ubuntu1.1 04/01/2014
> > Call Trace:
> > dump_stack+0x6d/0x89
> > check_noncircular+0xfe/0x110
> > ? lock_is_held_type+0x98/0x110
> > check_prev_add+0x8f/0xbf0
> > __lock_acquire+0x13f0/0x1d80
> > lock_acquire+0xb9/0x3a0
> > ? static_key_enable+0x9/0x20
> > ? mark_held_locks+0x49/0x70
> > cpus_read_lock+0x21/0xa0
> > ? static_key_enable+0x9/0x20
> > static_key_enable+0x9/0x20
> > __kmem_cache_create+0x38d/0x430
> > kmem_cache_create_usercopy+0x146/0x250
> > ? rcu_torture_stats_print+0xd0/0xd0
> > kmem_cache_create+0xd/0x10
> > rcu_torture_stats+0x79/0x280
> > ? rcu_torture_stats_print+0xd0/0xd0
> > kthread+0x10a/0x140
> > ? kthread_park+0x80/0x80
> > ret_from_fork+0x22/0x30
> >
> > This is because there's one order of locking from the hotplug callbacks:
> >
> > lock(cpu_hotplug_lock); // from hotplug machinery itself
> > lock(slab_mutex); // in e.g. slab_mem_going_offline_callback()
> >
> > And commit 1f0723a4c0df made the reverse sequence possible:
> > lock(slab_mutex); // in kmem_cache_create_usercopy()
> > lock(cpu_hotplug_lock); // kmem_cache_open() -> static_key_enable()
> >
> > The simplest fix is to move static_key_enable() to a place before slab_mutex is
> > taken. That means kmem_cache_create_usercopy() in mm/slab_common.c which is not
> > ideal for SLUB-specific code, but the #ifdef CONFIG_SLUB_DEBUG makes it
> > at least self-contained and obvious.
> >
> > [1] https://lore.kernel.org/lkml/20210502171827.GA3670492@paulmck-ThinkPad-P17-Gen-1/
> >
> > Reported-and-tested-by: Paul E. McKenney <paulmck@kernel.org>
>
> Forgot:
> Fixes: 1f0723a4c0df ("mm, slub: enable slub_debug static key when creating cache
> with explicit debug flags")
>
Acked-by: David Rientjes <rientjes@google.com>
Nice concise fix!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-05-05 2:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-04 12:00 [PATCH 5.13] mm, slub: move slub_debug static key enabling outside slab_mutex Vlastimil Babka
2021-05-04 12:01 ` Vlastimil Babka
2021-05-05 2:52 ` David Rientjes
2021-05-05 2:52 ` David Rientjes
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.