gigaset: freeing an active object

DISCUSSION Sasha Levin 5 participants 25 messages Nov 27, 2015 – Dec 7, 2015

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Paul Bolle

2015-11-27 17:57 UTC #1 lore raw

Hi Sascha,

Quoted 1 line · Sasha Levin

Fuzzing with syzkaller on the latest -next kernel produced this error:

(syzkaller is new to me. I'll have to do some web searches.)

Quoted 28 lines

[  413.536749] WARNING: CPU: 6 PID: 25400 at lib/debugobjects.c:263
debug_print_object+0x1c4/0x1e0()
[  413.538111] ODEBUG: free active (active state 0) object type:
timer_list hint: delayed_work_timer_fn+0x0/0x90
[  413.539598] Modules linked
in:3470693efef57268844f02f5de3ab392d8cf5e209671ddd87163cb964c510659
[  413.540448] CPU: 6 PID: 25400 Comm: syzkaller_execu Not tainted
4.4.0-rc2-next-20151126-sasha-00005-g00d303e-dirty #2653
[  413.547614] Call Trace:
[  413.548077]  [<ffffffffa8e6b5bb>] dump_stack+0x72/0xb7
[  413.548765]  [<ffffffffa73531d3>] warn_slowpath_common+0x113/0x140
[  413.551151]  [<ffffffffa73532cb>] warn_slowpath_fmt+0xcb/0x100
[  413.554295]  [<ffffffffa8ed0194>] debug_print_object+0x1c4/0x1e0
[  413.556592]  [<ffffffffa8ed1035>]
__debug_check_no_obj_freed+0x215/0x7a0
[  413.560526]  [<ffffffffa8ed2b6c>]
debug_check_no_obj_freed+0x2c/0x40
[  413.561328]  [<ffffffffa77aac4c>] kfree+0x1fc/0x2f0
[  413.561970]  [<ffffffffae74b021>] gigaset_freecshw+0xe1/0x120
[  413.562723]  [<ffffffffae70669d>] gigaset_freecs+0x2ad/0x600
[  413.564240]  [<ffffffffae74ba60>] gigaset_tty_close+0x210/0x280
[  413.565774]  [<ffffffffa95ba6f2>] tty_ldisc_close.isra.1+0xc2/0xd0
[  413.566550]  [<ffffffffa95ba81b>] tty_ldisc_kill+0x4b/0x170
[  413.567253]  [<ffffffffa95bbba3>] tty_ldisc_release+0x183/0x240
[  413.568000]  [<ffffffffa95a507c>] tty_release+0xd1c/0xe80
[  413.570176]  [<ffffffffa78182fa>] __fput+0x32a/0x680
[  413.570888]  [<ffffffffa78186da>] ____fput+0x1a/0x20
[  413.571565]  [<ffffffffa73adf5c>] task_work_run+0x19c/0x1e0

Quoted 8 lines

[  413.572290]  [<ffffffffa735cae7>] do_exit+0xdf7/0x28f0
[  413.576188]  [<ffffffffa735e805>] do_group_exit+0x1b5/0x300
[  413.576905]  [<ffffffffa7382222>] get_signal+0x1182/0x1360
[  413.577627]  [<ffffffffa717b553>] do_signal+0x93/0x1690
[  413.584630]  [<ffffffffa70063b0>] exit_to_usermode_loop+0xc0/0x1e0
[  413.585412]  [<ffffffffa7007feb>]
prepare_exit_to_usermode+0x10b/0x140
[  413.586187]  [<ffffffffb0a12c3e>] retint_user+0x8/0x23

Thanks for the report. I'll look into it.

A question that's nagging me for some time now is: how many people use gigaset drivers (and, actually ISDN drivers in general) while running recent kernel releases? I haven't seen reports that suggest people really do that for some time now.

So, just to be absolutely sure: this was generated with a kernel that used the ser-gigaset driver without actually using the related hardware, wasn't it? (That hardware is a clunky, self powered device that requires a good old serial port.)

Thanks,

Paul Bolle

Sasha Levin

author

2015-11-27 18:15 UTC #2 lore raw

Quoted 6 lines · Paul Bolle

Hi Sascha,
Quoted 1 line · Sasha Levin
Fuzzing with syzkaller on the latest -next kernel produced this error:
(syzkaller is new to me. I'll have to do some web searches.)

It's a new fancy syscall/ioctl fuzzer, https://github.com/google/syzkaller/blob/master/README.md

Quoted 44 lines

Quoted 28 lines

[  413.536749] WARNING: CPU: 6 PID: 25400 at lib/debugobjects.c:263
debug_print_object+0x1c4/0x1e0()
[  413.538111] ODEBUG: free active (active state 0) object type:
timer_list hint: delayed_work_timer_fn+0x0/0x90
[  413.539598] Modules linked
in:3470693efef57268844f02f5de3ab392d8cf5e209671ddd87163cb964c510659
[  413.540448] CPU: 6 PID: 25400 Comm: syzkaller_execu Not tainted
4.4.0-rc2-next-20151126-sasha-00005-g00d303e-dirty #2653
[  413.547614] Call Trace:
[  413.548077]  [<ffffffffa8e6b5bb>] dump_stack+0x72/0xb7
[  413.548765]  [<ffffffffa73531d3>] warn_slowpath_common+0x113/0x140
[  413.551151]  [<ffffffffa73532cb>] warn_slowpath_fmt+0xcb/0x100
[  413.554295]  [<ffffffffa8ed0194>] debug_print_object+0x1c4/0x1e0
[  413.556592]  [<ffffffffa8ed1035>]
__debug_check_no_obj_freed+0x215/0x7a0
[  413.560526]  [<ffffffffa8ed2b6c>]
debug_check_no_obj_freed+0x2c/0x40
[  413.561328]  [<ffffffffa77aac4c>] kfree+0x1fc/0x2f0
[  413.561970]  [<ffffffffae74b021>] gigaset_freecshw+0xe1/0x120
[  413.562723]  [<ffffffffae70669d>] gigaset_freecs+0x2ad/0x600
[  413.564240]  [<ffffffffae74ba60>] gigaset_tty_close+0x210/0x280
[  413.565774]  [<ffffffffa95ba6f2>] tty_ldisc_close.isra.1+0xc2/0xd0
[  413.566550]  [<ffffffffa95ba81b>] tty_ldisc_kill+0x4b/0x170
[  413.567253]  [<ffffffffa95bbba3>] tty_ldisc_release+0x183/0x240
[  413.568000]  [<ffffffffa95a507c>] tty_release+0xd1c/0xe80
[  413.570176]  [<ffffffffa78182fa>] __fput+0x32a/0x680
[  413.570888]  [<ffffffffa78186da>] ____fput+0x1a/0x20
[  413.571565]  [<ffffffffa73adf5c>] task_work_run+0x19c/0x1e0

Quoted 8 lines

[  413.572290]  [<ffffffffa735cae7>] do_exit+0xdf7/0x28f0
[  413.576188]  [<ffffffffa735e805>] do_group_exit+0x1b5/0x300
[  413.576905]  [<ffffffffa7382222>] get_signal+0x1182/0x1360
[  413.577627]  [<ffffffffa717b553>] do_signal+0x93/0x1690
[  413.584630]  [<ffffffffa70063b0>] exit_to_usermode_loop+0xc0/0x1e0
[  413.585412]  [<ffffffffa7007feb>]
prepare_exit_to_usermode+0x10b/0x140
[  413.586187]  [<ffffffffb0a12c3e>] retint_user+0x8/0x23

Thanks for the report. I'll look into it.

Quoted 4 lines

So, just to be absolutely sure: this was generated with a kernel that used the ser-gigaset driver without actually using the related hardware, wasn't it? (That hardware is a clunky, self powered device that requires a good old serial port.)

Right, no related hardware, inside a VM.

Thanks,
Sasha

Paul Bolle

2015-11-28 00:28 UTC #3 lore raw

(A few quick notes follow. The hope here is basically that my display of ignorance might trigger others to speak up while I'm still pondering on this bug.)

Quoted 9 lines · Sasha Levin

Quoted 5 lines · Paul Bolle
Quoted 2 lines · Sasha Levin
Fuzzing with syzkaller on the latest -next kernel produced this error:
(syzkaller is new to me. I'll have to do some web searches.)
It's a new fancy syscall/ioctl fuzzer,
https://github.com/google/syzkaller/blob/master/README.md

Thanks.

That fuzzer apparently requires either CONFIG_KASAN, CONFIG_KTSAN, or CONFIG_UBSAN, none of which I'm familiar with.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
[  413.536749] WARNING: CPU: 6 PID: 25400 at
lib/debugobjects.c:263
debug_print_object+0x1c4/0x1e0()
[  413.538111] ODEBUG: free active (active state 0) object type:
timer_list hint: delayed_work_timer_fn+0x0/0x90

There are two places that add "free" here, but there's no obvious way to distinguish between them. Annoying.

Anyhow, this is interesting. ser-gigaset doesn't use timer_list while bas-gigaset does.

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
[  413.539598] Modules linked
in:3470693efef57268844f02f5de3ab392d8cf5e209671ddd87163cb964c51065 9

Not sure what this means. The bug concerns ser-gigaset so it's safe to assume the fuzzer at one point called

    ioctl(fd, TIOCSETD, N_GIGASET_M101)

which would trigger the use of ser-gigaset.

Quoted 6 lines

[  413.540448] CPU: 6 PID: 25400 Comm: syzkaller_execu Not tainted
4.4.0-rc2-next-20151126-sasha-00005-g00d303e-dirty #2653
[  413.547614] Call Trace:
[  413.548077]  [<ffffffffa8e6b5bb>] dump_stack+0x72/0xb7
[  413.548765]  [<ffffffffa73531d3>]

warn_slowpath_common+0x113/0x140

Quoted 8 lines

[  413.551151]  [<ffffffffa73532cb>] warn_slowpath_fmt+0xcb/0x100
[  413.554295]  [<ffffffffa8ed0194>]
debug_print_object+0x1c4/0x1e0
[  413.556592]  [<ffffffffa8ed1035>]
__debug_check_no_obj_freed+0x215/0x7a0
[  413.560526]  [<ffffffffa8ed2b6c>]
debug_check_no_obj_freed+0x2c/0x40
[  413.561328]  [<ffffffffa77aac4c>] kfree+0x1fc/0x2f0

This should be

    kfree(cs->hw.ser)

Note that cs->hw is a union of struct base_cardstate, struct ser_cardstate, and struct usb_cardstate. And it's obvious that struct ser_cardstate is much smaller that struct bas_cardstate. So we're probably free-ing some memory that, so to speak, includes bas_cardstate.timer_int_in, a struct timer_list. That's likely way beyond the end of struct ser_cardstate and so it should still contain garbage.

Quoted 13 lines

[  413.561970]  [<ffffffffae74b021>] gigaset_freecshw+0xe1/0x120
[  413.562723]  [<ffffffffae70669d>] gigaset_freecs+0x2ad/0x600
[  413.564240]  [<ffffffffae74ba60>] gigaset_tty_close+0x210/0x280
[  413.565774]  [<ffffffffa95ba6f2>]
tty_ldisc_close.isra.1+0xc2/0xd0
[  413.566550]  [<ffffffffa95ba81b>] tty_ldisc_kill+0x4b/0x170
[  413.567253]  [<ffffffffa95bbba3>] tty_ldisc_release+0x183/0x240
[  413.568000]  [<ffffffffa95a507c>] tty_release+0xd1c/0xe80
[  413.570176]  [<ffffffffa78182fa>] __fput+0x32a/0x680
[  413.570888]  [<ffffffffa78186da>] ____fput+0x1a/0x20
[  413.571565]  [<ffffffffa73adf5c>] task_work_run+0x19c/0x1e0
[  413.572290]  [<ffffffffa735cae7>] do_exit+0xdf7/0x28f0
[  413.576188]  [<ffffffffa735e805>] do_group_exit+0x1b5/0x300

Quoted 8 lines

Quoted 7 lines

[  413.576905]  [<ffffffffa7382222>] get_signal+0x1182/0x1360
[  413.577627]  [<ffffffffa717b553>] do_signal+0x93/0x1690
[  413.584630]  [<ffffffffa70063b0>]
exit_to_usermode_loop+0xc0/0x1e0
[  413.585412]  [<ffffffffa7007feb>]
prepare_exit_to_usermode+0x10b/0x140
[  413.586187]  [<ffffffffb0a12c3e>] retint_user+0x8/0x23

I have no idea (yet) what triggers retint_user.

Anyhow, my first hunch is to do a s/kmalloc/kzalloc/ on

    drv->cs = kmalloc(minors * sizeof *drv->cs, GFP_KERNEL)

in drivers/isdn/gigaset/common.c and see if this still triggers. But to test that I need to know how to reproduce this.

To be continued...

Paul Bolle

Peter Hurley

2015-11-28 01:20 UTC #4 lore raw

Quoted 32 lines · Paul Bolle

(A few quick notes follow. The hope here is basically that my display of ignorance might trigger others to speak up while I'm still pondering on this bug.)
Quoted 9 lines · Sasha Levin
Quoted 5 lines · Paul Bolle
Quoted 2 lines · Sasha Levin
Fuzzing with syzkaller on the latest -next kernel produced this error:
(syzkaller is new to me. I'll have to do some web searches.)
It's a new fancy syscall/ioctl fuzzer,
https://github.com/google/syzkaller/blob/master/README.md
Thanks.
That fuzzer apparently requires either CONFIG_KASAN, CONFIG_KTSAN, or CONFIG_UBSAN, none of which I'm familiar with.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
[  413.536749] WARNING: CPU: 6 PID: 25400 at
lib/debugobjects.c:263
debug_print_object+0x1c4/0x1e0()
[  413.538111] ODEBUG: free active (active state 0) object type:
timer_list hint: delayed_work_timer_fn+0x0/0x90
There are two places that add "free" here, but there's no obvious way to distinguish between them. Annoying.
Anyhow, this is interesting. ser-gigaset doesn't use timer_list while bas-gigaset does.

Quoted 7 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
[  413.539598] Modules linked
in:3470693efef57268844f02f5de3ab392d8cf5e209671ddd87163cb964c51065 9
Not sure what this means. The bug concerns ser-gigaset so it's safe to assume the fuzzer at one point called
    ioctl(fd, TIOCSETD, N_GIGASET_M101)

Sasha,

It would really help if you included the syzkaller-generated applet with the bug reports; state previously established by the applet can be crucial in understanding why the call stack looks the way it does.

Also, every generated applet that triggers a report should become a future regression test; I'm collecting the ones pertinent to tty/serial/ ldisc (so that includes this one; if you could send me the x25 one too would be great).

Regards,
Peter Hurley

Quoted 59 lines

which would trigger the use of ser-gigaset.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
[  413.540448] CPU: 6 PID: 25400 Comm: syzkaller_execu Not tainted
4.4.0-rc2-next-20151126-sasha-00005-g00d303e-dirty #2653
[  413.547614] Call Trace:
[  413.548077]  [<ffffffffa8e6b5bb>] dump_stack+0x72/0xb7
[  413.548765]  [<ffffffffa73531d3>]
warn_slowpath_common+0x113/0x140
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
[  413.551151]  [<ffffffffa73532cb>] warn_slowpath_fmt+0xcb/0x100
[  413.554295]  [<ffffffffa8ed0194>]
debug_print_object+0x1c4/0x1e0
[  413.556592]  [<ffffffffa8ed1035>]
__debug_check_no_obj_freed+0x215/0x7a0
[  413.560526]  [<ffffffffa8ed2b6c>]
debug_check_no_obj_freed+0x2c/0x40
[  413.561328]  [<ffffffffa77aac4c>] kfree+0x1fc/0x2f0
This should be
    kfree(cs->hw.ser)
Note that cs->hw is a union of struct base_cardstate, struct ser_cardstate, and struct usb_cardstate. And it's obvious that struct ser_cardstate is much smaller that struct bas_cardstate. So we're probably free-ing some memory that, so to speak, includes bas_cardstate.timer_int_in, a struct timer_list. That's likely way beyond the end of struct ser_cardstate and so it should still contain garbage.
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
[  413.561970]  [<ffffffffae74b021>] gigaset_freecshw+0xe1/0x120
[  413.562723]  [<ffffffffae70669d>] gigaset_freecs+0x2ad/0x600
[  413.564240]  [<ffffffffae74ba60>] gigaset_tty_close+0x210/0x280
[  413.565774]  [<ffffffffa95ba6f2>]
tty_ldisc_close.isra.1+0xc2/0xd0
[  413.566550]  [<ffffffffa95ba81b>] tty_ldisc_kill+0x4b/0x170
[  413.567253]  [<ffffffffa95bbba3>] tty_ldisc_release+0x183/0x240
[  413.568000]  [<ffffffffa95a507c>] tty_release+0xd1c/0xe80
[  413.570176]  [<ffffffffa78182fa>] __fput+0x32a/0x680
[  413.570888]  [<ffffffffa78186da>] ____fput+0x1a/0x20
[  413.571565]  [<ffffffffa73adf5c>] task_work_run+0x19c/0x1e0
[  413.572290]  [<ffffffffa735cae7>] do_exit+0xdf7/0x28f0
[  413.576188]  [<ffffffffa735e805>] do_group_exit+0x1b5/0x300
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
[  413.576905]  [<ffffffffa7382222>] get_signal+0x1182/0x1360
[  413.577627]  [<ffffffffa717b553>] do_signal+0x93/0x1690
[  413.584630]  [<ffffffffa70063b0>]
exit_to_usermode_loop+0xc0/0x1e0
[  413.585412]  [<ffffffffa7007feb>]
prepare_exit_to_usermode+0x10b/0x140
[  413.586187]  [<ffffffffb0a12c3e>] retint_user+0x8/0x23
I have no idea (yet) what triggers retint_user.
Anyhow, my first hunch is to do a s/kmalloc/kzalloc/ on
    drv->cs = kmalloc(minors * sizeof *drv->cs, GFP_KERNEL)
in drivers/isdn/gigaset/common.c and see if this still triggers. But to test that I need to know how to reproduce this.
To be continued...

Sasha Levin

author

2015-11-28 01:27 UTC #5 lore raw

Quoted 8 lines · Peter Hurley

It would really help if you included the syzkaller-generated applet with the bug reports; state previously established by the applet can be crucial in understanding why the call stack looks the way it does.
Also, every generated applet that triggers a report should become a future regression test; I'm collecting the ones pertinent to tty/serial/ ldisc (so that includes this one; if you could send me the x25 one too would be great).

I went in to look for the 'crashers' that I thought are generated when syzkaller manages to crash a kernel, but none appear for me.

Dmitry, is there magic required to generate those?

Thanks,
Sasha

Dmitry Vyukov

2015-11-29 14:36 UTC #6 lore raw

Quoted 10 lines · Sasha Levin

Quoted 8 lines · Peter Hurley
It would really help if you included the syzkaller-generated applet with the bug reports; state previously established by the applet can be crucial in understanding why the call stack looks the way it does.
Also, every generated applet that triggers a report should become a future regression test; I'm collecting the ones pertinent to tty/serial/ ldisc (so that includes this one; if you could send me the x25 one too would be great).

Quoted 4 lines

I went in to look for the 'crashers' that I thought are generated when syzkaller manages to crash a kernel, but none appear for me.
Dmitry, is there magic required to generate those?

There is a little bit of magic, but mostly hard muscle labor. I've outlined how I create reproducers here:
https://github.com/google/syzkaller/wiki/Crash-reproducer-programs
The description is not super detailed, but should be give you some clue. Ideally it is all automated, but somebody needs to write some code for that...

Tilman Schmidt

2015-11-29 15:30 UTC #7 lore raw

In reply to Sasha Levin

Hi Sasha,

thanks for the report. As the original author of the code in question, I am somewhat at a loss what to make of it.

Am 27.11.2015 um 16:19 schrieb Sasha Levin:

Quoted 1 line

Fuzzing with syzkaller on the latest -next kernel produced this error:

Is there a way to know the actual sequence of events that triggered this warning?

Quoted 2 lines

[  413.536749] WARNING: CPU: 6 PID: 25400 at lib/debugobjects.c:263 debug_print_object+0x1c4/0x1e0()
[  413.538111] ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x90

This message seems to indicate that an object of type timer_list was freed which was still active. However the driver in question (ser_gigaset) does not use any timers.

What are the exact conditions for producing this message? IOW how does the ODEBUG code determine that an object of type timer_list is being freed, and that it is still in use?

Are there any messages from ser_gigaset or another one of the gigaset drivers before that warning?

Quoted 1 line

[  413.539598] Modules linked in:3470693efef57268844f02f5de3ab392d8cf5e209671ddd87163cb964c510659

This message does not tell me anything. What does that hex string after the colon mean?

Quoted 9 lines

[  413.540448] CPU: 6 PID: 25400 Comm: syzkaller_execu Not tainted 4.4.0-rc2-next-20151126-sasha-00005-g00d303e-dirty #2653
[  413.547614] Call Trace:
[  413.548077]  [<ffffffffa8e6b5bb>] dump_stack+0x72/0xb7
[  413.548765]  [<ffffffffa73531d3>] warn_slowpath_common+0x113/0x140
[  413.551151]  [<ffffffffa73532cb>] warn_slowpath_fmt+0xcb/0x100
[  413.554295]  [<ffffffffa8ed0194>] debug_print_object+0x1c4/0x1e0
[  413.556592]  [<ffffffffa8ed1035>] __debug_check_no_obj_freed+0x215/0x7a0
[  413.560526]  [<ffffffffa8ed2b6c>] debug_check_no_obj_freed+0x2c/0x40
[  413.561328]  [<ffffffffa77aac4c>] kfree+0x1fc/0x2f0

Judging from the backtrace below this must be the call

        kfree(cs->hw.ser);

in drivers/isdn/gigaset/ser-gigaset.c line 375. cs->hw.ser is of type struct ser_cardstate *.
struct ser_cardstate consists of a struct platform_device, a struct completion, an atomic_t and a pointer. No timer_list.

Quoted 1 line

[  413.561970]  [<ffffffffae74b021>] gigaset_freecshw+0xe1/0x120

There are functions by this name in all three Gigaset hardware dependent modules (bas_gigaset, ser_gigaset and usb_gigaset), but ...

Quoted 2 lines

[  413.562723]  [<ffffffffae70669d>] gigaset_freecs+0x2ad/0x600
[  413.564240]  [<ffffffffae74ba60>] gigaset_tty_close+0x210/0x280

this function only exists in ser_gigaset.

Thanks,
Tilman

Signature · 3 lines

Tilman Schmidt                              E-Mail: [email protected]
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.

Peter Hurley

2015-11-29 18:22 UTC #8 lore raw

Hi Tilman,

Quoted 51 lines · Tilman Schmidt

Hi Sasha,
thanks for the report. As the original author of the code in question, I am somewhat at a loss what to make of it.
Am 27.11.2015 um 16:19 schrieb Sasha Levin:
Quoted 1 line
Fuzzing with syzkaller on the latest -next kernel produced this error:
Is there a way to know the actual sequence of events that triggered this warning?
Quoted 2 lines
[  413.536749] WARNING: CPU: 6 PID: 25400 at lib/debugobjects.c:263 debug_print_object+0x1c4/0x1e0()
[  413.538111] ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x90
This message seems to indicate that an object of type timer_list was freed which was still active. However the driver in question (ser_gigaset) does not use any timers.
What are the exact conditions for producing this message? IOW how does the ODEBUG code determine that an object of type timer_list is being freed, and that it is still in use?
Are there any messages from ser_gigaset or another one of the gigaset drivers before that warning?
Quoted 1 line
[  413.539598] Modules linked in:3470693efef57268844f02f5de3ab392d8cf5e209671ddd87163cb964c510659
This message does not tell me anything. What does that hex string after the colon mean?
Quoted 9 lines
[  413.540448] CPU: 6 PID: 25400 Comm: syzkaller_execu Not tainted 4.4.0-rc2-next-20151126-sasha-00005-g00d303e-dirty #2653
[  413.547614] Call Trace:
[  413.548077]  [<ffffffffa8e6b5bb>] dump_stack+0x72/0xb7
[  413.548765]  [<ffffffffa73531d3>] warn_slowpath_common+0x113/0x140
[  413.551151]  [<ffffffffa73532cb>] warn_slowpath_fmt+0xcb/0x100
[  413.554295]  [<ffffffffa8ed0194>] debug_print_object+0x1c4/0x1e0
[  413.556592]  [<ffffffffa8ed1035>] __debug_check_no_obj_freed+0x215/0x7a0
[  413.560526]  [<ffffffffa8ed2b6c>] debug_check_no_obj_freed+0x2c/0x40
[  413.561328]  [<ffffffffa77aac4c>] kfree+0x1fc/0x2f0
Judging from the backtrace below this must be the call
        kfree(cs->hw.ser);
in drivers/isdn/gigaset/ser-gigaset.c line 375. cs->hw.ser is of type struct ser_cardstate *.
struct ser_cardstate consists of a struct platform_device, a struct completion, an atomic_t and a pointer. No timer_list.
Quoted 1 line
[  413.561970]  [<ffffffffae74b021>] gigaset_freecshw+0xe1/0x120

Quoted 7 lines

There are functions by this name in all three Gigaset hardware dependent modules (bas_gigaset, ser_gigaset and usb_gigaset), but ...
Quoted 2 lines
[  413.562723]  [<ffffffffae70669d>] gigaset_freecs+0x2ad/0x600
[  413.564240]  [<ffffffffae74ba60>] gigaset_tty_close+0x210/0x280
this function only exists in ser_gigaset.

The platform_device embedded in struct ser_cardstate hasn't been released when you kfree() the memory it's in.

Regards,
Peter Hurley

Tilman Schmidt

2015-11-29 18:38 UTC #9 lore raw

Am 29.11.2015 um 19:22 schrieb Peter Hurley:

Quoted 10 lines · Tilman Schmidt

Quoted 9 lines
Judging from the backtrace below this must be the call
        kfree(cs->hw.ser);
in drivers/isdn/gigaset/ser-gigaset.c line 375. cs->hw.ser is of type struct ser_cardstate *.
struct ser_cardstate consists of a struct platform_device, a struct completion, an atomic_t and a pointer. No timer_list.

[...]

Quoted 3 lines

The platform_device embedded in struct ser_cardstate hasn't been released when you kfree() the memory it's in.

How so? The kfree() call quoted above is immediately preceded by:

        platform_device_unregister(&cs->hw.ser->dev);

Thanks,
Tilman

Signature · 3 lines

Tilman Schmidt                              E-Mail: [email protected]
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.

Tilman Schmidt

2015-11-29 18:47 UTC #10 lore raw

In reply to Peter Hurley

Am 29.11.2015 um 19:22 schrieb Peter Hurley:

Quoted 6 lines

Quoted 6 lines
Quoted 2 lines
[  413.536749] WARNING: CPU: 6 PID: 25400 at lib/debugobjects.c:263 debug_print_object+0x1c4/0x1e0()
[  413.538111] ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x90
This message seems to indicate that an object of type timer_list was freed which was still active. However the driver in question (ser_gigaset) does not use any timers.

[...]

Quoted 8 lines

Quoted 8 lines
Judging from the backtrace below this must be the call
        kfree(cs->hw.ser);
in drivers/isdn/gigaset/ser-gigaset.c line 375. cs->hw.ser is of type struct ser_cardstate *.
struct ser_cardstate consists of a struct platform_device, a struct completion, an atomic_t and a pointer. No timer_list.

[...]

Quoted 2 lines

The platform_device embedded in struct ser_cardstate hasn't been released when you kfree() the memory it's in.

Btw I don't see a timer_list object in struct platform_device either. Nor in the embedded struct device.

Signature · 3 lines

Tilman Schmidt                              E-Mail: [email protected]
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.

Paul Bolle

2015-11-29 20:26 UTC #11 lore raw

Quoted 2 lines · Tilman Schmidt

Btw I don't see a timer_list object in struct platform_device either. Nor in the embedded struct device.

I found two instances of struct timer_list, rather deep down struct ser_cardstate:

struct ser_cardstate {
	struct platform_device dev {
		struct device dev {
			struct kobject kobj {
			#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
				struct delayed_work release {
					struct timer_list timer;
				};
			#endif
			};
			struct dev_pm_info power {
			#ifdef CONFIG_PM
				struct timer_list suspend_timer;
			#endif
			};
		};
	};
};

(I only spotted these two and don't think there are others in the rest of the exploded struct ser_cardstate.)

If the above is correct it would be nice to know the .config of the kernel used by syzkaller.

Anyhow, without further details of the chain of events that triggered this warning, I'm afraid it will be hard to determine which struct timer_list is at the root of all this. (Ie, there's probably quite a bit of code to wade through in order to determine that.)

Thanks,

Paul Bolle

2015-11-29 23:23 UTC #12 lore raw

Quoted 7 lines · Paul Bolle

If the above is correct it would be nice to know the .config of the kernel used by syzkaller.
Anyhow, without further details of the chain of events that triggered this warning, I'm afraid it will be hard to determine which struct timer_list is at the root of all this. (Ie, there's probably quite a bit of code to wade through in order to determine that.)

I've been able to reproduce this on 4.3 with both CONFIG_DEBUG_OBJECTS_TIMERS and CONFIG_DEBUG_KOBJECT_RELEASE set. (I have not tested yet whether just CONFIG_DEBUG_OBJECTS_TIMERS is enough.)

The WARNING is triggered by doing just:
ldattach GIGASET_M101 /dev/ttyS0
killall ldattach
modprobe -r ser_gigaset

(I haven't checked whether "modprobe -r ser_gigaset" is even needed, bit I doubt that.)

Relevant part of dmesg attached at the end of this message. This should give me (and Tilman too?) an entry to get to bottom of this. Since this is relevant for anyone with just the ser-gigaset module installed, I hope to do that soon.

Thanks, again, for the report Sacha!

Paul Bolle

<6>[  167.257866] gigaset: Driver for Gigaset 307x
<6>[  167.257870] gigaset: Kernel CAPI interface
<6>[  167.260966] ser_gigaset: Serial Driver for Gigaset 307x using Siemens M101
<5>[  167.260979] kcapi: controller [001]: ser_gigaset attached
<5>[  167.261627] kcapi: controller [001] "ser_gigaset" ready.
<5>[  170.953077] kcapi: controller [001] down.
<6>[  170.953339] kobject: 'ttyGS0' (ffff8803ee191410): kobject_release, parent           (null) (delayed 2000)
<6>[  170.953347] kobject: '(null)' (ffff8803ee393500): kobject_release, parent           (null) (delayed 3000)
<6>[  170.953398] kobject: 'ser_gigaset.0' (ffff8803ee195420): kobject_release, parent           (null) (delayed 4000)
<4>[  170.953402] ------------[ cut here ]------------
<4>[  170.953410] WARNING: CPU: 2 PID: 2711 at lib/debugobjects.c:263 debug_print_object+0x87/0xb0()
<3>[  170.953416] ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x20
<5>[  170.953419] Modules linked in: ser_gigaset gigaset kernelcapi crc_ccitt fuse ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 xt_conntrack ebtable_nat ebtable_broute bridge ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw bnep iTCO_wdt iTCO_vendor_support intel_rapl iosf_mbi x86_pkg_temp_thermal coretemp uvcvideo kvm_intel snd_hda_codec_hdmi kvm videobuf2_vmalloc snd_hda_codec_conexant snd_hda_codec_generic arc4 iwldvm videobuf2_core btusb btrtl snd_hda_intel mac80211 btbcm crct10dif_pclmul crc32_pclmul crc32c_intel btintel videobuf2_memops snd_hda_codec bluetooth v4l
 2_common videodev
<5>[  170.953480]  media snd_hda_core iwlwifi snd_hwdep snd_seq snd_seq_device cfg80211 pcspkr i2c_i801 snd_pcm thinkpad_acpi wmi tpm_tis snd_timer snd rfkill tpm mei_me mei soundcore lpc_ich shpchp 8021q garp stp llc mrp i915 i2c_algo_bit drm_kms_helper drm e1000e serio_raw sdhci_pci sdhci mmc_core ptp pps_core video
<5>[  170.953519] CPU: 2 PID: 2711 Comm: ldattach Tainted: G        W       4.3.0-1.local2.fc22.x86_64 #1
<5>[  170.953522] Hardware name: LENOVO 4291V7X/4291V7X, BIOS 8DET69WW (1.39 ) 07/18/2013
<5>[  170.953525]  0000000000000000 00000000de7b6fd7 ffff8803e596f918 ffffffff8139cd6f
<5>[  170.953530]  ffff8803e596f960 ffff8803e596f950 ffffffff8109ef52 ffff880408e22d20
<5>[  170.953535]  ffffffff81c54780 ffffffff81a661b4 ffff8803e596fa30 0000000000000001
<5>[  170.953540] Call Trace:
<5>[  170.953548]  [<ffffffff8139cd6f>] dump_stack+0x44/0x55
<5>[  170.953554]  [<ffffffff8109ef52>] warn_slowpath_common+0x82/0xc0
<5>[  170.953558]  [<ffffffff8109efec>] warn_slowpath_fmt+0x5c/0x80
<5>[  170.953563]  [<ffffffff813ba0c7>] debug_print_object+0x87/0xb0
<5>[  170.953568]  [<ffffffff810b6260>] ? __queue_work+0x330/0x330
<5>[  170.953573]  [<ffffffff813bb0e6>] debug_check_no_obj_freed+0x1e6/0x250
<5>[  170.953581]  [<ffffffffa08172b8>] ? gigaset_freecshw+0x48/0x60 [ser_gigaset]
<5>[  170.953587]  [<ffffffff811ff95c>] kfree+0x10c/0x160
<5>[  170.953591]  [<ffffffffa08172b8>] gigaset_freecshw+0x48/0x60 [ser_gigaset]
<5>[  170.953598]  [<ffffffffa07fd2d8>] gigaset_freecs+0xc8/0x1d0 [gigaset]
<5>[  170.953603]  [<ffffffffa0817795>] gigaset_tty_close+0x75/0x90 [ser_gigaset]
<5>[  170.953611]  [<ffffffff8147fb78>] tty_ldisc_close.isra.1+0x38/0x50
<5>[  170.953615]  [<ffffffff8147fca8>] tty_ldisc_kill+0x18/0x90
<5>[  170.953620]  [<ffffffff814805d4>] tty_ldisc_release+0x124/0x1a0
<5>[  170.953624]  [<ffffffff81479473>] tty_release+0x3b3/0x560
<5>[  170.953632]  [<ffffffff8122036c>] __fput+0xdc/0x1e0
<5>[  170.953637]  [<ffffffff812204ae>] ____fput+0xe/0x10
<5>[  170.953641]  [<ffffffff810bb213>] task_work_run+0x73/0x90
<5>[  170.953646]  [<ffffffff810a1bb1>] do_exit+0x391/0xae0
<5>[  170.953650]  [<ffffffff810a2387>] do_group_exit+0x47/0xb0
<5>[  170.953656]  [<ffffffff810ad6c4>] get_signal+0x274/0x600
<5>[  170.953665]  [<ffffffff81015287>] do_signal+0x37/0x6b0
<5>[  170.953670]  [<ffffffff810d3108>] ? dequeue_entity+0x3b8/0xa80
<5>[  170.953676]  [<ffffffff810d6034>] ? set_next_entity+0xa4/0x880
<5>[  170.953680]  [<ffffffff810146f1>] ? __switch_to+0x261/0x4b0
<5>[  170.953686]  [<ffffffff81003b8d>] prepare_exit_to_usermode+0xbd/0x110
<5>[  170.953690]  [<ffffffff81003c35>] syscall_return_slowpath+0x55/0x150
<5>[  170.953697]  [<ffffffff817773cc>] int_ret_from_sys_call+0x25/0x8f
<4>[  170.953701] ---[ end trace c70f8fb2d5e06c74 ]---
<5>[  170.953709] kcapi: controller [001]: ser_gigaset unregistered
<6>[  174.461524] kobject: 'ser_gigaset' (ffff8803eadfac00): kobject_release, parent ffff88040b427818 (delayed 4000)
<6>[  174.461628] kobject: 'drivers' (ffff8803ee393700): kobject_release, parent ffffffffa08191d0 (delayed 1000)
<6>[  174.461632] kobject: 'holders' (ffff8803ee392700): kobject_release, parent ffffffffa08191d0 (delayed 3000)
<6>[  174.461637] kobject: 'notes' (ffff8803ee393400): kobject_release, parent ffffffffa08191d0 (delayed 2000)
<6>[  177.460222] kobject: 'ser_gigaset' (ffffffffa08191d0): kobject_release, parent ffff88040b4f5a18 (delayed 1000)
<6>[  178.472578] kobject: 'holders' (ffff8803d7653500): kobject_release, parent ffffffffa080c950 (delayed 2000)
<6>[  178.472595] kobject: 'notes' (ffff8803d7652600): kobject_release, parent ffffffffa080c950 (delayed 3000)
<6>[  181.470840] kobject: 'gigaset' (ffffffffa080c950): kobject_release, parent ffff88040b4f5a18 (delayed 2000)
<6>[  183.474016] kobject: 'holders' (ffff8800d4341e00): kobject_release, parent ffffffffa07eb050 (delayed 4000)
<6>[  183.474034] kobject: 'notes' (ffff8800d4340c00): kobject_release, parent ffffffffa07eb050 (delayed 3000)
<6>[  187.472010] kobject: 'crc_ccitt' (ffffffffa07eb050): kobject_release, parent ffff88040b4f5a18 (delayed 2000)
<6>[  189.477972] kobject: 'holders' (ffff8803e5ae7900): kobject_release, parent ffffffffa07f41d0 (delayed 1000)
<6>[  189.477989] kobject: 'notes' (ffff8803ee3a4a00): kobject_release, parent ffffffffa07f41d0 (delayed 2000)
<6>[  191.476701] kobject: 'kernelcapi' (ffffffffa07f41d0): kobject_release, parent ffff88040b4f5a18 (delayed 3000)

Paul Bolle

2015-11-30 18:01 UTC #13 lore raw

Quoted 4 lines · Paul Bolle

Relevant part of dmesg attached at the end of this message. This should give me (and Tilman too?) an entry to get to bottom of this. Since this is relevant for anyone with just the ser-gigaset module installed, I hope to do that soon.

I'm planning to send something similar to the attached draft to netdev in a few days. It fixes the issue on my machine. Sascha, does it fix this issue for syzkaller too?

Should (something like) this go into stable too?

Any further comments on that draft are appreciated too, of course.

Paul Bolle
------
[DRAFT] gigaset: don't free() a struct platform_device

One is not supposed to free() a struct platform_device. Instead one should, in the common case, only call platform_device_unregister(). That will drop the platform device's reference count. (Actually it's the reference count of the embedded kobject that is important here. But for users of platform devices that's basically irrelevant.)

So move struct platform_device dev out of struct ser_cardstate, because ser_cardstate is (malloc'ed and) free'd.

Reported-by Sasha Levin <[email protected]>

Not-yet-signed-off-by Paul Bolle <[email protected]>

---
 drivers/isdn/gigaset/ser-gigaset.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

drivers/isdn/gigaset/ser-gigaset.c +10 -9

index 375be509e95f..f8ffa253496e 100644
@@ -42,8 +42,9 @@ MODULE_PARM_DESC(cidmode, "stay in CID mode when idle");
 
 static struct gigaset_driver *driver;
 
+static struct platform_device pdev;
+
 struct ser_cardstate {
-	struct platform_device	dev;
 	struct tty_struct	*tty;
 	atomic_t		refcnt;
 	struct completion	dead_cmp;

@@ -370,8 +371,8 @@ static void gigaset_freecshw(struct cardstate *cs)
 	tasklet_kill(&cs->write_tasklet);
 	if (!cs->hw.ser)
 		return;
-	dev_set_drvdata(&cs->hw.ser->dev.dev, NULL);
-	platform_device_unregister(&cs->hw.ser->dev);
+	dev_set_drvdata(&pdev.dev, NULL);
+	platform_device_unregister(&pdev);
 	kfree(cs->hw.ser);
 	cs->hw.ser = NULL;
 }

@@ -401,17 +402,17 @@ static int gigaset_initcshw(struct cardstate *cs)
 	}
 	cs->hw.ser = scs;
 
-	cs->hw.ser->dev.name = GIGASET_MODULENAME;
-	cs->hw.ser->dev.id = cs->minor_index;
-	cs->hw.ser->dev.dev.release = gigaset_device_release;
-	rc = platform_device_register(&cs->hw.ser->dev);
+	pdev.name = GIGASET_MODULENAME;
+	pdev.id = cs->minor_index;
+	pdev.dev.release = gigaset_device_release;
+	rc = platform_device_register(&pdev);
 	if (rc != 0) {
 		pr_err("error %d registering platform device\n", rc);
 		kfree(cs->hw.ser);
 		cs->hw.ser = NULL;
 		return rc;
 	}
-	dev_set_drvdata(&cs->hw.ser->dev.dev, cs);
+	dev_set_drvdata(&pdev.dev, cs);
 
 	tasklet_init(&cs->write_tasklet,
 		     gigaset_modem_fill, (unsigned long) cs);

@@ -520,7 +521,7 @@ gigaset_tty_open(struct tty_struct *tty)
 		goto error;
 	}
 
-	cs->dev = &cs->hw.ser->dev.dev;
+	cs->dev = &pdev.dev;
 	cs->hw.ser->tty = tty;
 	atomic_set(&cs->hw.ser->refcnt, 1);
 	init_completion(&cs->hw.ser->dead_cmp);

Signature · 1 line

2.4.3

Tilman Schmidt

2015-11-30 18:30 UTC #14 lore raw

Am 30.11.2015 um 19:01 schrieb Paul Bolle:

Quoted 10 lines

[DRAFT] gigaset: don't free() a struct platform_device
One is not supposed to free() a struct platform_device. Instead one should, in the common case, only call platform_device_unregister(). That will drop the platform device's reference count. (Actually it's the reference count of the embedded kobject that is important here. But for users of platform devices that's basically irrelevant.)
So move struct platform_device dev out of struct ser_cardstate, because ser_cardstate is (malloc'ed and) free'd.

I wonder how that will behave if someone attaches two of the devices to different serial ports. Not likely, but not forbidden either.

Regards,
Tilman

Signature · 3 lines

Tilman Schmidt                              E-Mail: [email protected]
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.

Paul Bolle

2015-11-30 21:07 UTC #15 lore raw

Quoted 2 lines · Tilman Schmidt

I wonder how that will behave if someone attaches two of the devices to different serial ports. Not likely, but not forbidden either.

I see.

Perhaps I should respin and a use a pointer to a struct platform_device in struct ser_cardstate, use the two step approach of platform_device_alloc() and friends, etc. Only slightly more complicated.

How would attaching two devices work with GIGASET_MINORS hardcoded to 1? Because I haven't yet stumbled on the mechanism with which ttyGS1 (and up) would then be created.

(I do have a second M105's in a box somewhere, so I could check myself what happens when a second USB device is added, for what that's worth.)

Thanks,

Paul Bolle

Tilman Schmidt

2015-12-01 09:30 UTC #16 lore raw

Am 30.11.2015 um 22:07 schrieb Paul Bolle:

Quoted 1 line

How would attaching two devices work with GIGASET_MINORS hardcoded to 1?

Ah, it wouldn't. You'd have to recompile with a bigger GIGASET_MINORS.

(I wonder if that would actually work. Somehow I still think of GIGASET_MINORS as a configurable value, but it has never been anything but 1 in the entire in-tree history of the driver.)

Signature · 3 lines

Tilman Schmidt                              E-Mail: [email protected]
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.

Paul Bolle

2015-12-01 10:05 UTC #17 lore raw

Quoted 9 lines · Tilman Schmidt

Am 30.11.2015 um 22:07 schrieb Paul Bolle:
Quoted 2 lines
How would attaching two devices work with GIGASET_MINORS hardcoded to 1?
Ah, it wouldn't. You'd have to recompile with a bigger GIGASET_MINORS.
(I wonder if that would actually work. Somehow I still think of GIGASET_MINORS as a configurable value, but it has never been anything but 1 in the entire in-tree history of the driver.)

That ends a prolonged session of head-scratching. I'll update the commit explanation to reflect this conversation.

(Perhaps it's reasonable to wish to use two identical gigaset adapters simultaneously. But given that the gigaset drivers have never supported it, that hardly matters. So I might remove the proto-support for multiple devices one day. That's far from urgent.)

Thanks for sticking around!

Paul Bolle

Peter Hurley

2015-12-02 23:48 UTC #18 lore raw

In reply to Paul Bolle

Quoted 11 lines · Paul Bolle

Quoted 4 lines · Paul Bolle
Relevant part of dmesg attached at the end of this message. This should give me (and Tilman too?) an entry to get to bottom of this. Since this is relevant for anyone with just the ser-gigaset module installed, I hope to do that soon.
I'm planning to send something similar to the attached draft to netdev in a few days. It fixes the issue on my machine. Sascha, does it fix this issue for syzkaller too?
Should (something like) this go into stable too?

Definitely for stable since it has a userspace triggerable component.

Quoted 38 lines

Any further comments on that draft are appreciated too, of course.
Paul Bolle
------
[DRAFT] gigaset: don't free() a struct platform_device
One is not supposed to free() a struct platform_device. Instead one should, in the common case, only call platform_device_unregister(). That will drop the platform device's reference count. (Actually it's the reference count of the embedded kobject that is important here. But for users of platform devices that's basically irrelevant.)
So move struct platform_device dev out of struct ser_cardstate, because ser_cardstate is (malloc'ed and) free'd.
Reported-by Sasha Levin <[email protected]>
Not-yet-signed-off-by Paul Bolle <[email protected]>
---
 drivers/isdn/gigaset/ser-gigaset.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)
drivers/isdn/gigaset/ser-gigaset.c +2 -1
index 375be509e95f..f8ffa253496e 100644
@@ -42,8 +42,9 @@ MODULE_PARM_DESC(cidmode, "stay in CID mode when idle");
 
 static struct gigaset_driver *driver;
 
+static struct platform_device pdev;
+
 struct ser_cardstate {
-	struct platform_device	dev;
 	struct tty_struct	*tty;
 	atomic_t		refcnt;
 	struct completion	dead_cmp;

drivers/isdn/gigaset/ser-gigaset.c static void gigaset_freecshw(struct cardstate *cs) · line 376

Quoted 8 lines

drivers/isdn/gigaset/ser-gigaset.c +2 -2

@@ -370,8 +371,8 @@ static void gigaset_freecshw(struct cardstate *cs)
 	tasklet_kill(&cs->write_tasklet);
 	if (!cs->hw.ser)
 		return;
-	dev_set_drvdata(&cs->hw.ser->dev.dev, NULL);
-	platform_device_unregister(&cs->hw.ser->dev);
+	dev_set_drvdata(&pdev.dev, NULL);
+	platform_device_unregister(&pdev);
 	kfree(cs->hw.ser);

Tilman,

Is there a 1:1 correspondence and lifetime for the embedded platform device and it's containing memory?

I ask because the typical approach for device teardown is to put the kfree() in the release method; naturally, that won't work if there is some other lifetime issue.

Regards,
Peter Hurley

Quoted 35 lines

drivers/isdn/gigaset/ser-gigaset.c +6 -6

 	cs->hw.ser = NULL;
 }

@@ -401,17 +402,17 @@ static int gigaset_initcshw(struct cardstate *cs)
 	}
 	cs->hw.ser = scs;
 
-	cs->hw.ser->dev.name = GIGASET_MODULENAME;
-	cs->hw.ser->dev.id = cs->minor_index;
-	cs->hw.ser->dev.dev.release = gigaset_device_release;
-	rc = platform_device_register(&cs->hw.ser->dev);
+	pdev.name = GIGASET_MODULENAME;
+	pdev.id = cs->minor_index;
+	pdev.dev.release = gigaset_device_release;
+	rc = platform_device_register(&pdev);
 	if (rc != 0) {
 		pr_err("error %d registering platform device\n", rc);
 		kfree(cs->hw.ser);
 		cs->hw.ser = NULL;
 		return rc;
 	}
-	dev_set_drvdata(&cs->hw.ser->dev.dev, cs);
+	dev_set_drvdata(&pdev.dev, cs);
 
 	tasklet_init(&cs->write_tasklet,
 		     gigaset_modem_fill, (unsigned long) cs);

@@ -520,7 +521,7 @@ gigaset_tty_open(struct tty_struct *tty)
 		goto error;
 	}
 
-	cs->dev = &cs->hw.ser->dev.dev;
+	cs->dev = &pdev.dev;
 	cs->hw.ser->tty = tty;
 	atomic_set(&cs->hw.ser->refcnt, 1);
 	init_completion(&cs->hw.ser->dead_cmp);

Paul Bolle

2015-12-06 13:31 UTC #19 lore raw

Quoted 4 lines · Peter Hurley

Quoted 1 line · Paul Bolle
Should (something like) this go into stable too?
Definitely for stable since it has a userspace triggerable component.

Thanks, will do.

Quoted 27 lines

Quoted 15 lines

drivers/isdn/gigaset/ser-gigaset.c +2 -1

@@ -42,8 +42,9 @@ MODULE_PARM_DESC(cidmode, "stay in CID mode when
 idle");
 
 static struct gigaset_driver *driver;
 
+static struct platform_device pdev;
+
 struct ser_cardstate {
-	struct platform_device	dev;
 	struct tty_struct	*tty;
 	atomic_t		refcnt;

drivers/isdn/gigaset/ser-gigaset.c +0 -0

 	struct completion	dead_cmp;

drivers/isdn/gigaset/ser-gigaset.c static void gigaset_freecshw(struct cardstate · line 377

Quoted 9 lines

drivers/isdn/gigaset/ser-gigaset.c +2 -2

@@ -370,8 +371,8 @@ static void gigaset_freecshw(struct cardstate
 *cs)
 	tasklet_kill(&cs->write_tasklet);
 	if (!cs->hw.ser)
 		return;
-	dev_set_drvdata(&cs->hw.ser->dev.dev, NULL);
-	platform_device_unregister(&cs->hw.ser->dev);
+	dev_set_drvdata(&pdev.dev, NULL);
+	platform_device_unregister(&pdev);
 	kfree(cs->hw.ser);

Tilman,

Quoted 2 lines

Is there a 1:1 correspondence and lifetime for the embedded platform device and it's containing memory?

(Haven't heard from Tilman, so I'll give this a try.)

That containing memory is a struct ser_cardstate. And currently instances of struct _ser_cardstate are malloced and freed in routines that also call platform_device_register() and platform_device_unregister(). So yes, I think there's a 1:1 correspondence.

Quoted 2 lines

I ask because the typical approach for device teardown is to put the kfree() in the release method;

(Side note: the (struct device) release method of this driver -gigaset_device_release() - is actually a nop. It only frees device ->platform_data and platform_device->resource, but neither are actually
used: they remain NULL through their entire life.)

Quoted 2 lines

naturally, that won't work if there
is some other lifetime issue.

I'm not sure I follow what you mean here. Could you point me at a driver that uses that approach, so that I can have a look at it?

Thanks,

Paul Bolle

Tilman Schmidt

2015-12-06 15:29 UTC #20 lore raw

Am 06.12.2015 um 14:31 schrieb Paul Bolle:

Quoted 30 lines · Peter Hurley

Quoted 29 lines · Paul Bolle

Quoted 16 lines

drivers/isdn/gigaset/ser-gigaset.c +2 -1

@@ -42,8 +42,9 @@ MODULE_PARM_DESC(cidmode, "stay in CID mode when
 idle");
 
 static struct gigaset_driver *driver;
 
+static struct platform_device pdev;
+
 struct ser_cardstate {
-	struct platform_device	dev;
 	struct tty_struct	*tty;
 	atomic_t		refcnt;

drivers/isdn/gigaset/ser-gigaset.c +0 -0

 	struct completion	dead_cmp;

drivers/isdn/gigaset/ser-gigaset.c static void gigaset_freecshw(struct cardstate · line 377

Quoted 9 lines

drivers/isdn/gigaset/ser-gigaset.c +2 -2

@@ -370,8 +371,8 @@ static void gigaset_freecshw(struct cardstate
 *cs)
 	tasklet_kill(&cs->write_tasklet);
 	if (!cs->hw.ser)
 		return;
-	dev_set_drvdata(&cs->hw.ser->dev.dev, NULL);
-	platform_device_unregister(&cs->hw.ser->dev);
+	dev_set_drvdata(&pdev.dev, NULL);
+	platform_device_unregister(&pdev);
 	kfree(cs->hw.ser);

Tilman,

Quoted 4 lines

Quoted 2 lines
Is there a 1:1 correspondence and lifetime for the embedded platform device and it's containing memory?
(Haven't heard from Tilman, so I'll give this a try.)

Sorry for that. Been busy.

Quoted 5 lines

That containing memory is a struct ser_cardstate. And currently instances of struct _ser_cardstate are malloced and freed in routines that also call platform_device_register() and platform_device_unregister(). So yes, I think there's a 1:1 correspondence.

Correct.

Quoted 7 lines

Quoted 2 lines
I ask because the typical approach for device teardown is to put the kfree() in the release method;
(Side note: the (struct device) release method of this driver -gigaset_device_release() - is actually a nop. It only frees device ->platform_data and platform_device->resource, but neither are actually
used: they remain NULL through their entire life.)

Yeah, that was just copied unthinkingly from driver/base/platform.c.

So the solution might be as simple as moving the kfree() call from gigaset_freecshw() to gigaset_device_release(). Something like this:

drivers/isdn/gigaset/ser-gigaset.c +1 -5

@@ -370,19 +370,18 @@ static void gigaset_freecshw(struct cardstate *cs)
        tasklet_kill(&cs->write_tasklet);
        if (!cs->hw.ser)
                return;
-       dev_set_drvdata(&cs->hw.ser->dev.dev, NULL);
        platform_device_unregister(&cs->hw.ser->dev);
-       kfree(cs->hw.ser);
-       cs->hw.ser = NULL;
 }
 
 static void gigaset_device_release(struct device *dev)
 {
-       struct platform_device *pdev = to_platform_device(dev);
+       struct cardstate *cs = dev_get_drvdata(dev);
 
-       /* adapted from platform_device_release() in

drivers/base/platform.c */

-       kfree(dev->platform_data);
-       kfree(pdev->resource);
+       if (!cs)
+               return;
+       dev_set_drvdata(dev, NULL);
+       kfree(cs->hw.ser);
+       cs->hw.ser = NULL;

}

(Off the top of my hat, completely untested, don't even know if that will compile.)

Signature · 3 lines

Tilman Schmidt                              E-Mail: [email protected]
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.

Paul Bolle

2015-12-06 20:12 UTC #21 lore raw

Quoted 23 lines · Tilman Schmidt

So the solution might be as simple as moving the kfree() call from gigaset_freecshw() to gigaset_device_release(). Something like this:

drivers/isdn/gigaset/ser-gigaset.c +1 -5

@@ -370,19 +370,18 @@ static void gigaset_freecshw(struct cardstate
 *cs)
        tasklet_kill(&cs->write_tasklet);
        if (!cs->hw.ser)
                return;
-       dev_set_drvdata(&cs->hw.ser->dev.dev, NULL);
        platform_device_unregister(&cs->hw.ser->dev);
-       kfree(cs->hw.ser);
-       cs->hw.ser = NULL;
 }
 
 static void gigaset_device_release(struct device *dev)
 {
-       struct platform_device *pdev = to_platform_device(dev);
+       struct cardstate *cs = dev_get_drvdata(dev);
 
-       /* adapted from platform_device_release() in

drivers/base/platform.c */

drivers/isdn/gigaset/ser-gigaset.c

Quoted 8 lines

drivers/isdn/gigaset/ser-gigaset.c +5 -2

-       kfree(dev->platform_data);
-       kfree(pdev->resource);
+       if (!cs)
+               return;
+       dev_set_drvdata(dev, NULL);
+       kfree(cs->hw.ser);
+       cs->hw.ser = NULL;
 }

This solution assumes that the struct platform_device is moved out of the struct ser_cardstate, doesn't it? In other words, this is something to do on top of my (draft) patch. Otherwise we'd still be freeing memory managed through reference counting.

Thanks,

Paul Bolle

Tilman Schmidt

2015-12-07 09:27 UTC #22 lore raw

Am 06.12.2015 um 21:12 schrieb Paul Bolle:

Quoted 22 lines · Tilman Schmidt

Quoted 21 lines

So the solution might be as simple as moving the kfree() call from gigaset_freecshw() to gigaset_device_release(). Something like this:

drivers/isdn/gigaset/ser-gigaset.c +1 -4

@@ -370,19 +370,18 @@ static void gigaset_freecshw(struct cardstate
 *cs)
        tasklet_kill(&cs->write_tasklet);
        if (!cs->hw.ser)
                return;
-       dev_set_drvdata(&cs->hw.ser->dev.dev, NULL);
        platform_device_unregister(&cs->hw.ser->dev);
-       kfree(cs->hw.ser);
-       cs->hw.ser = NULL;
 }
 
 static void gigaset_device_release(struct device *dev)
 {
-       struct platform_device *pdev = to_platform_device(dev);
+       struct cardstate *cs = dev_get_drvdata(dev);

Quoted 13 lines

drivers/isdn/gigaset/ser-gigaset.c static void gigaset_freecshw(struct cardstate · line 386
Quoted 9 lines
drivers/isdn/gigaset/ser-gigaset.c +5 -3
-       /* adapted from platform_device_release() in drivers/base/platform.c */
-       kfree(dev->platform_data);
-       kfree(pdev->resource);
+       if (!cs)
+               return;
+       dev_set_drvdata(dev, NULL);
+       kfree(cs->hw.ser);
+       cs->hw.ser = NULL;
 }
This solution assumes that the struct platform_device is moved out of the struct ser_cardstate, doesn't it? In other words, this is something to do on top of my (draft) patch.

No, that wasn't my intention. I thought of that solution as an alternative, not an increment to your patch.

Quoted 2 lines

Otherwise we'd still be freeing memory
managed through reference counting.

Now I#m confused. I thought by following Peter's suggestion to put the kfree() in the release method we'd avoid just that.

Regards,
Tilman

Signature · 3 lines

Tilman Schmidt                              E-Mail: [email protected]
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.

Paul Bolle

2015-12-07 12:25 UTC #23 lore raw

[Re-added mailinglist that got dropped somehow.]

Quoted 7 lines · Tilman Schmidt

Am 06.12.2015 um 21:12 schrieb Paul Bolle:
Quoted 5 lines
This solution assumes that the struct platform_device is moved out of
the struct ser_cardstate, doesn't it? In other words, this is something
to do on top of my (draft) patch.

Quoted 8 lines

No, that wasn't my intention. I thought of that solution as an alternative, not an increment to your patch.
Quoted 2 lines
Otherwise we'd still be freeing memory
managed through reference counting.
Now I#m confused. I thought by following Peter's suggestion to put the kfree() in the release method we'd avoid just that.

(Your patch compiles just fine.)

Apparently it does, because I can't trigger the WARNING we're discussing here with your patch applied. I'll have to dive into this stuff again, because apparently my mental model of what's going on is incomplete at best.

In the mean time you might want to turn your patch into something that can actually be applied (with or without my Sign-off or Ack; I don't care how it finds its way into the tree). Please add add

    Fixes: 2869b23e4b95 ("[PATCH] drivers/isdn/gigaset: new M101 driver (v2)")

(Perhaps with a comment that v2.6.32 needs a trivial context change; I'm not sure how that needs to be communicated.)

But I'm fine with cobbling together a commit explanation myself if you're too busy right now.

Thanks,

Paul Bolle

Tilman Schmidt

2015-12-07 18:40 UTC #24 lore raw

Am 07.12.2015 um 13:25 schrieb Paul Bolle:

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Otherwise we'd still be freeing memory
managed through reference counting.
Now I#m confused. I thought by following Peter's suggestion to put the kfree() in the release method we'd avoid just that.
Apparently it does, because I can't trigger the WARNING we're discussing here with your patch applied.

Nice.

Quoted 3 lines

I'll have to dive into this stuff again,
because apparently my mental model of what's going on is incomplete at best.

I won't claim anything like completeness for mine.

Quoted 4 lines

In the mean time you might want to turn your patch into something that can actually be applied (with or without my Sign-off or Ack; I don't care how it finds its way into the tree). Please add add
    Fixes: 2869b23e4b95 ("[PATCH] drivers/isdn/gigaset: new M101 driver (v2)")

Will do. (Not today, though.)

Regards,
Tilman

Signature · 3 lines

Tilman Schmidt                              E-Mail: [email protected]
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.