2014-04-24 16:56:55

by Sasha Levin

[permalink] [raw]
Subject: fs: dcookie: freeing active timer

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


2014-04-24 17:27:13

by Al Viro

[permalink] [raw]
Subject: Re: fs: dcookie: freeing active timer

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?

2014-04-24 17:34:32

by Sasha Levin

[permalink] [raw]
Subject: Re: fs: dcookie: freeing active timer

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

2014-04-24 21:56:07

by Al Viro

[permalink] [raw]
Subject: Re: fs: dcookie: freeing active timer

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

2014-04-24 23:49:47

by Al Viro

[permalink] [raw]
Subject: Re: fs: dcookie: freeing active timer

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 <[email protected]>
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...

2014-04-25 03:32:10

by Sasha Levin

[permalink] [raw]
Subject: Re: fs: dcookie: freeing active timer

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 <[email protected]>
> 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

2014-04-30 21:48:27

by Sasha Levin

[permalink] [raw]
Subject: Re: fs: dcookie: freeing active timer

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 <[email protected]>
>> 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?

Subject: Re: fs: dcookie: freeing active timer

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.