* fs: dcookie: freeing active timer @ 2014-04-24 16:56 Sasha Levin 2014-04-24 17:27 ` Al Viro 0 siblings, 1 reply; 13+ messages in thread From: Sasha Levin @ 2014-04-24 16:56 UTC (permalink / raw) To: linux-fsdevel; +Cc: Al Viro, Jan Kara, Dave Jones, LKML, Thomas Gleixner Hi all, While fuzzing with trinity inside a KVM tools guest running the latest -next kernel I've stumbled on the following: [ 191.835040] WARNING: CPU: 12 PID: 9725 at lib/debugobjects.c:260 debug_print_object+0x8c/0xb0() [ 191.838129] ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x20 [ 191.840545] Modules linked in: [ 191.841297] CPU: 12 PID: 9725 Comm: trinity-c141 Not tainted 3.15.0-rc2-next-20140423-sasha-00018-gc4ff6c4 #408 [ 191.842479] 0000000000000009 ffff88000a8ebc38 ffffffff975271f2 0000000000000001 [ 191.842479] ffff88000a8ebc88 ffff88000a8ebc78 ffffffff9415afcc ffff88000a8a3d98 [ 191.842479] ffff8801b3ad5140 ffffffff98e76200 ffffffff988dc502 ffffffff9b70c870 [ 191.842479] Call Trace: [ 191.842479] dump_stack (lib/dump_stack.c:52) [ 191.842479] warn_slowpath_common (kernel/panic.c:430) [ 191.842479] warn_slowpath_fmt (kernel/panic.c:445) [ 191.842479] debug_print_object (lib/debugobjects.c:262) [ 191.842479] ? __queue_work (kernel/workqueue.c:1452) [ 191.842479] __debug_check_no_obj_freed (lib/debugobjects.c:697) [ 191.842479] debug_check_no_obj_freed (lib/debugobjects.c:726) [ 191.842479] kmem_cache_free (mm/slub.c:2689 mm/slub.c:2717) [ 191.871535] ? kmem_cache_destroy (mm/slab_common.c:363) [ 191.871535] kmem_cache_destroy (mm/slab_common.c:363) [ 191.871535] dcookie_unregister (fs/dcookies.c:302 fs/dcookies.c:343) [ 191.871535] event_buffer_release (arch/x86/oprofile/../../../drivers/oprofile/event_buffer.c:153) [ 191.871535] __fput (fs/file_table.c:217) [ 191.871535] ____fput (fs/file_table.c:253) [ 191.871535] task_work_run (kernel/task_work.c:125 (discriminator 1)) [ 191.871535] do_notify_resume (include/linux/tracehook.h:196 arch/x86/kernel/signal.c:751) [ 191.871535] int_signal (arch/x86/kernel/entry_64.S:807) Thanks, Sasha ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: fs: dcookie: freeing active timer 2014-04-24 16:56 fs: dcookie: freeing active timer Sasha Levin @ 2014-04-24 17:27 ` Al Viro 2014-04-24 17:34 ` Sasha Levin 0 siblings, 1 reply; 13+ messages in thread From: Al Viro @ 2014-04-24 17:27 UTC (permalink / raw) To: Sasha Levin; +Cc: linux-fsdevel, Jan Kara, Dave Jones, LKML, Thomas Gleixner On Thu, Apr 24, 2014 at 12:56:36PM -0400, Sasha Levin wrote: > Hi all, > > While fuzzing with trinity inside a KVM tools guest running the latest -next > kernel I've stumbled on the following: > [ 191.871535] kmem_cache_destroy (mm/slab_common.c:363) > [ 191.871535] dcookie_unregister (fs/dcookies.c:302 fs/dcookies.c:343) So it's dcookie_exit() doing kmem_cache_destroy(dcookie_cache) while some timer is active? Why does that code bother with destroying/creating that sucker dynamically? Is there any point at all? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: fs: dcookie: freeing active timer 2014-04-24 17:27 ` Al Viro @ 2014-04-24 17:34 ` Sasha Levin 2014-04-24 21:55 ` Al Viro 0 siblings, 1 reply; 13+ messages in thread From: Sasha Levin @ 2014-04-24 17:34 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel, Jan Kara, Dave Jones, LKML, Thomas Gleixner On 04/24/2014 01:27 PM, Al Viro wrote: > On Thu, Apr 24, 2014 at 12:56:36PM -0400, Sasha Levin wrote: >> > Hi all, >> > >> > While fuzzing with trinity inside a KVM tools guest running the latest -next >> > kernel I've stumbled on the following: >> > [ 191.871535] kmem_cache_destroy (mm/slab_common.c:363) >> > [ 191.871535] dcookie_unregister (fs/dcookies.c:302 fs/dcookies.c:343) > So it's dcookie_exit() doing kmem_cache_destroy(dcookie_cache) while > some timer is active? > > Why does that code bother with destroying/creating that sucker dynamically? > Is there any point at all? I'm not sure about the dynamic allocation part, but I fear that if we just switch to using static allocations it'll hide the underlying issue that triggered this bug instead of fixing it. Thanks, Sasha ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: fs: dcookie: freeing active timer 2014-04-24 17:34 ` Sasha Levin @ 2014-04-24 21:55 ` Al Viro 2014-04-24 23:49 ` Al Viro 0 siblings, 1 reply; 13+ messages in thread From: Al Viro @ 2014-04-24 21:55 UTC (permalink / raw) To: Sasha Levin; +Cc: linux-fsdevel, Jan Kara, Dave Jones, LKML, Thomas Gleixner On Thu, Apr 24, 2014 at 01:34:14PM -0400, Sasha Levin wrote: > > Why does that code bother with destroying/creating that sucker dynamically? > > Is there any point at all? > > I'm not sure about the dynamic allocation part, but I fear that if we just > switch to using static allocations it'll hide the underlying issue that > triggered this bug instead of fixing it. FWIW, slub.c variant of kmem_cache_destroy() is buggered - struct kobject embedded into struct kmem_cache, its ktype is slab_ktype, which has NULL ->release()... ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: fs: dcookie: freeing active timer 2014-04-24 21:55 ` Al Viro @ 2014-04-24 23:49 ` Al Viro 2014-04-25 3:31 ` Sasha Levin 0 siblings, 1 reply; 13+ messages in thread From: Al Viro @ 2014-04-24 23:49 UTC (permalink / raw) To: Sasha Levin Cc: linux-fsdevel, Jan Kara, Dave Jones, LKML, Thomas Gleixner, Greg Kroah-Hartman, Glauber Costa, Christoph Lameter On Thu, Apr 24, 2014 at 10:55:58PM +0100, Al Viro wrote: > On Thu, Apr 24, 2014 at 01:34:14PM -0400, Sasha Levin wrote: > > > > Why does that code bother with destroying/creating that sucker dynamically? > > > Is there any point at all? > > > > I'm not sure about the dynamic allocation part, but I fear that if we just > > switch to using static allocations it'll hide the underlying issue that > > triggered this bug instead of fixing it. > > FWIW, slub.c variant of kmem_cache_destroy() is buggered - struct kobject > embedded into struct kmem_cache, its ktype is slab_ktype, which has > NULL ->release()... BTW, if your config has CONFIG_DEBUG_KOBJECT_RELEASE, that's exactly where that warning comes from. Got broken by commit b7454a, Author: Glauber Costa <glommer@parallels.com> Date: Fri Oct 19 18:20:25 2012 +0400 mm/sl[au]b: Move slabinfo processing to slab_common.c We *do* need ->release(). Greg and guilty parties Cc'd... ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: fs: dcookie: freeing active timer 2014-04-24 23:49 ` Al Viro @ 2014-04-25 3:31 ` Sasha Levin 2014-04-30 21:48 ` Sasha Levin 0 siblings, 1 reply; 13+ messages in thread From: Sasha Levin @ 2014-04-25 3:31 UTC (permalink / raw) To: Al Viro Cc: linux-fsdevel, Jan Kara, Dave Jones, LKML, Thomas Gleixner, Greg Kroah-Hartman, Glauber Costa, Christoph Lameter On 04/24/2014 07:49 PM, Al Viro wrote: > On Thu, Apr 24, 2014 at 10:55:58PM +0100, Al Viro wrote: >> On Thu, Apr 24, 2014 at 01:34:14PM -0400, Sasha Levin wrote: >> >>>> Why does that code bother with destroying/creating that sucker dynamically? >>>> Is there any point at all? >>> >>> I'm not sure about the dynamic allocation part, but I fear that if we just >>> switch to using static allocations it'll hide the underlying issue that >>> triggered this bug instead of fixing it. >> >> FWIW, slub.c variant of kmem_cache_destroy() is buggered - struct kobject >> embedded into struct kmem_cache, its ktype is slab_ktype, which has >> NULL ->release()... > > BTW, if your config has CONFIG_DEBUG_KOBJECT_RELEASE, that's exactly where > that warning comes from. Got broken by commit b7454a, > Author: Glauber Costa <glommer@parallels.com> > Date: Fri Oct 19 18:20:25 2012 +0400 > > mm/sl[au]b: Move slabinfo processing to slab_common.c > > We *do* need ->release(). Greg and guilty parties Cc'd... We actually had that conversation a long time ago, and Christoph has sent out a patch to fix that (http://www.spinics.net/lists/netdev/msg259431.html). I was assuming that it was merged upstream and went straight to blaming fs/ (and Greg's drivers/usb/ actually) without checking that first. Sorry! Could someone pretty please merge that patch? Specially since Greg acked it? Thanks, Sasha ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: fs: dcookie: freeing active timer 2014-04-25 3:31 ` Sasha Levin @ 2014-04-30 21:48 ` Sasha Levin 2014-04-30 23:19 ` Christoph Lameter 0 siblings, 1 reply; 13+ messages in thread From: Sasha Levin @ 2014-04-30 21:48 UTC (permalink / raw) To: Al Viro Cc: linux-fsdevel, Jan Kara, Dave Jones, LKML, Thomas Gleixner, Greg Kroah-Hartman, Glauber Costa, Christoph Lameter On 04/24/2014 11:31 PM, Sasha Levin wrote: > On 04/24/2014 07:49 PM, Al Viro wrote: >> On Thu, Apr 24, 2014 at 10:55:58PM +0100, Al Viro wrote: >>> On Thu, Apr 24, 2014 at 01:34:14PM -0400, Sasha Levin wrote: >>> >>>>> Why does that code bother with destroying/creating that sucker dynamically? >>>>> Is there any point at all? >>>> >>>> I'm not sure about the dynamic allocation part, but I fear that if we just >>>> switch to using static allocations it'll hide the underlying issue that >>>> triggered this bug instead of fixing it. >>> >>> FWIW, slub.c variant of kmem_cache_destroy() is buggered - struct kobject >>> embedded into struct kmem_cache, its ktype is slab_ktype, which has >>> NULL ->release()... >> >> BTW, if your config has CONFIG_DEBUG_KOBJECT_RELEASE, that's exactly where >> that warning comes from. Got broken by commit b7454a, >> Author: Glauber Costa <glommer@parallels.com> >> Date: Fri Oct 19 18:20:25 2012 +0400 >> >> mm/sl[au]b: Move slabinfo processing to slab_common.c >> >> We *do* need ->release(). Greg and guilty parties Cc'd... > > We actually had that conversation a long time ago, and Christoph has > sent out a patch to fix that (http://www.spinics.net/lists/netdev/msg259431.html). > > I was assuming that it was merged upstream and went straight to blaming > fs/ (and Greg's drivers/usb/ actually) without checking that first. Sorry! > > Could someone pretty please merge that patch? Specially since Greg acked it? Ping? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: fs: dcookie: freeing active timer 2014-04-30 21:48 ` Sasha Levin @ 2014-04-30 23:19 ` Christoph Lameter 2014-05-01 20:10 ` Andrew Morton 0 siblings, 1 reply; 13+ messages in thread From: Christoph Lameter @ 2014-04-30 23:19 UTC (permalink / raw) To: Sasha Levin Cc: Al Viro, linux-fsdevel, Jan Kara, Dave Jones, LKML, Thomas Gleixner, Greg Kroah-Hartman, Glauber Costa, Pekka Enberg, akpm On Wed, 30 Apr 2014, Sasha Levin wrote: > > Could someone pretty please merge that patch? Specially since Greg acked it? > > Ping? Ok please repost. Andrew or Pekka can merge it. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: fs: dcookie: freeing active timer 2014-04-30 23:19 ` Christoph Lameter @ 2014-05-01 20:10 ` Andrew Morton 2014-05-01 20:23 ` Christoph Lameter 2014-05-05 20:44 ` Sasha Levin 0 siblings, 2 replies; 13+ messages in thread From: Andrew Morton @ 2014-05-01 20:10 UTC (permalink / raw) To: Christoph Lameter Cc: Sasha Levin, Al Viro, linux-fsdevel, Jan Kara, Dave Jones, LKML, Thomas Gleixner, Greg Kroah-Hartman, Glauber Costa, Pekka Enberg, akpm On Wed, 30 Apr 2014 18:19:56 -0500 (CDT) Christoph Lameter <cl@linux.com> wrote: > On Wed, 30 Apr 2014, Sasha Levin wrote: > > > > Could someone pretty please merge that patch? Specially since Greg acked it? > > > > Ping? > > Ok please repost. Andrew or Pekka can merge it. OK, I seem to have pieced it all together. The changelog sucked :( I fixed it up a bit but I still don't see why we added that SLAB_SUPPORTS_SYSFS thing. Why do slab and slub differ here? What about slob? A number of rejects had to be fixed. Please check this over carefully. From: Christoph Lameter <cl@linux.com> Subject: slub: use sysfs'es release mechanism for kmem_cache debugobjects warning during netfilter exit: [ 418.311956] ------------[ cut here ]------------ [ 418.312449] WARNING: CPU: 6 PID: 4178 at lib/debugobjects.c:260 debug_print_object+0x8d/0xb0() [ 418.313243] ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x20 [ 418.314038] Modules linked in: [ 418.314038] CPU: 6 PID: 4178 Comm: kworker/u16:2 Tainted: G W 3.11.0-next-20130906-sasha #3984 [ 418.314038] Workqueue: netns cleanup_net [ 418.314038] 0000000000000104 ffff880410371a58 ffffffff841b7815 0000000000000104 [ 418.314038] ffff880410371aa8 ffff880410371a98 ffffffff8112540c ffff880410371a78 [ 418.314038] ffffffff853ea0ab ffff8800bdc68b80 ffffffff85a65c00 ffffffff87676658 [ 418.314038] Call Trace: [ 418.314038] [<ffffffff841b7815>] dump_stack+0x52/0x87 [ 418.320315] [<ffffffff8112540c>] warn_slowpath_common+0x8c/0xc0 [ 418.321040] [<ffffffff811254f6>] warn_slowpath_fmt+0x46/0x50 [ 418.321101] [<ffffffff81a60ded>] debug_print_object+0x8d/0xb0 [ 418.321101] [<ffffffff811444e0>] ? __queue_work+0x390/0x390 [ 418.321101] [<ffffffff81a61605>] __debug_check_no_obj_freed+0xa5/0x220 [ 418.321101] [<ffffffff81249e76>] ? kmem_cache_destroy+0x86/0xe0 [ 418.321101] [<ffffffff81a61795>] debug_check_no_obj_freed+0x15/0x20 [ 418.321101] [<ffffffff812874d7>] kmem_cache_free+0x197/0x340 [ 418.321101] [<ffffffff81249e76>] kmem_cache_destroy+0x86/0xe0 [ 418.321101] [<ffffffff83d5d681>] nf_conntrack_cleanup_net_list+0x131/0x170 [ 418.321101] [<ffffffff83d5ec5d>] nf_conntrack_pernet_exit+0x5d/0x70 [ 418.321101] [<ffffffff83cda1ce>] ops_exit_list+0x5e/0x70 [ 418.321101] [<ffffffff83cda80b>] cleanup_net+0xfb/0x1c0 [ 418.321101] [<ffffffff81145c28>] process_one_work+0x338/0x550 [ 418.321101] [<ffffffff81145b50>] ? process_one_work+0x260/0x550 [ 418.321883] [<ffffffff81147605>] worker_thread+0x215/0x350 [ 418.321883] [<ffffffff811473f0>] ? manage_workers+0x180/0x180 [ 418.321883] [<ffffffff81150887>] kthread+0xe7/0xf0 [ 418.321883] [<ffffffff811507a0>] ? __init_kthread_worker+0x70/0x70 [ 418.321883] [<ffffffff841c7a2c>] ret_from_fork+0x7c/0xb0 [ 418.321883] [<ffffffff811507a0>] ? __init_kthread_worker+0x70/0x70 Also during dkkokie cleanup: [ 191.835040] WARNING: CPU: 12 PID: 9725 at lib/debugobjects.c:260 debug_print_object+0x8c/0xb0() [ 191.838129] ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x20 [ 191.840545] Modules linked in: [ 191.841297] CPU: 12 PID: 9725 Comm: trinity-c141 Not tainted 3.15.0-rc2-next-20140423-sasha-00018-gc4ff6c4 #408 [ 191.842479] 0000000000000009 ffff88000a8ebc38 ffffffff975271f2 0000000000000001 [ 191.842479] ffff88000a8ebc88 ffff88000a8ebc78 ffffffff9415afcc ffff88000a8a3d98 [ 191.842479] ffff8801b3ad5140 ffffffff98e76200 ffffffff988dc502 ffffffff9b70c870 [ 191.842479] Call Trace: [ 191.842479] dump_stack (lib/dump_stack.c:52) [ 191.842479] warn_slowpath_common (kernel/panic.c:430) [ 191.842479] warn_slowpath_fmt (kernel/panic.c:445) [ 191.842479] debug_print_object (lib/debugobjects.c:262) [ 191.842479] ? __queue_work (kernel/workqueue.c:1452) [ 191.842479] __debug_check_no_obj_freed (lib/debugobjects.c:697) [ 191.842479] debug_check_no_obj_freed (lib/debugobjects.c:726) [ 191.842479] kmem_cache_free (mm/slub.c:2689 mm/slub.c:2717) [ 191.871535] ? kmem_cache_destroy (mm/slab_common.c:363) [ 191.871535] kmem_cache_destroy (mm/slab_common.c:363) [ 191.871535] dcookie_unregister (fs/dcookies.c:302 fs/dcookies.c:343) [ 191.871535] event_buffer_release (arch/x86/oprofile/../../../drivers/oprofile/event_buffer.c:153) [ 191.871535] __fput (fs/file_table.c:217) [ 191.871535] ____fput (fs/file_table.c:253) [ 191.871535] task_work_run (kernel/task_work.c:125 (discriminator 1)) [ 191.871535] do_notify_resume (include/linux/tracehook.h:196 arch/x86/kernel/signal.c:751) [ 191.871535] int_signal (arch/x86/kernel/entry_64.S:807) Sysfs has a release mechanism. Use that to release the kmem_cache structure if CONFIG_SYSFS is enabled. Signed-off-by: Christoph Lameter <cl@linux.com> Reported-by: Sasha Levin <sasha.levin@oracle.com> Tested-by: Sasha Levin <sasha.levin@oracle.com> Acked-by: Greg KH <greg@kroah.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pekka Enberg <penberg@kernel.org> Cc: Russell King <rmk@arm.linux.org.uk> Cc: Bart Van Assche <bvanassche@acm.org> Cc: Al Viro <viro@ZenIV.linux.org.uk> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- include/linux/slub_def.h | 4 ++++ mm/slab.h | 2 ++ mm/slab_common.c | 13 +++++++++++-- mm/slub.c | 28 ++++++++-------------------- 4 files changed, 25 insertions(+), 22 deletions(-) diff -puN include/linux/slub_def.h~slub-use-sysfses-release-mechanism-for-kmem_cache include/linux/slub_def.h --- a/include/linux/slub_def.h~slub-use-sysfses-release-mechanism-for-kmem_cache +++ a/include/linux/slub_def.h @@ -101,4 +101,8 @@ struct kmem_cache { struct kmem_cache_node *node[MAX_NUMNODES]; }; +#ifdef CONFIG_SYSFS +#define SLAB_SUPPORTS_SYSFS +#endif + #endif /* _LINUX_SLUB_DEF_H */ diff -puN mm/slab.h~slub-use-sysfses-release-mechanism-for-kmem_cache mm/slab.h --- a/mm/slab.h~slub-use-sysfses-release-mechanism-for-kmem_cache +++ a/mm/slab.h @@ -57,6 +57,7 @@ struct mem_cgroup; struct kmem_cache * __kmem_cache_alias(const char *name, size_t size, size_t align, unsigned long flags, void (*ctor)(void *)); +void sysfs_slab_remove(struct kmem_cache *); #else static inline struct kmem_cache * __kmem_cache_alias(const char *name, size_t size, size_t align, @@ -91,6 +92,7 @@ __kmem_cache_alias(const char *name, siz #define CACHE_CREATE_MASK (SLAB_CORE_FLAGS | SLAB_DEBUG_FLAGS | SLAB_CACHE_FLAGS) int __kmem_cache_shutdown(struct kmem_cache *); +void slab_kmem_cache_release(struct kmem_cache *); struct seq_file; struct file; diff -puN mm/slab_common.c~slub-use-sysfses-release-mechanism-for-kmem_cache mm/slab_common.c --- a/mm/slab_common.c~slub-use-sysfses-release-mechanism-for-kmem_cache +++ a/mm/slab_common.c @@ -323,6 +323,12 @@ static int kmem_cache_destroy_memcg_chil } #endif /* CONFIG_MEMCG_KMEM */ +void slab_kmem_cache_release(struct kmem_cache *s) +{ + kfree(s->name); + kmem_cache_free(kmem_cache, s); +} + void kmem_cache_destroy(struct kmem_cache *s) { get_online_cpus(); @@ -352,8 +358,11 @@ void kmem_cache_destroy(struct kmem_cach rcu_barrier(); memcg_free_cache_params(s); - kfree(s->name); - kmem_cache_free(kmem_cache, s); +#ifdef SLAB_SUPPORTS_SYSFS + sysfs_slab_remove(s); +#else + slab_kmem_cache_release(); +#endif goto out_put_cpus; out_unlock: diff -puN mm/slub.c~slub-use-sysfses-release-mechanism-for-kmem_cache mm/slub.c --- a/mm/slub.c~slub-use-sysfses-release-mechanism-for-kmem_cache +++ a/mm/slub.c @@ -210,7 +210,6 @@ enum track_item { TRACK_ALLOC, TRACK_FRE #ifdef CONFIG_SYSFS static int sysfs_slab_add(struct kmem_cache *); static int sysfs_slab_alias(struct kmem_cache *, const char *); -static void sysfs_slab_remove(struct kmem_cache *); static void memcg_propagate_slab_attrs(struct kmem_cache *s); #else static inline int sysfs_slab_add(struct kmem_cache *s) { return 0; } @@ -3238,24 +3237,7 @@ static inline int kmem_cache_close(struc int __kmem_cache_shutdown(struct kmem_cache *s) { - int rc = kmem_cache_close(s); - - if (!rc) { - /* - * Since slab_attr_store may take the slab_mutex, we should - * release the lock while removing the sysfs entry in order to - * avoid a deadlock. Because this is pretty much the last - * operation we do and the lock will be released shortly after - * that in slab_common.c, we could just move sysfs_slab_remove - * to a later point in common code. We should do that when we - * have a common sysfs framework for all allocators. - */ - mutex_unlock(&slab_mutex); - sysfs_slab_remove(s); - mutex_lock(&slab_mutex); - } - - return rc; + return kmem_cache_close(s); } /******************************************************************** @@ -5122,6 +5104,11 @@ static void memcg_propagate_slab_attrs(s #endif } +static void kmem_cache_release(struct kobject *k) +{ + slab_kmem_cache_release(to_slab(k)); +} + static const struct sysfs_ops slab_sysfs_ops = { .show = slab_attr_show, .store = slab_attr_store, @@ -5129,6 +5116,7 @@ static const struct sysfs_ops slab_sysfs static struct kobj_type slab_ktype = { .sysfs_ops = &slab_sysfs_ops, + .release = kmem_cache_release, }; static int uevent_filter(struct kset *kset, struct kobject *kobj) @@ -5255,7 +5243,7 @@ out_put_kobj: goto out; } -static void sysfs_slab_remove(struct kmem_cache *s) +void sysfs_slab_remove(struct kmem_cache *s) { if (slab_state < FULL) /* _ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: fs: dcookie: freeing active timer 2014-05-01 20:10 ` Andrew Morton @ 2014-05-01 20:23 ` Christoph Lameter 2014-05-05 20:44 ` Sasha Levin 1 sibling, 0 replies; 13+ messages in thread From: Christoph Lameter @ 2014-05-01 20:23 UTC (permalink / raw) To: Andrew Morton Cc: Sasha Levin, Al Viro, linux-fsdevel, Jan Kara, Dave Jones, LKML, Thomas Gleixner, Greg Kroah-Hartman, Glauber Costa, Pekka Enberg, akpm On Thu, 1 May 2014, Andrew Morton wrote: > OK, I seem to have pieced it all together. The changelog sucked :( I > fixed it up a bit but I still don't see why we added that > SLAB_SUPPORTS_SYSFS thing. Why do slab and slub differ here? What > about slob? SLAB currently only supports /proc/slabinfo and not /sys/kernel/slab/*. We talked about adding that and someone was working on it. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: fs: dcookie: freeing active timer 2014-05-01 20:10 ` Andrew Morton 2014-05-01 20:23 ` Christoph Lameter @ 2014-05-05 20:44 ` Sasha Levin 2014-05-05 20:51 ` Andrew Morton 1 sibling, 1 reply; 13+ messages in thread From: Sasha Levin @ 2014-05-05 20:44 UTC (permalink / raw) To: Andrew Morton, Christoph Lameter Cc: Al Viro, linux-fsdevel, Jan Kara, Dave Jones, LKML, Thomas Gleixner, Greg Kroah-Hartman, Glauber Costa, Pekka Enberg, akpm On 05/01/2014 04:10 PM, Andrew Morton wrote: >> On Wed, 30 Apr 2014, Sasha Levin wrote: >> > >>>> > > > Could someone pretty please merge that patch? Specially since Greg acked it? >>> > > >>> > > Ping? >> > >> > Ok please repost. Andrew or Pekka can merge it. > OK, I seem to have pieced it all together. The changelog sucked :( I > fixed it up a bit but I still don't see why we added that > SLAB_SUPPORTS_SYSFS thing. Why do slab and slub differ here? What > about slob? > > A number of rejects had to be fixed. Please check this over carefully. That patch fixed the issues I've been seeing, and I'm not seeing any new issues caused by the patch. Thanks, sasha ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: fs: dcookie: freeing active timer 2014-05-05 20:44 ` Sasha Levin @ 2014-05-05 20:51 ` Andrew Morton 2014-05-06 12:31 ` Thomas Gleixner 0 siblings, 1 reply; 13+ messages in thread From: Andrew Morton @ 2014-05-05 20:51 UTC (permalink / raw) To: Sasha Levin Cc: Christoph Lameter, Al Viro, linux-fsdevel, Jan Kara, Dave Jones, LKML, Thomas Gleixner, Greg Kroah-Hartman, Glauber Costa, Pekka Enberg On Mon, 05 May 2014 16:44:34 -0400 Sasha Levin <sasha.levin@oracle.com> wrote: > On 05/01/2014 04:10 PM, Andrew Morton wrote: > >> On Wed, 30 Apr 2014, Sasha Levin wrote: > >> > > >>>> > > > Could someone pretty please merge that patch? Specially since Greg acked it? > >>> > > > >>> > > Ping? > >> > > >> > Ok please repost. Andrew or Pekka can merge it. > > OK, I seem to have pieced it all together. The changelog sucked :( I > > fixed it up a bit but I still don't see why we added that > > SLAB_SUPPORTS_SYSFS thing. Why do slab and slub differ here? What > > about slob? > > > > A number of rejects had to be fixed. Please check this over carefully. > > That patch fixed the issues I've been seeing, and I'm not seeing any new > issues caused by the patch. OK, thanks. That patch was busted with CONFIG_SYSFS=n and I ended up moving things around quite a bit to fix that. The new version is below. I'll queue this for 3.15 but not for -stable: I don't think we care a lot about debugobjects warnings in stable kernels? From: Christoph Lameter <cl@linux.com> Subject: slub: use sysfs'es release mechanism for kmem_cache debugobjects warning during netfilter exit: [ 418.311956] ------------[ cut here ]------------ [ 418.312449] WARNING: CPU: 6 PID: 4178 at lib/debugobjects.c:260 debug_print_object+0x8d/0xb0() [ 418.313243] ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x20 [ 418.314038] Modules linked in: [ 418.314038] CPU: 6 PID: 4178 Comm: kworker/u16:2 Tainted: G W 3.11.0-next-20130906-sasha #3984 [ 418.314038] Workqueue: netns cleanup_net [ 418.314038] 0000000000000104 ffff880410371a58 ffffffff841b7815 0000000000000104 [ 418.314038] ffff880410371aa8 ffff880410371a98 ffffffff8112540c ffff880410371a78 [ 418.314038] ffffffff853ea0ab ffff8800bdc68b80 ffffffff85a65c00 ffffffff87676658 [ 418.314038] Call Trace: [ 418.314038] [<ffffffff841b7815>] dump_stack+0x52/0x87 [ 418.320315] [<ffffffff8112540c>] warn_slowpath_common+0x8c/0xc0 [ 418.321040] [<ffffffff811254f6>] warn_slowpath_fmt+0x46/0x50 [ 418.321101] [<ffffffff81a60ded>] debug_print_object+0x8d/0xb0 [ 418.321101] [<ffffffff811444e0>] ? __queue_work+0x390/0x390 [ 418.321101] [<ffffffff81a61605>] __debug_check_no_obj_freed+0xa5/0x220 [ 418.321101] [<ffffffff81249e76>] ? kmem_cache_destroy+0x86/0xe0 [ 418.321101] [<ffffffff81a61795>] debug_check_no_obj_freed+0x15/0x20 [ 418.321101] [<ffffffff812874d7>] kmem_cache_free+0x197/0x340 [ 418.321101] [<ffffffff81249e76>] kmem_cache_destroy+0x86/0xe0 [ 418.321101] [<ffffffff83d5d681>] nf_conntrack_cleanup_net_list+0x131/0x170 [ 418.321101] [<ffffffff83d5ec5d>] nf_conntrack_pernet_exit+0x5d/0x70 [ 418.321101] [<ffffffff83cda1ce>] ops_exit_list+0x5e/0x70 [ 418.321101] [<ffffffff83cda80b>] cleanup_net+0xfb/0x1c0 [ 418.321101] [<ffffffff81145c28>] process_one_work+0x338/0x550 [ 418.321101] [<ffffffff81145b50>] ? process_one_work+0x260/0x550 [ 418.321883] [<ffffffff81147605>] worker_thread+0x215/0x350 [ 418.321883] [<ffffffff811473f0>] ? manage_workers+0x180/0x180 [ 418.321883] [<ffffffff81150887>] kthread+0xe7/0xf0 [ 418.321883] [<ffffffff811507a0>] ? __init_kthread_worker+0x70/0x70 [ 418.321883] [<ffffffff841c7a2c>] ret_from_fork+0x7c/0xb0 [ 418.321883] [<ffffffff811507a0>] ? __init_kthread_worker+0x70/0x70 Also during dcookie cleanup: [ 191.835040] WARNING: CPU: 12 PID: 9725 at lib/debugobjects.c:260 debug_print_object+0x8c/0xb0() [ 191.838129] ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x20 [ 191.840545] Modules linked in: [ 191.841297] CPU: 12 PID: 9725 Comm: trinity-c141 Not tainted 3.15.0-rc2-next-20140423-sasha-00018-gc4ff6c4 #408 [ 191.842479] 0000000000000009 ffff88000a8ebc38 ffffffff975271f2 0000000000000001 [ 191.842479] ffff88000a8ebc88 ffff88000a8ebc78 ffffffff9415afcc ffff88000a8a3d98 [ 191.842479] ffff8801b3ad5140 ffffffff98e76200 ffffffff988dc502 ffffffff9b70c870 [ 191.842479] Call Trace: [ 191.842479] dump_stack (lib/dump_stack.c:52) [ 191.842479] warn_slowpath_common (kernel/panic.c:430) [ 191.842479] warn_slowpath_fmt (kernel/panic.c:445) [ 191.842479] debug_print_object (lib/debugobjects.c:262) [ 191.842479] ? __queue_work (kernel/workqueue.c:1452) [ 191.842479] __debug_check_no_obj_freed (lib/debugobjects.c:697) [ 191.842479] debug_check_no_obj_freed (lib/debugobjects.c:726) [ 191.842479] kmem_cache_free (mm/slub.c:2689 mm/slub.c:2717) [ 191.871535] ? kmem_cache_destroy (mm/slab_common.c:363) [ 191.871535] kmem_cache_destroy (mm/slab_common.c:363) [ 191.871535] dcookie_unregister (fs/dcookies.c:302 fs/dcookies.c:343) [ 191.871535] event_buffer_release (arch/x86/oprofile/../../../drivers/oprofile/event_buffer.c:153) [ 191.871535] __fput (fs/file_table.c:217) [ 191.871535] ____fput (fs/file_table.c:253) [ 191.871535] task_work_run (kernel/task_work.c:125 (discriminator 1)) [ 191.871535] do_notify_resume (include/linux/tracehook.h:196 arch/x86/kernel/signal.c:751) [ 191.871535] int_signal (arch/x86/kernel/entry_64.S:807) Sysfs has a release mechanism. Use that to release the kmem_cache structure if CONFIG_SYSFS is enabled. Only slub is changed - slab currently only supports /proc/slabinfo and not /sys/kernel/slab/*. We talked about adding that and someone was working on it. [akpm@linux-foundation.org: fix CONFIG_SYSFS=n build] [akpm@linux-foundation.org: fix CONFIG_SYSFS=n build even more] Signed-off-by: Christoph Lameter <cl@linux.com> Reported-by: Sasha Levin <sasha.levin@oracle.com> Tested-by: Sasha Levin <sasha.levin@oracle.com> Acked-by: Greg KH <greg@kroah.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Pekka Enberg <penberg@kernel.org> Cc: Russell King <rmk@arm.linux.org.uk> Cc: Bart Van Assche <bvanassche@acm.org> Cc: Al Viro <viro@ZenIV.linux.org.uk> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- include/linux/slub_def.h | 9 +++++++++ mm/slab.h | 1 + mm/slab_common.c | 13 +++++++++++-- mm/slub.c | 30 ++++++++---------------------- 4 files changed, 29 insertions(+), 24 deletions(-) diff -puN include/linux/slub_def.h~slub-use-sysfses-release-mechanism-for-kmem_cache include/linux/slub_def.h --- a/include/linux/slub_def.h~slub-use-sysfses-release-mechanism-for-kmem_cache +++ a/include/linux/slub_def.h @@ -101,4 +101,13 @@ struct kmem_cache { struct kmem_cache_node *node[MAX_NUMNODES]; }; +#ifdef CONFIG_SYSFS +#define SLAB_SUPPORTS_SYSFS +void sysfs_slab_remove(struct kmem_cache *); +#else +static inline void sysfs_slab_remove(struct kmem_cache *s) +{ +} +#endif + #endif /* _LINUX_SLUB_DEF_H */ diff -puN mm/slab.h~slub-use-sysfses-release-mechanism-for-kmem_cache mm/slab.h --- a/mm/slab.h~slub-use-sysfses-release-mechanism-for-kmem_cache +++ a/mm/slab.h @@ -91,6 +91,7 @@ __kmem_cache_alias(const char *name, siz #define CACHE_CREATE_MASK (SLAB_CORE_FLAGS | SLAB_DEBUG_FLAGS | SLAB_CACHE_FLAGS) int __kmem_cache_shutdown(struct kmem_cache *); +void slab_kmem_cache_release(struct kmem_cache *); struct seq_file; struct file; diff -puN mm/slab_common.c~slub-use-sysfses-release-mechanism-for-kmem_cache mm/slab_common.c --- a/mm/slab_common.c~slub-use-sysfses-release-mechanism-for-kmem_cache +++ a/mm/slab_common.c @@ -323,6 +323,12 @@ static int kmem_cache_destroy_memcg_chil } #endif /* CONFIG_MEMCG_KMEM */ +void slab_kmem_cache_release(struct kmem_cache *s) +{ + kfree(s->name); + kmem_cache_free(kmem_cache, s); +} + void kmem_cache_destroy(struct kmem_cache *s) { get_online_cpus(); @@ -352,8 +358,11 @@ void kmem_cache_destroy(struct kmem_cach rcu_barrier(); memcg_free_cache_params(s); - kfree(s->name); - kmem_cache_free(kmem_cache, s); +#ifdef SLAB_SUPPORTS_SYSFS + sysfs_slab_remove(s); +#else + slab_kmem_cache_release(s); +#endif goto out_put_cpus; out_unlock: diff -puN mm/slub.c~slub-use-sysfses-release-mechanism-for-kmem_cache mm/slub.c --- a/mm/slub.c~slub-use-sysfses-release-mechanism-for-kmem_cache +++ a/mm/slub.c @@ -210,14 +210,11 @@ enum track_item { TRACK_ALLOC, TRACK_FRE #ifdef CONFIG_SYSFS static int sysfs_slab_add(struct kmem_cache *); static int sysfs_slab_alias(struct kmem_cache *, const char *); -static void sysfs_slab_remove(struct kmem_cache *); static void memcg_propagate_slab_attrs(struct kmem_cache *s); #else static inline int sysfs_slab_add(struct kmem_cache *s) { return 0; } static inline int sysfs_slab_alias(struct kmem_cache *s, const char *p) { return 0; } -static inline void sysfs_slab_remove(struct kmem_cache *s) { } - static inline void memcg_propagate_slab_attrs(struct kmem_cache *s) { } #endif @@ -3238,24 +3235,7 @@ static inline int kmem_cache_close(struc int __kmem_cache_shutdown(struct kmem_cache *s) { - int rc = kmem_cache_close(s); - - if (!rc) { - /* - * Since slab_attr_store may take the slab_mutex, we should - * release the lock while removing the sysfs entry in order to - * avoid a deadlock. Because this is pretty much the last - * operation we do and the lock will be released shortly after - * that in slab_common.c, we could just move sysfs_slab_remove - * to a later point in common code. We should do that when we - * have a common sysfs framework for all allocators. - */ - mutex_unlock(&slab_mutex); - sysfs_slab_remove(s); - mutex_lock(&slab_mutex); - } - - return rc; + return kmem_cache_close(s); } /******************************************************************** @@ -5122,6 +5102,11 @@ static void memcg_propagate_slab_attrs(s #endif } +static void kmem_cache_release(struct kobject *k) +{ + slab_kmem_cache_release(to_slab(k)); +} + static const struct sysfs_ops slab_sysfs_ops = { .show = slab_attr_show, .store = slab_attr_store, @@ -5129,6 +5114,7 @@ static const struct sysfs_ops slab_sysfs static struct kobj_type slab_ktype = { .sysfs_ops = &slab_sysfs_ops, + .release = kmem_cache_release, }; static int uevent_filter(struct kset *kset, struct kobject *kobj) @@ -5255,7 +5241,7 @@ out_put_kobj: goto out; } -static void sysfs_slab_remove(struct kmem_cache *s) +void sysfs_slab_remove(struct kmem_cache *s) { if (slab_state < FULL) /* _ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: fs: dcookie: freeing active timer 2014-05-05 20:51 ` Andrew Morton @ 2014-05-06 12:31 ` Thomas Gleixner 0 siblings, 0 replies; 13+ messages in thread From: Thomas Gleixner @ 2014-05-06 12:31 UTC (permalink / raw) To: Andrew Morton Cc: Sasha Levin, Christoph Lameter, Al Viro, linux-fsdevel, Jan Kara, Dave Jones, LKML, Greg Kroah-Hartman, Glauber Costa, Pekka Enberg On Mon, 5 May 2014, Andrew Morton wrote: > On Mon, 05 May 2014 16:44:34 -0400 Sasha Levin <sasha.levin@oracle.com> wrote: > > > On 05/01/2014 04:10 PM, Andrew Morton wrote: > > >> On Wed, 30 Apr 2014, Sasha Levin wrote: > > >> > > > >>>> > > > Could someone pretty please merge that patch? Specially since Greg acked it? > > >>> > > > > >>> > > Ping? > > >> > > > >> > Ok please repost. Andrew or Pekka can merge it. > > > OK, I seem to have pieced it all together. The changelog sucked :( I > > > fixed it up a bit but I still don't see why we added that > > > SLAB_SUPPORTS_SYSFS thing. Why do slab and slub differ here? What > > > about slob? > > > > > > A number of rejects had to be fixed. Please check this over carefully. > > > > That patch fixed the issues I've been seeing, and I'm not seeing any new > > issues caused by the patch. > > OK, thanks. That patch was busted with CONFIG_SYSFS=n and I ended up > moving things around quite a bit to fix that. The new version is > below. > > I'll queue this for 3.15 but not for -stable: I don't think we care a > lot about debugobjects warnings in stable kernels? If debugobjects warns you about freeing an active timer, then you better care. With debugobjects disabled the kernel will just crash in the timer softirq or explode somewhere else. Thanks, tglx ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-05-06 12:31 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-24 16:56 fs: dcookie: freeing active timer Sasha Levin 2014-04-24 17:27 ` Al Viro 2014-04-24 17:34 ` Sasha Levin 2014-04-24 21:55 ` Al Viro 2014-04-24 23:49 ` Al Viro 2014-04-25 3:31 ` Sasha Levin 2014-04-30 21:48 ` Sasha Levin 2014-04-30 23:19 ` Christoph Lameter 2014-05-01 20:10 ` Andrew Morton 2014-05-01 20:23 ` Christoph Lameter 2014-05-05 20:44 ` Sasha Levin 2014-05-05 20:51 ` Andrew Morton 2014-05-06 12:31 ` Thomas Gleixner
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).