linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [LOCKDEP BUG] from slub: separate out sysfs_slab_release() from sysfs_slab_remove()
@ 2017-06-16 12:55 Steven Rostedt
  2017-06-19 20:35 ` Tejun Heo
  0 siblings, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2017-06-16 12:55 UTC (permalink / raw)
  To: Tejun Heo
  Cc: LKML, Linus Torvalds, Vladimir Davydov, Christoph Lameter,
	Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton

[-- Attachment #1: Type: text/plain, Size: 3499 bytes --]

In my tests, I hit the following lockdep splat:

 
 ======================================================
 [ INFO: possible circular locking dependency detected ]
 4.10.0-test+ #48 Not tainted
 -------------------------------------------------------
 rmmod/1211 is trying to acquire lock:
  (s_active#120){++++.+}, at: [<ffffffff81308073>] kernfs_remove+0x23/0x40
 
 but task is already holding lock:
  (slab_mutex){+.+.+.}, at: [<ffffffff8120f691>] kmem_cache_destroy+0x41/0x2d0
 
 which lock already depends on the new lock.
 
 
 the existing dependency chain (in reverse order) is:
 
 -> #1 (slab_mutex){+.+.+.}:
        lock_acquire+0xf6/0x1f0
        __mutex_lock+0x75/0x950
        mutex_lock_nested+0x1b/0x20
        slab_attr_store+0x75/0xd0
        sysfs_kf_write+0x45/0x60
        kernfs_fop_write+0x13c/0x1c0
        __vfs_write+0x28/0x120
        vfs_write+0xc8/0x1e0
        SyS_write+0x49/0xa0
        entry_SYSCALL_64_fastpath+0x1f/0xc2
 
 -> #0 (s_active#120){++++.+}:
        __lock_acquire+0x10ed/0x1260
        lock_acquire+0xf6/0x1f0
        __kernfs_remove+0x254/0x320
        kernfs_remove+0x23/0x40
        sysfs_remove_dir+0x51/0x80
        kobject_del+0x18/0x50
        __kmem_cache_shutdown+0x3e6/0x460
        kmem_cache_destroy+0x1fb/0x2d0
        kvm_exit+0x2d/0x80 [kvm]
        vmx_exit+0x19/0xa1b [kvm_intel]
        SyS_delete_module+0x198/0x1f0
        entry_SYSCALL_64_fastpath+0x1f/0xc2
 
 other info that might help us debug this:
 
  Possible unsafe locking scenario:
 
        CPU0                    CPU1
        ----                    ----
   lock(slab_mutex);
                                lock(s_active#120);
                                lock(slab_mutex);
   lock(s_active#120);
 
  *** DEADLOCK ***
 
 2 locks held by rmmod/1211:
  #0:  (cpu_hotplug.dep_map){++++++}, at: [<ffffffff810a7877>] get_online_cpus+0x37/0x80
  #1:  (slab_mutex){+.+.+.}, at: [<ffffffff8120f691>] kmem_cache_destroy+0x41/0x2d0
 
 stack backtrace:
 CPU: 3 PID: 1211 Comm: rmmod Not tainted 4.10.0-test+ #48
 Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v02.05 05/07/2012
 Call Trace:
  dump_stack+0x86/0xc3
  print_circular_bug+0x1be/0x210
  __lock_acquire+0x10ed/0x1260
  ? 0xffffffffa0000077
  lock_acquire+0xf6/0x1f0
  ? kernfs_remove+0x23/0x40
  __kernfs_remove+0x254/0x320
  ? kernfs_remove+0x23/0x40
  ? __kernfs_remove+0x5/0x320
  kernfs_remove+0x23/0x40
  sysfs_remove_dir+0x51/0x80
  kobject_del+0x18/0x50
  __kmem_cache_shutdown+0x3e6/0x460
  kmem_cache_destroy+0x1fb/0x2d0
  kvm_exit+0x2d/0x80 [kvm]
  vmx_exit+0x19/0xa1b [kvm_intel]
  SyS_delete_module+0x198/0x1f0
  ? SyS_delete_module+0x5/0x1f0
  entry_SYSCALL_64_fastpath+0x1f/0xc2
 RIP: 0033:0x7fdf0439f487
 RSP: 002b:00007ffcd39eb6b8 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
 RAX: ffffffffffffffda RBX: 0000000000000046 RCX: 00007fdf0439f487
 RDX: 000000000000000a RSI: 0000000000000800 RDI: 000055e4e32f3258
 RBP: 0000000000000000 R08: 000000000000000a R09: 0000000000000000
 R10: 00007fdf0440ace0 R11: 0000000000000206 R12: 000055e4e32f31f0
 R13: 00007ffcd39ea6a0 R14: 0000000000000000 R15: 000055e4e32f31f0


I bisected it down to commit bf5eb3de3847ebcfd

"slub: separate out sysfs_slab_release() from sysfs_slab_remove()"

To hit this bug, I simply had to log in and perform:

 # rmmod kvm_intel

and it triggered.

It looks as though this commit was added to allow for other changes, so
just reverting it wont work.

I attached the config that triggers this too.

-- Steve

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 28205 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LOCKDEP BUG] from slub: separate out sysfs_slab_release() from sysfs_slab_remove()
  2017-06-16 12:55 [LOCKDEP BUG] from slub: separate out sysfs_slab_release() from sysfs_slab_remove() Steven Rostedt
@ 2017-06-19 20:35 ` Tejun Heo
  2017-06-19 21:27   ` Steven Rostedt
  0 siblings, 1 reply; 9+ messages in thread
From: Tejun Heo @ 2017-06-19 20:35 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: LKML, Linus Torvalds, Vladimir Davydov, Christoph Lameter,
	Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton

Hello, Steven.

Can you please see whether the following patch makes the lockdep
warning go away?

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index 07ef550c6627..93315d6b21a8 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -84,6 +84,7 @@ struct kmem_cache {
 	int red_left_pad;	/* Left redzone padding size */
 #ifdef CONFIG_SYSFS
 	struct kobject kobj;	/* For sysfs */
+	struct work_struct kobj_remove_work;
 #endif
 #ifdef CONFIG_MEMCG
 	struct memcg_cache_params memcg_params;
diff --git a/mm/slub.c b/mm/slub.c
index 7449593fca72..8addc535bcdc 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5625,6 +5625,28 @@ static char *create_unique_id(struct kmem_cache *s)
 	return name;
 }
 
+static void sysfs_slab_remove_workfn(struct work_struct *work)
+{
+	struct kmem_cache *s =
+		container_of(work, struct kmem_cache, kobj_remove_work);
+
+	if (!s->kobj.state_in_sysfs)
+		/*
+		 * For a memcg cache, this may be called during
+		 * deactivation and again on shutdown.  Remove only once.
+		 * A cache is never shut down before deactivation is
+		 * complete, so no need to worry about synchronization.
+		 */
+		return;
+
+#ifdef CONFIG_MEMCG
+	kset_unregister(s->memcg_kset);
+#endif
+	kobject_uevent(&s->kobj, KOBJ_REMOVE);
+	kobject_del(&s->kobj);
+	kobject_put(&s->kobj);
+}
+
 static int sysfs_slab_add(struct kmem_cache *s)
 {
 	int err;
@@ -5632,6 +5654,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
 	struct kset *kset = cache_kset(s);
 	int unmergeable = slab_unmergeable(s);
 
+	INIT_WORK(&s->kobj_remove_work, sysfs_slab_remove_workfn);
+
 	if (!kset) {
 		kobject_init(&s->kobj, &slab_ktype);
 		return 0;
@@ -5695,20 +5719,8 @@ static void sysfs_slab_remove(struct kmem_cache *s)
 		 */
 		return;
 
-	if (!s->kobj.state_in_sysfs)
-		/*
-		 * For a memcg cache, this may be called during
-		 * deactivation and again on shutdown.  Remove only once.
-		 * A cache is never shut down before deactivation is
-		 * complete, so no need to worry about synchronization.
-		 */
-		return;
-
-#ifdef CONFIG_MEMCG
-	kset_unregister(s->memcg_kset);
-#endif
-	kobject_uevent(&s->kobj, KOBJ_REMOVE);
-	kobject_del(&s->kobj);
+	kobject_get(&s->kobj);
+	schedule_work(&s->kobj_remove_work);
 }
 
 void sysfs_slab_release(struct kmem_cache *s)

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [LOCKDEP BUG] from slub: separate out sysfs_slab_release() from sysfs_slab_remove()
  2017-06-19 20:35 ` Tejun Heo
