netfilter: active obj WARN when cleaning up

netfilter: active obj WARN when cleaning up

[Sasha Levin]

Hi all,

While fuzzing with trinity inside a KVM tools guest, running latest -next kernel, I've
stumbled on the following:

[  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
[  418.321883] ---[ end trace c974221542b61d0d ]---


Thanks,
Sasha

Re: netfilter: active obj WARN when cleaning up

[Sasha Levin]

Ping? I still see this warning.

On 09/07/2013 09:10 AM, Sasha Levin wrote:
> Hi all,
>
> While fuzzing with trinity inside a KVM tools guest, running latest -next kernel, I've
> stumbled on the following:
>
> [  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
> [  418.321883] ---[ end trace c974221542b61d0d ]---
>
>
> Thanks,
> Sasha

Re: netfilter: active obj WARN when cleaning up

[Pablo Neira Ayuso]

On Tue, Nov 26, 2013 at 02:11:57PM -0500, Sasha Levin wrote:
> Ping? I still see this warning.

Did your test include patch 0c3c6c00c6?

> On 09/07/2013 09:10 AM, Sasha Levin wrote:
> >Hi all,
> >
> >While fuzzing with trinity inside a KVM tools guest, running latest -next kernel, I've
> >stumbled on the following:
> >
> >[  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
> >[  418.321883] ---[ end trace c974221542b61d0d ]---
> >
> >
> >Thanks,
> >Sasha
> 

Re: netfilter: active obj WARN when cleaning up

[Sasha Levin]

On 11/26/2013 06:07 PM, Pablo Neira Ayuso wrote:
> On Tue, Nov 26, 2013 at 02:11:57PM -0500, Sasha Levin wrote:
>> >Ping? I still see this warning.
> Did your test include patch 0c3c6c00c6?

Yeah, it reproduces with today's -next tree, which includes that commit.


Thanks,
Sasha

Re: netfilter: active obj WARN when cleaning up

[Thomas Gleixner]

On Wed, 27 Nov 2013, Pablo Neira Ayuso wrote:

> On Tue, Nov 26, 2013 at 02:11:57PM -0500, Sasha Levin wrote:
> > Ping? I still see this warning.
> 
> Did your test include patch 0c3c6c00c6?

And how is that patch supposed to help?
 
> > >[  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.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

The debug code detects an active timer, which itself is part of a
delayed work struct. The call comes from kmem_cache_destroy().

         kmem_cache_free(kmem_cache, s);

So debug object says: s contains an active timer. s is the kmem_cache
which is destroyed from nf_conntrack_cleanup_net_list.

Now struct kmem_cache has in case of SLUB:

    struct kobject kobj;    /* For sysfs */

and struct kobject has:

#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
    struct delayed_work     release;
#endif

So this is the thing you want to look at:

commit c817a67ec (kobject: delayed kobject release: help find buggy
drivers) added that delayed work thing.

I fear that does not work for kobjects which are embedded into
something else.

Handing off to rmk, mm and kobject folks.

Thanks,

	tglx

Re: netfilter: active obj WARN when cleaning up

[Russell King - ARM Linux]

On Wed, Nov 27, 2013 at 11:45:17AM +0100, Thomas Gleixner wrote:
> On Wed, 27 Nov 2013, Pablo Neira Ayuso wrote:
> 
> > On Tue, Nov 26, 2013 at 02:11:57PM -0500, Sasha Levin wrote:
> > > Ping? I still see this warning.
> > 
> > Did your test include patch 0c3c6c00c6?
> 
> And how is that patch supposed to help?
>  
> > > >[  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.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
> 
> The debug code detects an active timer, which itself is part of a
> delayed work struct. The call comes from kmem_cache_destroy().
> 
>          kmem_cache_free(kmem_cache, s);
> 
> So debug object says: s contains an active timer. s is the kmem_cache
> which is destroyed from nf_conntrack_cleanup_net_list.
> 
> Now struct kmem_cache has in case of SLUB:
> 
>     struct kobject kobj;    /* For sysfs */
> 
> and struct kobject has:
> 
> #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
>     struct delayed_work     release;
> #endif
> 
> So this is the thing you want to look at:
> 
> commit c817a67ec (kobject: delayed kobject release: help find buggy
> drivers) added that delayed work thing.
> 
> I fear that does not work for kobjects which are embedded into
> something else.

No, kobjects embedded into something else have their lifetime determined
by the embedded kobject.  That's rule #1 of kobjects - or rather reference
counted objects.

The point at which the kobject gets destructed is when the release function
is called.  If it is destructed before that time, that's a violation of
the reference counted nature of kobjects, and that's what the delay on
releasing is designed to catch.

It's designed to catch code which does this exact path:

	put(obj)
	free(obj)

rather than code which does it the right way:

	put(obj)
		-> refcount becomes 0
			-> release function gets called
				->free(obj)

The former is unsafe because obj may have other references.

Re: netfilter: active obj WARN when cleaning up

[Thomas Gleixner]

On Wed, 27 Nov 2013, Russell King - ARM Linux wrote:
> On Wed, Nov 27, 2013 at 11:45:17AM +0100, Thomas Gleixner wrote:
> > On Wed, 27 Nov 2013, Pablo Neira Ayuso wrote:
> > 
> > > On Tue, Nov 26, 2013 at 02:11:57PM -0500, Sasha Levin wrote:
> > > > Ping? I still see this warning.
> > > 
> > > Did your test include patch 0c3c6c00c6?
> > 
> > And how is that patch supposed to help?
> >  
> > > > >[  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.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
> > 
> > The debug code detects an active timer, which itself is part of a
> > delayed work struct. The call comes from kmem_cache_destroy().
> > 
> >          kmem_cache_free(kmem_cache, s);
> > 
> > So debug object says: s contains an active timer. s is the kmem_cache
> > which is destroyed from nf_conntrack_cleanup_net_list.
> > 
> > Now struct kmem_cache has in case of SLUB:
> > 
> >     struct kobject kobj;    /* For sysfs */
> > 
> > and struct kobject has:
> > 
> > #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> >     struct delayed_work     release;
> > #endif
> > 
> > So this is the thing you want to look at:
> > 
> > commit c817a67ec (kobject: delayed kobject release: help find buggy
> > drivers) added that delayed work thing.
> > 
> > I fear that does not work for kobjects which are embedded into
> > something else.
> 
> No, kobjects embedded into something else have their lifetime determined
> by the embedded kobject.  That's rule #1 of kobjects - or rather reference
> counted objects.
> 
> The point at which the kobject gets destructed is when the release function
> is called.  If it is destructed before that time, that's a violation of
> the reference counted nature of kobjects, and that's what the delay on
> releasing is designed to catch.
> 
> It's designed to catch code which does this exact path:
> 
> 	put(obj)
> 	free(obj)
> 
> rather than code which does it the right way:
> 
> 	put(obj)
> 		-> refcount becomes 0
> 			-> release function gets called
> 				->free(obj)
> 
> The former is unsafe because obj may have other references.

Though the kobject is the only thing which has a delayed work embedded
inside struct kmem_cache. And the debug object splat points at the
kmem_cache_free() of the struct kmem_cache itself. That's why I
assumed the wreckage around that place. And indeed:

kmem_cache_destroy(s)
    __kmem_cache_shutdown(s)
      sysfs_slab_remove(s)
        ....
	kobject_put(&s->kobj)
           kref_put(&kobj->kref, kobject_release);
	     kobject_release(kref)
    	       #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
	         schedule_delayed_work(&kobj->release)
	       #else
	        kobject_cleanup(kobj)
	       #endif

So in the CONFIG_DEBUG_KOBJECT_RELEASE=y case, schedule_delayed_work()
_IS_ called which arms the timer. debugobjects catches the attempt to
free struct kmem_cache which contains the armed timer.

So much for rule #1

Thanks,

	tglx

Re: netfilter: active obj WARN when cleaning up

[Russell King - ARM Linux]

On Wed, Nov 27, 2013 at 02:29:41PM +0100, Thomas Gleixner wrote:
> Though the kobject is the only thing which has a delayed work embedded
> inside struct kmem_cache. And the debug object splat points at the
> kmem_cache_free() of the struct kmem_cache itself. That's why I
> assumed the wreckage around that place. And indeed:
> 
> kmem_cache_destroy(s)
>     __kmem_cache_shutdown(s)
>       sysfs_slab_remove(s)
>         ....
> 	kobject_put(&s->kobj)
>            kref_put(&kobj->kref, kobject_release);
> 	     kobject_release(kref)
>     	       #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> 	         schedule_delayed_work(&kobj->release)
> 	       #else
> 	        kobject_cleanup(kobj)
> 	       #endif
> 
> So in the CONFIG_DEBUG_KOBJECT_RELEASE=y case, schedule_delayed_work()
> _IS_ called which arms the timer. debugobjects catches the attempt to
> free struct kmem_cache which contains the armed timer.

You fail to show where the free is in the above path.

Re: netfilter: active obj WARN when cleaning up

[Russell King - ARM Linux]

On Wed, Nov 27, 2013 at 01:32:31PM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 27, 2013 at 02:29:41PM +0100, Thomas Gleixner wrote:
> > Though the kobject is the only thing which has a delayed work embedded
> > inside struct kmem_cache. And the debug object splat points at the
> > kmem_cache_free() of the struct kmem_cache itself. That's why I
> > assumed the wreckage around that place. And indeed:
> > 
> > kmem_cache_destroy(s)
> >     __kmem_cache_shutdown(s)
> >       sysfs_slab_remove(s)
> >         ....
> > 	kobject_put(&s->kobj)
> >            kref_put(&kobj->kref, kobject_release);
> > 	     kobject_release(kref)
> >     	       #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> > 	         schedule_delayed_work(&kobj->release)
> > 	       #else
> > 	        kobject_cleanup(kobj)
> > 	       #endif
> > 
> > So in the CONFIG_DEBUG_KOBJECT_RELEASE=y case, schedule_delayed_work()
> > _IS_ called which arms the timer. debugobjects catches the attempt to
> > free struct kmem_cache which contains the armed timer.
> 
> You fail to show where the free is in the above path.

Right, there's a kmem_cache_free(kmem_cache, s); by the slob code
after the above sequence, which is The Bug(tm).

As I said, a kobject has its own lifetime.  If you embed that into
another structure, that structure inherits the lifetime of the kobject,
which is from the point at which it's created to the point at which the
kobject's release function is called.

So no, the code here is buggy.  The kobject debugging has yet again
found a violation of the kobject lifetime rules.  slub needs fixing.

Re: netfilter: active obj WARN when cleaning up

[Thomas Gleixner]

On Wed, 27 Nov 2013, Russell King - ARM Linux wrote:

> On Wed, Nov 27, 2013 at 02:29:41PM +0100, Thomas Gleixner wrote:
> > Though the kobject is the only thing which has a delayed work embedded
> > inside struct kmem_cache. And the debug object splat points at the
> > kmem_cache_free() of the struct kmem_cache itself. That's why I
> > assumed the wreckage around that place. And indeed:
> > 
> > kmem_cache_destroy(s)
> >     __kmem_cache_shutdown(s) 

  	{

> >       sysfs_slab_remove(s)
> >         ....
> > 	kobject_put(&s->kobj)
> >            kref_put(&kobj->kref, kobject_release);
> > 	         kobject_release(kref)
> >     	       #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> > 	         schedule_delayed_work(&kobj->release)
> > 	       #else
> > 	        kobject_cleanup(kobj)
> > 	       #endif

  	}

	kmem_cache_free(s);

> > 
> > So in the CONFIG_DEBUG_KOBJECT_RELEASE=y case, schedule_delayed_work()
> > _IS_ called which arms the timer. debugobjects catches the attempt to
> > free struct kmem_cache which contains the armed timer.
> 
> You fail to show where the free is in the above path.

So, yes. it's the issue you are trying to catch, but that code in
question made already sure, that there are no references held, because
it detached itself from sysfs before calling kobject_put().

Thanks,

	tglx

Re: netfilter: active obj WARN when cleaning up

[Thomas Gleixner]

On Wed, 27 Nov 2013, Russell King - ARM Linux wrote:

> On Wed, Nov 27, 2013 at 01:32:31PM +0000, Russell King - ARM Linux wrote:
> > On Wed, Nov 27, 2013 at 02:29:41PM +0100, Thomas Gleixner wrote:
> > > Though the kobject is the only thing which has a delayed work embedded
> > > inside struct kmem_cache. And the debug object splat points at the
> > > kmem_cache_free() of the struct kmem_cache itself. That's why I
> > > assumed the wreckage around that place. And indeed:
> > > 
> > > kmem_cache_destroy(s)
> > >     __kmem_cache_shutdown(s)
> > >       sysfs_slab_remove(s)
> > >         ....
> > > 	kobject_put(&s->kobj)
> > >            kref_put(&kobj->kref, kobject_release);
> > > 	     kobject_release(kref)
> > >     	       #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> > > 	         schedule_delayed_work(&kobj->release)
> > > 	       #else
> > > 	        kobject_cleanup(kobj)
> > > 	       #endif
> > > 
> > > So in the CONFIG_DEBUG_KOBJECT_RELEASE=y case, schedule_delayed_work()
> > > _IS_ called which arms the timer. debugobjects catches the attempt to
> > > free struct kmem_cache which contains the armed timer.
> > 
> > You fail to show where the free is in the above path.
> 
> Right, there's a kmem_cache_free(kmem_cache, s); by the slob code
> after the above sequence, which is The Bug(tm).
> 
> As I said, a kobject has its own lifetime.  If you embed that into
> another structure, that structure inherits the lifetime of the kobject,
> which is from the point at which it's created to the point at which the
> kobject's release function is called.
> 
> So no, the code here is buggy.  The kobject debugging has yet again
> found a violation of the kobject lifetime rules.  slub needs fixing.

I leave that discussion to you, greg and the slub folks.

/me prepares deck chair, drinks and popcorn 

Re: netfilter: active obj WARN when cleaning up

[Greg KH]

On Wed, Nov 27, 2013 at 02:44:58PM +0100, Thomas Gleixner wrote:
> On Wed, 27 Nov 2013, Russell King - ARM Linux wrote:
> 
> > On Wed, Nov 27, 2013 at 01:32:31PM +0000, Russell King - ARM Linux wrote:
> > > On Wed, Nov 27, 2013 at 02:29:41PM +0100, Thomas Gleixner wrote:
> > > > Though the kobject is the only thing which has a delayed work embedded
> > > > inside struct kmem_cache. And the debug object splat points at the
> > > > kmem_cache_free() of the struct kmem_cache itself. That's why I
> > > > assumed the wreckage around that place. And indeed:
> > > > 
> > > > kmem_cache_destroy(s)
> > > >     __kmem_cache_shutdown(s)
> > > >       sysfs_slab_remove(s)
> > > >         ....
> > > > 	kobject_put(&s->kobj)
> > > >            kref_put(&kobj->kref, kobject_release);
> > > > 	     kobject_release(kref)
> > > >     	       #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> > > > 	         schedule_delayed_work(&kobj->release)
> > > > 	       #else
> > > > 	        kobject_cleanup(kobj)
> > > > 	       #endif
> > > > 
> > > > So in the CONFIG_DEBUG_KOBJECT_RELEASE=y case, schedule_delayed_work()
> > > > _IS_ called which arms the timer. debugobjects catches the attempt to
> > > > free struct kmem_cache which contains the armed timer.
> > > 
> > > You fail to show where the free is in the above path.
> > 
> > Right, there's a kmem_cache_free(kmem_cache, s); by the slob code
> > after the above sequence, which is The Bug(tm).
> > 
> > As I said, a kobject has its own lifetime.  If you embed that into
> > another structure, that structure inherits the lifetime of the kobject,
> > which is from the point at which it's created to the point at which the
> > kobject's release function is called.
> > 
> > So no, the code here is buggy.  The kobject debugging has yet again
> > found a violation of the kobject lifetime rules.  slub needs fixing.
> 
> I leave that discussion to you, greg and the slub folks.

/me grabs some popcorn from tglx

It's really not that much of a discussion, Documentation/kobject.txt has
said this for years, it's as if no one even reads documentation
anymore...

If you embed a kobject into a structure, you have to use the kobject for
the reference counting of the structure, otherwise it's a bug.  If you
don't want to use a kobject to reference count the structure, don't
embed it into it, use a pointer.

Are "slabs" never freed in the slub allocator?  Surely someone should
have seen the huge "this kobject doesn't have a release function" error
message that the kernel should have spit out for it?

Just make the kobject "dynamic" instead of embedded in struct kmem_cache
and all will be fine.  I can't believe this code has been broken for
this long.

thanks,

greg k-h

Re: netfilter: active obj WARN when cleaning up

[Christoph Lameter]

On Wed, 27 Nov 2013, Greg KH wrote:

> Just make the kobject "dynamic" instead of embedded in struct kmem_cache
> and all will be fine.  I can't believe this code has been broken for
> this long.

The slub code is was designed to use an embedded structure since we
only get the kobj  pointer passed to us from sysfs. If kobj is not
embedded then how can we get from the sysfs object to the kmem_cache
structure from the sysfs callbacks? Sysfs was designed to have embedded
objects as far as I can recall.

Re: netfilter: active obj WARN when cleaning up

[Greg KH]

On Mon, Dec 02, 2013 at 04:33:20PM +0000, Christoph Lameter wrote:
> On Wed, 27 Nov 2013, Greg KH wrote:
> 
> > Just make the kobject "dynamic" instead of embedded in struct kmem_cache
> > and all will be fine.  I can't believe this code has been broken for
> > this long.
> 
> The slub code is was designed to use an embedded structure since we
> only get the kobj  pointer passed to us from sysfs. If kobj is not
> embedded then how can we get from the sysfs object to the kmem_cache
> structure from the sysfs callbacks? Sysfs was designed to have embedded
> objects as far as I can recall.

Yes, it's designed to have embedded objects, so then use it that way and
clean up the structure when the kobject goes away.  Don't use a
different reference count for your structure than the one in the kobject
and think that all will be fine.

greg k-h

Re: netfilter: active obj WARN when cleaning up

[Christoph Lameter]

On Mon, 2 Dec 2013, Greg KH wrote:

> On Mon, Dec 02, 2013 at 04:33:20PM +0000, Christoph Lameter wrote:
> > On Wed, 27 Nov 2013, Greg KH wrote:
> >
> > > Just make the kobject "dynamic" instead of embedded in struct kmem_cache
> > > and all will be fine.  I can't believe this code has been broken for
> > > this long.
> >
> > The slub code is was designed to use an embedded structure since we
> > only get the kobj  pointer passed to us from sysfs. If kobj is not
> > embedded then how can we get from the sysfs object to the kmem_cache
> > structure from the sysfs callbacks? Sysfs was designed to have embedded
> > objects as far as I can recall.
>
> Yes, it's designed to have embedded objects, so then use it that way and
> clean up the structure when the kobject goes away.  Don't use a
> different reference count for your structure than the one in the kobject
> and think that all will be fine.

We need our own reference count. So we just have to defer the
release of the kmem_cache struct until the ->release callback is
triggered. The put of the embedded kobject must be the last action on the
kmem_cache  structure which will then trigger release and that will
trigger the kmem_cache_free().

Re: netfilter: active obj WARN when cleaning up

[Greg KH]

On Mon, Dec 02, 2013 at 05:18:16PM +0000, Christoph Lameter wrote:
> On Mon, 2 Dec 2013, Greg KH wrote:
> 
> > On Mon, Dec 02, 2013 at 04:33:20PM +0000, Christoph Lameter wrote:
> > > On Wed, 27 Nov 2013, Greg KH wrote:
> > >
> > > > Just make the kobject "dynamic" instead of embedded in struct kmem_cache
> > > > and all will be fine.  I can't believe this code has been broken for
> > > > this long.
> > >
> > > The slub code is was designed to use an embedded structure since we
> > > only get the kobj  pointer passed to us from sysfs. If kobj is not
> > > embedded then how can we get from the sysfs object to the kmem_cache
> > > structure from the sysfs callbacks? Sysfs was designed to have embedded
> > > objects as far as I can recall.
> >
> > Yes, it's designed to have embedded objects, so then use it that way and
> > clean up the structure when the kobject goes away.  Don't use a
> > different reference count for your structure than the one in the kobject
> > and think that all will be fine.
> 
> We need our own reference count. So we just have to defer the
> release of the kmem_cache struct until the ->release callback is
> triggered. The put of the embedded kobject must be the last action on the
> kmem_cache  structure which will then trigger release and that will
> trigger the kmem_cache_free().
> 

Ok, that sounds reasonable, or you can just create a "tiny" structure
for the kobject that has a pointer back to your kmem_cache structure
that you can then reference from the show/store functions.  Either is
fine with me.

thanks,

greg k-h

Re: netfilter: active obj WARN when cleaning up

[Christoph Lameter]

On Mon, 2 Dec 2013, Greg KH wrote:

> >
> > We need our own reference count. So we just have to defer the
> > release of the kmem_cache struct until the ->release callback is
> > triggered. The put of the embedded kobject must be the last action on the
> > kmem_cache  structure which will then trigger release and that will
> > trigger the kmem_cache_free().
> >
>
> Ok, that sounds reasonable, or you can just create a "tiny" structure
> for the kobject that has a pointer back to your kmem_cache structure
> that you can then reference from the show/store functions.  Either is
> fine with me.

Problem is that the release field is only available if
CONFIG_DEBUG_KOBJECT_RELEASE is enabled. Without the callback I cannot
tell when it is legit to release the kobject structure unless I keep
scanning it once in awhile.

Re: netfilter: active obj WARN when cleaning up

[Greg KH]

On Mon, Dec 02, 2013 at 07:00:23PM +0000, Christoph Lameter wrote:
> On Mon, 2 Dec 2013, Greg KH wrote:
> 
> > >
> > > We need our own reference count. So we just have to defer the
> > > release of the kmem_cache struct until the ->release callback is
> > > triggered. The put of the embedded kobject must be the last action on the
> > > kmem_cache  structure which will then trigger release and that will
> > > trigger the kmem_cache_free().
> > >
> >
> > Ok, that sounds reasonable, or you can just create a "tiny" structure
> > for the kobject that has a pointer back to your kmem_cache structure
> > that you can then reference from the show/store functions.  Either is
> > fine with me.
> 
> Problem is that the release field is only available if
> CONFIG_DEBUG_KOBJECT_RELEASE is enabled. Without the callback I cannot
> tell when it is legit to release the kobject structure unless I keep
> scanning it once in awhile.

No, the release callback is in the kobj_type, not the kobject itself.

Re: netfilter: active obj WARN when cleaning up

[Christoph Lameter]

On Mon, 2 Dec 2013, Greg KH wrote:

> No, the release callback is in the kobj_type, not the kobject itself.

Ahh... Ok. Patch follows:


Subject: slub: use sysfs'es release mechanism for kmem_cache

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>

Index: linux/include/linux/slub_def.h
===================================================================
--- linux.orig/include/linux/slub_def.h	2013-12-02 13:31:07.395905824 -0600
+++ linux/include/linux/slub_def.h	2013-12-02 13:31:07.385906101 -0600
@@ -98,4 +98,8 @@ struct kmem_cache {
 	struct kmem_cache_node *node[MAX_NUMNODES];
 };

+#ifdef CONFIG_SYSFS
+#define SLAB_SUPPORTS_SYSFS
+#endif
+
 #endif /* _LINUX_SLUB_DEF_H */
Index: linux/mm/slab.h
===================================================================
--- linux.orig/mm/slab.h	2013-12-02 13:31:07.395905824 -0600
+++ linux/mm/slab.h	2013-12-02 13:39:46.671476284 -0600
@@ -57,6 +57,7 @@ struct mem_cgroup;
 struct kmem_cache *
 __kmem_cache_alias(struct mem_cgroup *memcg, 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(struct mem_cgroup *memcg, const char *name, size_t size,
@@ -91,6 +92,7 @@ __kmem_cache_alias(struct mem_cgroup *me
 #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;
Index: linux/mm/slab_common.c
===================================================================
--- linux.orig/mm/slab_common.c	2013-12-02 13:31:07.395905824 -0600
+++ linux/mm/slab_common.c	2013-12-02 13:33:57.221186749 -0600
@@ -251,6 +251,12 @@ kmem_cache_create(const char *name, size
 }
 EXPORT_SYMBOL(kmem_cache_create);

+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)
 {
 	/* Destroy all the children caches if we aren't a memcg cache */
@@ -268,8 +274,12 @@ void kmem_cache_destroy(struct kmem_cach
 				rcu_barrier();

 			memcg_release_cache(s);
-			kfree(s->name);
-			kmem_cache_free(kmem_cache, s);
+#ifdef SLAB_SUPPORTS_SYSFS
+			sysfs_slab_remove(s);
+#else
+			slab_kmem_cache_release();
+
+#endif
 		} else {
 			list_add(&s->list, &slab_caches);
 			mutex_unlock(&slab_mutex);
Index: linux/mm/slub.c
===================================================================
--- linux.orig/mm/slub.c	2013-12-02 13:31:07.395905824 -0600
+++ linux/mm/slub.c	2013-12-02 13:35:44.208213810 -0600
@@ -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; }
@@ -3208,23 +3207,7 @@ static inline int kmem_cache_close(struc

 int __kmem_cache_shutdown(struct kmem_cache *s)
 {
-	int rc = kmem_cache_close(s);
-
-	if (!rc) {
-		/*
-		 * We do the same lock strategy around sysfs_slab_add, see
-		 * __kmem_cache_create. 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);
 }

 /********************************************************************
@@ -5073,6 +5056,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,
@@ -5080,6 +5068,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)
@@ -5184,7 +5173,7 @@ static int sysfs_slab_add(struct kmem_ca
 	return 0;
 }

-static void sysfs_slab_remove(struct kmem_cache *s)
+void sysfs_slab_remove(struct kmem_cache *s)
 {
 	if (slab_state < FULL)
 		/*

Re: netfilter: active obj WARN when cleaning up

[Greg KH]

On Mon, Dec 02, 2013 at 07:41:15PM +0000, Christoph Lameter wrote:
> On Mon, 2 Dec 2013, Greg KH wrote:
> 
> > No, the release callback is in the kobj_type, not the kobject itself.
> 
> Ahh... Ok. Patch follows:
> 
> 
> Subject: slub: use sysfs'es release mechanism for kmem_cache
> 
> 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>

That looks good, if you fix the indentation issue :)

> 
> Index: linux/include/linux/slub_def.h
> ===================================================================
> --- linux.orig/include/linux/slub_def.h	2013-12-02 13:31:07.395905824 -0600
> +++ linux/include/linux/slub_def.h	2013-12-02 13:31:07.385906101 -0600
> @@ -98,4 +98,8 @@ struct kmem_cache {
>  	struct kmem_cache_node *node[MAX_NUMNODES];
>  };
> 
> +#ifdef CONFIG_SYSFS
> +#define SLAB_SUPPORTS_SYSFS

Why even define this?  Why not just use CONFIG_SYSFS?

thanks,

greg k-h

Re: netfilter: active obj WARN when cleaning up

[Christoph Lameter]

On Mon, 2 Dec 2013, Greg KH wrote:

> > Signed-off-by: Christoph Lameter <cl@linux.com>
>
> That looks good, if you fix the indentation issue :)

Huh?

> > Index: linux/include/linux/slub_def.h
> > ===================================================================
> > --- linux.orig/include/linux/slub_def.h	2013-12-02 13:31:07.395905824 -0600
> > +++ linux/include/linux/slub_def.h	2013-12-02 13:31:07.385906101 -0600
> > @@ -98,4 +98,8 @@ struct kmem_cache {
> >  	struct kmem_cache_node *node[MAX_NUMNODES];
> >  };
> >
> > +#ifdef CONFIG_SYSFS
> > +#define SLAB_SUPPORTS_SYSFS
>
> Why even define this?  Why not just use CONFIG_SYSFS?

Because not all slab allocators currently support SYSFS and there is the
need to have different code now in slab_common.c depending on the
configuration of the allocator.

Re: netfilter: active obj WARN when cleaning up

[Greg KH]

On Mon, Dec 02, 2013 at 09:55:49PM +0000, Christoph Lameter wrote:
> On Mon, 2 Dec 2013, Greg KH wrote:
> 
> > > Signed-off-by: Christoph Lameter <cl@linux.com>
> >
> > That looks good, if you fix the indentation issue :)
> 
> Huh?

Your release function had 2 tabs for the lines, not one.

> > > Index: linux/include/linux/slub_def.h
> > > ===================================================================
> > > --- linux.orig/include/linux/slub_def.h	2013-12-02 13:31:07.395905824 -0600
> > > +++ linux/include/linux/slub_def.h	2013-12-02 13:31:07.385906101 -0600
> > > @@ -98,4 +98,8 @@ struct kmem_cache {
> > >  	struct kmem_cache_node *node[MAX_NUMNODES];
> > >  };
> > >
> > > +#ifdef CONFIG_SYSFS
> > > +#define SLAB_SUPPORTS_SYSFS
> >
> > Why even define this?  Why not just use CONFIG_SYSFS?
> 
> Because not all slab allocators currently support SYSFS and there is the
> need to have different code now in slab_common.c depending on the
> configuration of the allocator.

But you are defining something that you only ever check once, why not
just use CONFIG_SYSFS instead as it makes more sense, not the other way
around.

thanks,

greg k-h

Re: netfilter: active obj WARN when cleaning up

[Christoph Lameter]

On Mon, 2 Dec 2013, Greg KH wrote:

> Your release function had 2 tabs for the lines, not one.

Ah ok. Fixed.

> > > > Index: linux/include/linux/slub_def.h
> > > > ===================================================================
> > > > --- linux.orig/include/linux/slub_def.h	2013-12-02 13:31:07.395905824 -0600
> > > > +++ linux/include/linux/slub_def.h	2013-12-02 13:31:07.385906101 -0600
> > > > @@ -98,4 +98,8 @@ struct kmem_cache {
> > > >  	struct kmem_cache_node *node[MAX_NUMNODES];
> > > >  };
> > > >
> > > > +#ifdef CONFIG_SYSFS
> > > > +#define SLAB_SUPPORTS_SYSFS
> > >
> > > Why even define this?  Why not just use CONFIG_SYSFS?
> >
> > Because not all slab allocators currently support SYSFS and there is the
> > need to have different code now in slab_common.c depending on the
> > configuration of the allocator.
>
> But you are defining something that you only ever check once, why not
> just use CONFIG_SYSFS instead as it makes more sense, not the other way
> around.

We cannot use CONFIG_SYSFS otherwise it would break SLAB since some of
the code modified is shared between allocators. SLAB currently does not
support sysfs. When we add that then we can get rid of the #define.

Subject: slub: use sysfs'es release mechanism for kmem_cache

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>

Index: linux/include/linux/slub_def.h
===================================================================
--- linux.orig/include/linux/slub_def.h	2013-12-03 09:19:26.214666210 -0600
+++ linux/include/linux/slub_def.h	2013-12-03 09:19:26.214666210 -0600
@@ -98,4 +98,8 @@ struct kmem_cache {
 	struct kmem_cache_node *node[MAX_NUMNODES];
 };

+#ifdef CONFIG_SYSFS
+#define SLAB_SUPPORTS_SYSFS
+#endif
+
 #endif /* _LINUX_SLUB_DEF_H */
Index: linux/mm/slab.h
===================================================================
--- linux.orig/mm/slab.h	2013-12-03 09:19:26.214666210 -0600
+++ linux/mm/slab.h	2013-12-03 09:19:26.214666210 -0600
@@ -57,6 +57,7 @@ struct mem_cgroup;
 struct kmem_cache *
 __kmem_cache_alias(struct mem_cgroup *memcg, 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(struct mem_cgroup *memcg, const char *name, size_t size,
@@ -91,6 +92,7 @@ __kmem_cache_alias(struct mem_cgroup *me
 #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;
Index: linux/mm/slab_common.c
===================================================================
--- linux.orig/mm/slab_common.c	2013-12-03 09:19:26.214666210 -0600
+++ linux/mm/slab_common.c	2013-12-03 09:19:54.373883727 -0600
@@ -251,6 +251,12 @@ kmem_cache_create(const char *name, size
 }
 EXPORT_SYMBOL(kmem_cache_create);

+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)
 {
 	/* Destroy all the children caches if we aren't a memcg cache */
@@ -268,8 +274,12 @@ void kmem_cache_destroy(struct kmem_cach
 				rcu_barrier();

 			memcg_release_cache(s);
-			kfree(s->name);
-			kmem_cache_free(kmem_cache, s);
+#ifdef SLAB_SUPPORTS_SYSFS
+			sysfs_slab_remove(s);
+#else
+			slab_kmem_cache_release();
+
+#endif
 		} else {
 			list_add(&s->list, &slab_caches);
 			mutex_unlock(&slab_mutex);
Index: linux/mm/slub.c
===================================================================
--- linux.orig/mm/slub.c	2013-12-03 09:19:26.214666210 -0600
+++ linux/mm/slub.c	2013-12-03 09:19:26.214666210 -0600
@@ -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; }
@@ -3208,23 +3207,7 @@ static inline int kmem_cache_close(struc

 int __kmem_cache_shutdown(struct kmem_cache *s)
 {
-	int rc = kmem_cache_close(s);
-
-	if (!rc) {
-		/*
-		 * We do the same lock strategy around sysfs_slab_add, see
-		 * __kmem_cache_create. 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);
 }

 /********************************************************************
@@ -5073,6 +5056,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,
@@ -5080,6 +5068,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)
@@ -5184,7 +5173,7 @@ static int sysfs_slab_add(struct kmem_ca
 	return 0;
 }

-static void sysfs_slab_remove(struct kmem_cache *s)
+void sysfs_slab_remove(struct kmem_cache *s)
 {
 	if (slab_state < FULL)
 		/*

Re: netfilter: active obj WARN when cleaning up

[Sasha Levin]

On 12/03/2013 10:22 AM, Christoph Lameter wrote:
> On Mon, 2 Dec 2013, Greg KH wrote:
>
>> Your release function had 2 tabs for the lines, not one.
>
> Ah ok. Fixed.
>
>>>>> Index: linux/include/linux/slub_def.h
>>>>> ===================================================================
>>>>> --- linux.orig/include/linux/slub_def.h	2013-12-02 13:31:07.395905824 -0600
>>>>> +++ linux/include/linux/slub_def.h	2013-12-02 13:31:07.385906101 -0600
>>>>> @@ -98,4 +98,8 @@ struct kmem_cache {
>>>>>   	struct kmem_cache_node *node[MAX_NUMNODES];
>>>>>   };
>>>>>
>>>>> +#ifdef CONFIG_SYSFS
>>>>> +#define SLAB_SUPPORTS_SYSFS
>>>>
>>>> Why even define this?  Why not just use CONFIG_SYSFS?
>>>
>>> Because not all slab allocators currently support SYSFS and there is the
>>> need to have different code now in slab_common.c depending on the
>>> configuration of the allocator.
>>
>> But you are defining something that you only ever check once, why not
>> just use CONFIG_SYSFS instead as it makes more sense, not the other way
>> around.
>
> We cannot use CONFIG_SYSFS otherwise it would break SLAB since some of
> the code modified is shared between allocators. SLAB currently does not
> support sysfs. When we add that then we can get rid of the #define.
>
> Subject: slub: use sysfs'es release mechanism for kmem_cache
>
> 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>

I'm still seeing warnings with this patch applied:

[   24.900482] WARNING: CPU: 12 PID: 3654 at lib/debugobjects.c:260 debug_print_object+
0x8d/0xb0()
[   24.900482] ODEBUG: free active (active state 0) object type: timer_list hint: delay
ed_work_timer_fn+0x0/0x20
[   24.900482] Modules linked in:
[   24.900482] CPU: 12 PID: 3654 Comm: kworker/12:1 Tainted: G        W    3.13.0-rc4-n
ext-20131217-sasha-00013-ga878504-dirty #4149
[   24.900482] Workqueue: events kobject_delayed_cleanup
[   24.900482]  0000000000000104 ffff8804f429bae8 ffffffff8439501c ffffffff8555a92c
[   24.900482]  ffff8804f429bb38 ffff8804f429bb28 ffffffff8112f8ac ffff8804f429bb58
[   24.900482]  ffffffff856a9413 ffff880826333530 ffffffff85c68c40 ffffffff8801bb58
[   24.900482] Call Trace:
[   24.900482]  [<ffffffff8439501c>] dump_stack+0x52/0x7f
[   24.900482]  [<ffffffff8112f8ac>] warn_slowpath_common+0x8c/0xc0
[   24.900482]  [<ffffffff8112f996>] warn_slowpath_fmt+0x46/0x50
[   24.900482]  [<ffffffff81adb50d>] debug_print_object+0x8d/0xb0
[   24.900482]  [<ffffffff81153090>] ? __queue_work+0x3f0/0x3f0
[   24.900482]  [<ffffffff81adbd15>] __debug_check_no_obj_freed+0xa5/0x220
[   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff81adbea5>] debug_check_no_obj_freed+0x15/0x20
[   24.900482]  [<ffffffff812ad54f>] kfree+0x21f/0x2e0
[   24.900482]  [<ffffffff832b1acb>] rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff8207efd5>] device_release+0x65/0xc0
[   24.900482]  [<ffffffff81ab05e5>] kobject_cleanup+0x145/0x190
[   24.900482]  [<ffffffff81ab063d>] kobject_delayed_cleanup+0xd/0x10
[   24.900482]  [<ffffffff81153a60>] process_one_work+0x320/0x530
[   24.900482]  [<ffffffff81153940>] ? process_one_work+0x200/0x530
[   24.900482]  [<ffffffff81155fe5>] worker_thread+0x215/0x350
[   24.900482]  [<ffffffff81155dd0>] ? manage_workers+0x180/0x180
[   24.900482]  [<ffffffff8115c9c5>] kthread+0x105/0x110
[   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
[   24.900482]  [<ffffffff843a5e7c>] ret_from_fork+0x7c/0xb0
[   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
[   24.900482] ---[ end trace 45529ebf79b2573e ]---


Thanks,
Sasha

Re: netfilter: active obj WARN when cleaning up

[Christoph Lameter]

On Tue, 17 Dec 2013, Sasha Levin wrote:

> I'm still seeing warnings with this patch applied:

Looks like this is related to some device release mechanism that frees
twice?

I do not see any kmem_cache management functions in the backtrace and
therefore would guess that this is not the same issue.

Re: netfilter: active obj WARN when cleaning up

[Thomas Gleixner]

On Tue, 17 Dec 2013, Sasha Levin wrote:

Cc'ing RTC folks

> I'm still seeing warnings with this patch applied:

That's not a sl*b issue.
 
> [   24.900482] WARNING: CPU: 12 PID: 3654 at lib/debugobjects.c:260
> debug_print_object+
> 0x8d/0xb0()
> [   24.900482] ODEBUG: free active (active state 0) object type: timer_list
> hint: delay
> ed_work_timer_fn+0x0/0x20
> [   24.900482] Modules linked in:
> [   24.900482] CPU: 12 PID: 3654 Comm: kworker/12:1 Tainted: G        W
> 3.13.0-rc4-n
> ext-20131217-sasha-00013-ga878504-dirty #4149
> [   24.900482] Workqueue: events kobject_delayed_cleanup
> [   24.900482]  0000000000000104 ffff8804f429bae8 ffffffff8439501c
> ffffffff8555a92c
> [   24.900482]  ffff8804f429bb38 ffff8804f429bb28 ffffffff8112f8ac
> ffff8804f429bb58
> [   24.900482]  ffffffff856a9413 ffff880826333530 ffffffff85c68c40
> ffffffff8801bb58
> [   24.900482] Call Trace:
> [   24.900482]  [<ffffffff8439501c>] dump_stack+0x52/0x7f
> [   24.900482]  [<ffffffff8112f8ac>] warn_slowpath_common+0x8c/0xc0
> [   24.900482]  [<ffffffff8112f996>] warn_slowpath_fmt+0x46/0x50
> [   24.900482]  [<ffffffff81adb50d>] debug_print_object+0x8d/0xb0
> [   24.900482]  [<ffffffff81153090>] ? __queue_work+0x3f0/0x3f0
> [   24.900482]  [<ffffffff81adbd15>] __debug_check_no_obj_freed+0xa5/0x220
> [   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
> [   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
> [   24.900482]  [<ffffffff81adbea5>] debug_check_no_obj_freed+0x15/0x20
> [   24.900482]  [<ffffffff812ad54f>] kfree+0x21f/0x2e0
> [   24.900482]  [<ffffffff832b1acb>] rtc_device_release+0x2b/0x40
> [   24.900482]  [<ffffffff8207efd5>] device_release+0x65/0xc0
> [   24.900482]  [<ffffffff81ab05e5>] kobject_cleanup+0x145/0x190
> [   24.900482]  [<ffffffff81ab063d>] kobject_delayed_cleanup+0xd/0x10
> [   24.900482]  [<ffffffff81153a60>] process_one_work+0x320/0x530
> [   24.900482]  [<ffffffff81153940>] ? process_one_work+0x200/0x530
> [   24.900482]  [<ffffffff81155fe5>] worker_thread+0x215/0x350
> [   24.900482]  [<ffffffff81155dd0>] ? manage_workers+0x180/0x180
> [   24.900482]  [<ffffffff8115c9c5>] kthread+0x105/0x110
> [   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
> [   24.900482]  [<ffffffff843a5e7c>] ret_from_fork+0x7c/0xb0
> [   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
> [   24.900482] ---[ end trace 45529ebf79b2573e ]---
> 
> 
> Thanks,
> Sasha
> 
> 

Re: netfilter: active obj WARN when cleaning up

[Sasha Levin]

On 12/17/2013 03:37 PM, Christoph Lameter wrote:
> On Tue, 17 Dec 2013, Sasha Levin wrote:
>
>> I'm still seeing warnings with this patch applied:
>
> Looks like this is related to some device release mechanism that frees
> twice?
>
> I do not see any kmem_cache management functions in the backtrace and
> therefore would guess that this is not the same issue.

rgr, sorry about that - I grepped for 'timer_list' which seemed to have been in
all previous kmem_cache spews and thought this one is related.

If that's not the case, your patch works for me.


Thanks,
Sasha

Re: netfilter: active obj WARN when cleaning up

[Bart Van Assche]

On 12/17/13 20:53, Sasha Levin wrote:
> I'm still seeing warnings with this patch applied:
> 
> [   24.900482] WARNING: CPU: 12 PID: 3654 at lib/debugobjects.c:260
> debug_print_object+
> 0x8d/0xb0()
> [   24.900482] ODEBUG: free active (active state 0) object type:
> timer_list hint: delay
> ed_work_timer_fn+0x0/0x20
> [   24.900482] Modules linked in:
> [   24.900482] CPU: 12 PID: 3654 Comm: kworker/12:1 Tainted: G       
> W    3.13.0-rc4-n
> ext-20131217-sasha-00013-ga878504-dirty #4149
> [   24.900482] Workqueue: events kobject_delayed_cleanup
> [   24.900482]  0000000000000104 ffff8804f429bae8 ffffffff8439501c
> ffffffff8555a92c
> [   24.900482]  ffff8804f429bb38 ffff8804f429bb28 ffffffff8112f8ac
> ffff8804f429bb58
> [   24.900482]  ffffffff856a9413 ffff880826333530 ffffffff85c68c40
> ffffffff8801bb58
> [   24.900482] Call Trace:
> [   24.900482]  [<ffffffff8439501c>] dump_stack+0x52/0x7f
> [   24.900482]  [<ffffffff8112f8ac>] warn_slowpath_common+0x8c/0xc0
> [   24.900482]  [<ffffffff8112f996>] warn_slowpath_fmt+0x46/0x50
> [   24.900482]  [<ffffffff81adb50d>] debug_print_object+0x8d/0xb0
> [   24.900482]  [<ffffffff81153090>] ? __queue_work+0x3f0/0x3f0
> [   24.900482]  [<ffffffff81adbd15>] __debug_check_no_obj_freed+0xa5/0x220
> [   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
> [   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
> [   24.900482]  [<ffffffff81adbea5>] debug_check_no_obj_freed+0x15/0x20
> [   24.900482]  [<ffffffff812ad54f>] kfree+0x21f/0x2e0
> [   24.900482]  [<ffffffff832b1acb>] rtc_device_release+0x2b/0x40
> [   24.900482]  [<ffffffff8207efd5>] device_release+0x65/0xc0
> [   24.900482]  [<ffffffff81ab05e5>] kobject_cleanup+0x145/0x190
> [   24.900482]  [<ffffffff81ab063d>] kobject_delayed_cleanup+0xd/0x10
> [   24.900482]  [<ffffffff81153a60>] process_one_work+0x320/0x530
> [   24.900482]  [<ffffffff81153940>] ? process_one_work+0x200/0x530
> [   24.900482]  [<ffffffff81155fe5>] worker_thread+0x215/0x350
> [   24.900482]  [<ffffffff81155dd0>] ? manage_workers+0x180/0x180
> [   24.900482]  [<ffffffff8115c9c5>] kthread+0x105/0x110
> [   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
> [   24.900482]  [<ffffffff843a5e7c>] ret_from_fork+0x7c/0xb0
> [   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
> [   24.900482] ---[ end trace 45529ebf79b2573e ]---

Can anyone tell me whether the patch below makes sense ? Please note
that I'm not familiar with the timer subsystem.

diff --git a/kernel/timer.c b/kernel/timer.c
index accfd24..828b666 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -969,6 +969,8 @@ int del_timer(struct timer_list *timer)
                base = lock_timer_base(timer, &flags);
                ret = detach_if_pending(timer, base, true);
                spin_unlock_irqrestore(&base->lock, flags);
+       } else {
+               debug_deactivate(timer);
        }

        return ret;

Re: netfilter: active obj WARN when cleaning up

[Bart Van Assche]

On 12/19/13 13:12, Bart Van Assche wrote:
> On 12/17/13 20:53, Sasha Levin wrote:
>> I'm still seeing warnings with this patch applied:
>>
>> [   24.900482] WARNING: CPU: 12 PID: 3654 at lib/debugobjects.c:260
>> debug_print_object+0x8d/0xb0()
>> [   24.900482] ODEBUG: free active (active state 0) object type:
>> timer_list hint: delayed_work_timer_fn+0x0/0x20
>> [   24.900482] Modules linked in:
>> [   24.900482] CPU: 12 PID: 3654 Comm: kworker/12:1 Tainted: G       
>> W    3.13.0-rc4-n
>> ext-20131217-sasha-00013-ga878504-dirty #4149
>> [   24.900482] Workqueue: events kobject_delayed_cleanup
>> [   24.900482]  0000000000000104 ffff8804f429bae8 ffffffff8439501c
>> ffffffff8555a92c
>> [   24.900482]  ffff8804f429bb38 ffff8804f429bb28 ffffffff8112f8ac
>> ffff8804f429bb58
>> [   24.900482]  ffffffff856a9413 ffff880826333530 ffffffff85c68c40
>> ffffffff8801bb58
>> [   24.900482] Call Trace:
>> [   24.900482]  [<ffffffff8439501c>] dump_stack+0x52/0x7f
>> [   24.900482]  [<ffffffff8112f8ac>] warn_slowpath_common+0x8c/0xc0
>> [   24.900482]  [<ffffffff8112f996>] warn_slowpath_fmt+0x46/0x50
>> [   24.900482]  [<ffffffff81adb50d>] debug_print_object+0x8d/0xb0
>> [   24.900482]  [<ffffffff81153090>] ? __queue_work+0x3f0/0x3f0
>> [   24.900482]  [<ffffffff81adbd15>] __debug_check_no_obj_freed+0xa5/0x220
>> [   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
>> [   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
>> [   24.900482]  [<ffffffff81adbea5>] debug_check_no_obj_freed+0x15/0x20
>> [   24.900482]  [<ffffffff812ad54f>] kfree+0x21f/0x2e0
>> [   24.900482]  [<ffffffff832b1acb>] rtc_device_release+0x2b/0x40
>> [   24.900482]  [<ffffffff8207efd5>] device_release+0x65/0xc0
>> [   24.900482]  [<ffffffff81ab05e5>] kobject_cleanup+0x145/0x190
>> [   24.900482]  [<ffffffff81ab063d>] kobject_delayed_cleanup+0xd/0x10
>> [   24.900482]  [<ffffffff81153a60>] process_one_work+0x320/0x530
>> [   24.900482]  [<ffffffff81153940>] ? process_one_work+0x200/0x530
>> [   24.900482]  [<ffffffff81155fe5>] worker_thread+0x215/0x350
>> [   24.900482]  [<ffffffff81155dd0>] ? manage_workers+0x180/0x180
>> [   24.900482]  [<ffffffff8115c9c5>] kthread+0x105/0x110
>> [   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
>> [   24.900482]  [<ffffffff843a5e7c>] ret_from_fork+0x7c/0xb0
>> [   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
>> [   24.900482] ---[ end trace 45529ebf79b2573e ]---
> 
> Can anyone tell me whether the patch below makes sense ?
> 
> [ ... ]

(replying to my own e-mail)

Please ignore the patch in the previous e-mail - it did not make sense.

Regarding the warning above: the "delayed_work_timer_fn+0x0/0x20" hint
probably indicates an attempt to free a delayed_work structure that is
embedded in struct rtc_device and that is still scheduled. It's
unfortunate that debug_check_no_obj_freed() does not print the address
of the offending object and the argument passed to kfree(). That
information would allow to compute the offset of the embedded
delayed_work structure make it easier to figure out what's going on.

Bart.