@ 2017-06-19 21:27   ` Steven Rostedt
  2017-06-20 20:45     ` [PATCH] slub: make sysfs file removal asynchronous Tejun Heo
  0 siblings, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2017-06-19 21:27 UTC (permalink / raw)
  To: Tejun Heo
  Cc: LKML, Linus Torvalds, Vladimir Davydov, Christoph Lameter,
	Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton

On Mon, 19 Jun 2017 16:35:38 -0400
Tejun Heo <tj@kernel.org> wrote:

> Hello, Steven.
> 
> Can you please see whether the following patch makes the lockdep
> warning go away?
> 

Added the patch and the lockdep splat goes away. Removed it, and it
comes back.

Reported-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve


> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
> index 07ef550c6627..93315d6b21a8 100644
> --- a/include/linux/slub_def.h
> +++ b/include/linux/slub_def.h
> @@ -84,6 +84,7 @@ struct kmem_cache {
>  	int red_left_pad;	/* Left redzone padding size */
>  #ifdef CONFIG_SYSFS
>  	struct kobject kobj;	/* For sysfs */
> +	struct work_struct kobj_remove_work;
>  #endif
>  #ifdef CONFIG_MEMCG
>  	struct memcg_cache_params memcg_params;
> diff --git a/mm/slub.c b/mm/slub.c
> index 7449593fca72..8addc535bcdc 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5625,6 +5625,28 @@ static char *create_unique_id(struct kmem_cache *s)
>  	return name;
>  }
>  
> +static void sysfs_slab_remove_workfn(struct work_struct *work)
> +{
> +	struct kmem_cache *s =
> +		container_of(work, struct kmem_cache, kobj_remove_work);
> +
> +	if (!s->kobj.state_in_sysfs)
> +		/*
> +		 * For a memcg cache, this may be called during
> +		 * deactivation and again on shutdown.  Remove only once.
> +		 * A cache is never shut down before deactivation is
> +		 * complete, so no need to worry about synchronization.
> +		 */
> +		return;
> +
> +#ifdef CONFIG_MEMCG
> +	kset_unregister(s->memcg_kset);
> +#endif
> +	kobject_uevent(&s->kobj, KOBJ_REMOVE);
> +	kobject_del(&s->kobj);
> +	kobject_put(&s->kobj);
> +}
> +
>  static int sysfs_slab_add(struct kmem_cache *s)
>  {
>  	int err;
> @@ -5632,6 +5654,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
>  	struct kset *kset = cache_kset(s);
>  	int unmergeable = slab_unmergeable(s);
>  
> +	INIT_WORK(&s->kobj_remove_work, sysfs_slab_remove_workfn);
> +
>  	if (!kset) {
>  		kobject_init(&s->kobj, &slab_ktype);
>  		return 0;
> @@ -5695,20 +5719,8 @@ static void sysfs_slab_remove(struct kmem_cache *s)
>  		 */
>  		return;
>  
> -	if (!s->kobj.state_in_sysfs)
> -		/*
> -		 * For a memcg cache, this may be called during
> -		 * deactivation and again on shutdown.  Remove only once.
> -		 * A cache is never shut down before deactivation is
> -		 * complete, so no need to worry about synchronization.
> -		 */
> -		return;
> -
> -#ifdef CONFIG_MEMCG
> -	kset_unregister(s->memcg_kset);
> -#endif
> -	kobject_uevent(&s->kobj, KOBJ_REMOVE);
> -	kobject_del(&s->kobj);
> +	kobject_get(&s->kobj);
> +	schedule_work(&s->kobj_remove_work);
>  }
>  
>  void sysfs_slab_release(struct kmem_cache *s)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] slub: make sysfs file removal asynchronous
  2017-06-19 21:27   ` Steven Rostedt
@ 2017-06-20 20:45     ` Tejun Heo
  2017-06-20 21:58       ` Andrew Morton
  0 siblings, 1 reply; 9+ messages in thread
From: Tejun Heo @ 2017-06-20 20:45 UTC (permalink / raw)
  To: Christoph Lameter, Pekka Enberg
  Cc: LKML, Linus Torvalds, Vladimir Davydov, David Rientjes,
	Joonsoo Kim, Andrew Morton, Steven Rostedt

bf5eb3de3847 ("slub: separate out sysfs_slab_release() from
sysfs_slab_remove()") made slub sysfs file removals synchronous to
kmem_cache shutdown.  Unfortunately, this created a possible ABBA
deadlock between slab_mutex and sysfs draining mechanism triggering
the following lockdep warning.

  ======================================================
  [ INFO: possible circular locking dependency detected ]
  4.10.0-test+ #48 Not tainted
  -------------------------------------------------------
  rmmod/1211 is trying to acquire lock:
   (s_active#120){++++.+}, at: [<ffffffff81308073>] kernfs_remove+0x23/0x40

  but task is already holding lock:
   (slab_mutex){+.+.+.}, at: [<ffffffff8120f691>] kmem_cache_destroy+0x41/0x2d0

  which lock already depends on the new lock.


  the existing dependency chain (in reverse order) is:

  -> #1 (slab_mutex){+.+.+.}:
	 lock_acquire+0xf6/0x1f0
	 __mutex_lock+0x75/0x950
	 mutex_lock_nested+0x1b/0x20
	 slab_attr_store+0x75/0xd0
	 sysfs_kf_write+0x45/0x60
	 kernfs_fop_write+0x13c/0x1c0
	 __vfs_write+0x28/0x120
	 vfs_write+0xc8/0x1e0
	 SyS_write+0x49/0xa0
	 entry_SYSCALL_64_fastpath+0x1f/0xc2

  -> #0 (s_active#120){++++.+}:
	 __lock_acquire+0x10ed/0x1260
	 lock_acquire+0xf6/0x1f0
	 __kernfs_remove+0x254/0x320
	 kernfs_remove+0x23/0x40
	 sysfs_remove_dir+0x51/0x80
	 kobject_del+0x18/0x50
	 __kmem_cache_shutdown+0x3e6/0x460
	 kmem_cache_destroy+0x1fb/0x2d0
	 kvm_exit+0x2d/0x80 [kvm]
	 vmx_exit+0x19/0xa1b [kvm_intel]
	 SyS_delete_module+0x198/0x1f0
	 entry_SYSCALL_64_fastpath+0x1f/0xc2

  other info that might help us debug this:

   Possible unsafe locking scenario:

	 CPU0                    CPU1
	 ----                    ----
    lock(slab_mutex);
				 lock(s_active#120);
				 lock(slab_mutex);
    lock(s_active#120);

   *** DEADLOCK ***

  2 locks held by rmmod/1211:
   #0:  (cpu_hotplug.dep_map){++++++}, at: [<ffffffff810a7877>] get_online_cpus+0x37/0x80
   #1:  (slab_mutex){+.+.+.}, at: [<ffffffff8120f691>] kmem_cache_destroy+0x41/0x2d0

  stack backtrace:
  CPU: 3 PID: 1211 Comm: rmmod Not tainted 4.10.0-test+ #48
  Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v02.05 05/07/2012
  Call Trace:
   dump_stack+0x86/0xc3
   print_circular_bug+0x1be/0x210
   __lock_acquire+0x10ed/0x1260
   ? 0xffffffffa0000077
   lock_acquire+0xf6/0x1f0
   ? kernfs_remove+0x23/0x40
   __kernfs_remove+0x254/0x320
   ? kernfs_remove+0x23/0x40
   ? __kernfs_remove+0x5/0x320
   kernfs_remove+0x23/0x40
   sysfs_remove_dir+0x51/0x80
   kobject_del+0x18/0x50
   __kmem_cache_shutdown+0x3e6/0x460
   kmem_cache_destroy+0x1fb/0x2d0
   kvm_exit+0x2d/0x80 [kvm]
   vmx_exit+0x19/0xa1b [kvm_intel]
   SyS_delete_module+0x198/0x1f0
   ? SyS_delete_module+0x5/0x1f0
   entry_SYSCALL_64_fastpath+0x1f/0xc2
  RIP: 0033:0x7fdf0439f487
  RSP: 002b:00007ffcd39eb6b8 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
  RAX: ffffffffffffffda RBX: 0000000000000046 RCX: 00007fdf0439f487
  RDX: 000000000000000a RSI: 0000000000000800 RDI: 000055e4e32f3258
  RBP: 0000000000000000 R08: 000000000000000a R09: 0000000000000000
  R10: 00007fdf0440ace0 R11: 0000000000000206 R12: 000055e4e32f31f0
  R13: 00007ffcd39ea6a0 R14: 0000000000000000 R15: 000055e4e32f31f0

It'd be the cleanest to deal with the issue by removing sysfs files
without holding slab_mutex before the rest of shutdown; however, given
the current code structure, it is pretty difficult to do so.  This
patch punts sysfs file removal to a work item.  Before bf5eb3de3847,
the removal was punted to a RCU delayed work item which is executed
after release.  Now, we're punting to a different work item on
shutdown which still maintains the goal removing the sysfs files
earlier when destroying kmem_caches.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Fixes: bf5eb3de3847 ("slub: separate out sysfs_slab_release() from sysfs_slab_remove()")
---
 include/linux/slub_def.h |    1 +
 mm/slub.c                |   40 ++++++++++++++++++++++++++--------------
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index 07ef550c6627..93315d6b21a8 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -84,6 +84,7 @@ struct kmem_cache {
 	int red_left_pad;	/* Left redzone padding size */
 #ifdef CONFIG_SYSFS
 	struct kobject kobj;	/* For sysfs */
+	struct work_struct kobj_remove_work;
 #endif
 #ifdef CONFIG_MEMCG
 	struct memcg_cache_params memcg_params;
diff --git a/mm/slub.c b/mm/slub.c
index 7449593fca72..8addc535bcdc 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5625,6 +5625,28 @@ static char *create_unique_id(struct kmem_cache *s)
 	return name;
 }
 
+static void sysfs_slab_remove_workfn(struct work_struct *work)
+{
+	struct kmem_cache *s =
+		container_of(work, struct kmem_cache, kobj_remove_work);
+
+	if (!s->kobj.state_in_sysfs)
+		/*
+		 * For a memcg cache, this may be called during
+		 * deactivation and again on shutdown.  Remove only once.
+		 * A cache is never shut down before deactivation is
+		 * complete, so no need to worry about synchronization.
+		 */
+		return;
+
+#ifdef CONFIG_MEMCG
+	kset_unregister(s->memcg_kset);
+#endif
+	kobject_uevent(&s->kobj, KOBJ_REMOVE);
+	kobject_del(&s->kobj);
+	kobject_put(&s->kobj);
+}
+
 static int sysfs_slab_add(struct kmem_cache *s)
 {
 	int err;
@@ -5632,6 +5654,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
 	struct kset *kset = cache_kset(s);
 	int unmergeable = slab_unmergeable(s);
 
+	INIT_WORK(&s->kobj_remove_work, sysfs_slab_remove_workfn);
+
 	if (!kset) {
 		kobject_init(&s->kobj, &slab_ktype);
 		return 0;
@@ -5695,20 +5719,8 @@ static void sysfs_slab_remove(struct kmem_cache *s)
 		 */
 		return;
 
-	if (!s->kobj.state_in_sysfs)
-		/*
-		 * For a memcg cache, this may be called during
-		 * deactivation and again on shutdown.  Remove only once.
-		 * A cache is never shut down before deactivation is
-		 * complete, so no need to worry about synchronization.
-		 */
-		return;
-
-#ifdef CONFIG_MEMCG
-	kset_unregister(s->memcg_kset);
-#endif
-	kobject_uevent(&s->kobj, KOBJ_REMOVE);
-	kobject_del(&s->kobj);
+	kobject_get(&s->kobj);
+	schedule_work(&s->kobj_remove_work);
 }
 
 void sysfs_slab_release(struct kmem_cache *s)

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] slub: make sysfs file removal asynchronous
  2017-06-20 20:45     ` [PATCH] slub: make sysfs file removal asynchronous Tejun Heo
@ 2017-06-20 21:58       ` Andrew Morton
  2017-06-20 22:00         ` Tejun Heo
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2017-06-20 21:58 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Christoph Lameter, Pekka Enberg, LKML, Linus Torvalds,
	Vladimir Davydov, David Rientjes, Joonsoo Kim, Steven Rostedt

On Tue, 20 Jun 2017 16:45:12 -0400 Tejun Heo <tj@kernel.org> wrote:

> bf5eb3de3847 ("slub: separate out sysfs_slab_release() from
> sysfs_slab_remove()") made slub sysfs file removals synchronous to
> kmem_cache shutdown.  Unfortunately, this created a possible ABBA
> deadlock between slab_mutex and sysfs draining mechanism triggering
> the following lockdep warning.
> 
> ...
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Tested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Fixes: bf5eb3de3847 ("slub: separate out sysfs_slab_release() from sysfs_slab_remove()")

Do you think we should add cc:stable [4.11+]?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] slub: make sysfs file removal asynchronous
  2017-06-20 21:58       ` Andrew Morton
@ 2017-06-20 22:00         ` Tejun Heo
  2017-06-20 22:22           ` Steven Rostedt
  0 siblings, 1 reply; 9+ messages in thread
From: Tejun Heo @ 2017-06-20 22:00 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Christoph Lameter, Pekka Enberg, LKML, Linus Torvalds,
	Vladimir Davydov, David Rientjes, Joonsoo Kim, Steven Rostedt

On Tue, Jun 20, 2017 at 02:58:14PM -0700, Andrew Morton wrote:
> On Tue, 20 Jun 2017 16:45:12 -0400 Tejun Heo <tj@kernel.org> wrote:
> 
> > bf5eb3de3847 ("slub: separate out sysfs_slab_release() from
> > sysfs_slab_remove()") made slub sysfs file removals synchronous to
> > kmem_cache shutdown.  Unfortunately, this created a possible ABBA
> > deadlock between slab_mutex and sysfs draining mechanism triggering
> > the following lockdep warning.
> > 
> > ...
> >
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Reported-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > Tested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > Fixes: bf5eb3de3847 ("slub: separate out sysfs_slab_release() from sysfs_slab_remove()")
> 
> Do you think we should add cc:stable [4.11+]?

I think we'd risk more by backporting it through -stable than keeping
the bug there.  The bug is very difficult to hit.  Writing to a slub
sysfs file has to race against kmem_cache destruction and AFAICS all
slub sysfs files are for debugging.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] slub: make sysfs file removal asynchronous
  2017-06-20 22:00         ` Tejun Heo
@ 2017-06-20 22:22           ` Steven Rostedt
  2017-06-20 22:48             ` Tejun Heo
  0 siblings, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2017-06-20 22:22 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, LKML,
	Linus Torvalds, Vladimir Davydov, David Rientjes, Joonsoo Kim

On Tue, 20 Jun 2017 18:00:11 -0400
Tejun Heo <tj@kernel.org> wrote:

> > > Signed-off-by: Tejun Heo <tj@kernel.org>
> > > Reported-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > Tested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > Fixes: bf5eb3de3847 ("slub: separate out sysfs_slab_release() from sysfs_slab_remove()")  
> > 
> > Do you think we should add cc:stable [4.11+]?  
> 
> I think we'd risk more by backporting it through -stable than keeping
> the bug there.  The bug is very difficult to hit.

Famous last words.

>  Writing to a slub
> sysfs file has to race against kmem_cache destruction and AFAICS all
> slub sysfs files are for debugging.

It's not that big of a change. It's simply moving the work to a work
queue. I've done bigger changes than this and backported it to stable
for similar reasons.

All it takes is for it to be hit once in a billion, and that billionth
time could be critical. 

-- Steve

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] slub: make sysfs file removal asynchronous
  2017-06-20 22:22           ` Steven Rostedt
@ 2017-06-20 22:48             ` Tejun Heo
  2017-06-28 14:47               ` Christoph Lameter
  0 siblings, 1 reply; 9+ messages in thread
From: Tejun Heo @ 2017-06-20 22:48 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, LKML,
	Linus Torvalds, Vladimir Davydov, David Rientjes, Joonsoo Kim

Hello,

On Tue, Jun 20, 2017 at 06:22:05PM -0400, Steven Rostedt wrote:
> > I think we'd risk more by backporting it through -stable than keeping
> > the bug there.  The bug is very difficult to hit.
> 
> Famous last words.
>
> >  Writing to a slub
> > sysfs file has to race against kmem_cache destruction and AFAICS all
> > slub sysfs files are for debugging.
> 
> It's not that big of a change. It's simply moving the work to a work
> queue. I've done bigger changes than this and backported it to stable
> for similar reasons.

Some of our -stable backports do backfire.  This isn't a black and
white issue.  We all know even a trivial looking change carries some
level of risk.

> All it takes is for it to be hit once in a billion, and that billionth
> time could be critical. 

And we have to weight that against the possibility of breakage from
the backport, however low it may be, right?  I'm not strongly
convinced either way on this one and AFAICS the slub sysfs files there
are mostly for debugging, so we'd be risking breakage in a way more
common path (kmem_cache destruction) to avoid unlikely deadlock with a
debug facility.  I think -stable backports should be conservative and
justified as breaking things through -stable undermines the whole
thing.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] slub: make sysfs file removal asynchronous
  2017-06-20 22:48             ` Tejun Heo
@ 2017-06-28 14:47               ` Christoph Lameter
  0 siblings, 0 replies; 9+ messages in thread
From: Christoph Lameter @ 2017-06-28 14:47 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Steven Rostedt, Andrew Morton, Pekka Enberg, LKML,
	Linus Torvalds, Vladimir Davydov, David Rientjes, Joonsoo Kim

On Tue, 20 Jun 2017, Tejun Heo wrote:

> And we have to weight that against the possibility of breakage from
> the backport, however low it may be, right?  I'm not strongly
> convinced either way on this one and AFAICS the slub sysfs files there
> are mostly for debugging, so we'd be risking breakage in a way more
> common path (kmem_cache destruction) to avoid unlikely deadlock with a
> debug facility.  I think -stable backports should be conservative and
> justified as breaking things through -stable undermines the whole
> thing.

The sysfs files are mainly used for reporting (the "slabinfo" tool
accesses these files f.e.)

But given the high rate of breakage of sysfs related patches: Lets just
skip stable for now.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-06-28 14:47 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-16 12:55 [LOCKDEP BUG] from slub: separate out sysfs_slab_release() from sysfs_slab_remove() Steven Rostedt
2017-06-19 20:35 ` Tejun Heo
2017-06-19 21:27   ` Steven Rostedt
2017-06-20 20:45     ` [PATCH] slub: make sysfs file removal asynchronous Tejun Heo
2017-06-20 21:58       ` Andrew Morton
2017-06-20 22:00         ` Tejun Heo
2017-06-20 22:22           ` Steven Rostedt
2017-06-20 22:48             ` Tejun Heo
2017-06-28 14:47               ` Christoph Lameter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).