* gigaset: freeing an active object @ 2015-11-27 15:19 Sasha Levin 2015-11-27 17:57 ` Paul Bolle 2015-11-29 15:30 ` Tilman Schmidt 0 siblings, 2 replies; 26+ messages in thread From: Sasha Levin @ 2015-11-27 15:19 UTC (permalink / raw) To: pebolle, isdn; +Cc: davem, tilman, gigaset307x-common, LKML, netdev, syzkaller Hi, Fuzzing with syzkaller on the latest -next kernel produced this error: [ 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 [ 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, Sasha ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-27 15:19 gigaset: freeing an active object Sasha Levin @ 2015-11-27 17:57 ` Paul Bolle 2015-11-27 18:15 ` Sasha Levin 2015-11-29 15:30 ` Tilman Schmidt 1 sibling, 1 reply; 26+ messages in thread From: Paul Bolle @ 2015-11-27 17:57 UTC (permalink / raw) To: Sasha Levin Cc: isdn, davem, tilman, gigaset307x-common, LKML, netdev, syzkaller Hi Sascha, On vr, 2015-11-27 at 10:19 -0500, Sasha Levin wrote: > Fuzzing with syzkaller on the latest -next kernel produced this error: (syzkaller is new to me. I'll have to do some web searches.) > [ 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 > [ 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-27 17:57 ` Paul Bolle @ 2015-11-27 18:15 ` Sasha Levin 2015-11-28 0:28 ` Paul Bolle 0 siblings, 1 reply; 26+ messages in thread From: Sasha Levin @ 2015-11-27 18:15 UTC (permalink / raw) To: Paul Bolle Cc: isdn, davem, tilman, gigaset307x-common, LKML, netdev, syzkaller On 11/27/2015 12:57 PM, Paul Bolle wrote: > Hi Sascha, > > On vr, 2015-11-27 at 10:19 -0500, Sasha Levin wrote: >> 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 >> [ 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 >> [ 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.) Right, no related hardware, inside a VM. Thanks, Sasha ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-27 18:15 ` Sasha Levin @ 2015-11-28 0:28 ` Paul Bolle 2015-11-28 1:20 ` Peter Hurley 0 siblings, 1 reply; 26+ messages in thread From: Paul Bolle @ 2015-11-28 0:28 UTC (permalink / raw) To: Sasha Levin Cc: isdn, davem, tilman, gigaset307x-common, LKML, netdev, syzkaller (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.) On vr, 2015-11-27 at 13:15 -0500, Sasha Levin wrote: > On 11/27/2015 12:57 PM, Paul Bolle wrote: > > On vr, 2015-11-27 at 10:19 -0500, Sasha Levin wrote: > > > 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. > > > [ 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. > > > [ 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. > > > [ 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 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. > > > [ 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 > > > [ 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-28 0:28 ` Paul Bolle @ 2015-11-28 1:20 ` Peter Hurley 2015-11-28 1:27 ` Sasha Levin 0 siblings, 1 reply; 26+ messages in thread From: Peter Hurley @ 2015-11-28 1:20 UTC (permalink / raw) To: Paul Bolle, Sasha Levin Cc: isdn, davem, tilman, gigaset307x-common, LKML, netdev, syzkaller On 11/27/2015 07:28 PM, Paul Bolle wrote: > (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.) > > On vr, 2015-11-27 at 13:15 -0500, Sasha Levin wrote: >> On 11/27/2015 12:57 PM, Paul Bolle wrote: >>> On vr, 2015-11-27 at 10:19 -0500, Sasha Levin wrote: >>>> 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. > >>>> [ 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. > >>>> [ 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 > which would trigger the use of ser-gigaset. > >>>> [ 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 > > 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. > >>>> [ 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 >>>> [ 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... ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-28 1:20 ` Peter Hurley @ 2015-11-28 1:27 ` Sasha Levin 2015-11-29 14:36 ` Dmitry Vyukov 0 siblings, 1 reply; 26+ messages in thread From: Sasha Levin @ 2015-11-28 1:27 UTC (permalink / raw) To: Peter Hurley, Paul Bolle, Dmitry Vyukov Cc: isdn, davem, tilman, gigaset307x-common, LKML, netdev, syzkaller On 11/27/2015 08:20 PM, Peter Hurley wrote: > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-28 1:27 ` Sasha Levin @ 2015-11-29 14:36 ` Dmitry Vyukov 0 siblings, 0 replies; 26+ messages in thread From: Dmitry Vyukov @ 2015-11-29 14:36 UTC (permalink / raw) To: Sasha Levin Cc: Peter Hurley, Paul Bolle, isdn, David Miller, tilman, gigaset307x-common, LKML, netdev, syzkaller On Sat, Nov 28, 2015 at 2:27 AM, Sasha Levin <sasha.levin@oracle.com> wrote: > On 11/27/2015 08:20 PM, Peter Hurley wrote: >> 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? 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... ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-27 15:19 gigaset: freeing an active object Sasha Levin 2015-11-27 17:57 ` Paul Bolle @ 2015-11-29 15:30 ` Tilman Schmidt 2015-11-29 18:22 ` Peter Hurley 1 sibling, 1 reply; 26+ messages in thread From: Tilman Schmidt @ 2015-11-29 15:30 UTC (permalink / raw) To: Sasha Levin Cc: pebolle, isdn, davem, gigaset307x-common, LKML, netdev, syzkaller [-- Attachment #1: Type: text/plain, Size: 2728 bytes --] 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: > 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? > [ 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? > [ 413.539598] Modules linked in:3470693efef57268844f02f5de3ab392d8cf5e209671ddd87163cb964c510659 This message does not tell me anything. What does that hex string after the colon mean? > [ 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. > [ 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 ... > [ 413.562723] [<ffffffffae70669d>] gigaset_freecs+0x2ad/0x600 > [ 413.564240] [<ffffffffae74ba60>] gigaset_tty_close+0x210/0x280 this function only exists in ser_gigaset. Thanks, Tilman -- Tilman Schmidt E-Mail: tilman@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-29 15:30 ` Tilman Schmidt @ 2015-11-29 18:22 ` Peter Hurley 2015-11-29 18:38 ` Tilman Schmidt 2015-11-29 18:47 ` Tilman Schmidt 0 siblings, 2 replies; 26+ messages in thread From: Peter Hurley @ 2015-11-29 18:22 UTC (permalink / raw) To: Tilman Schmidt, Sasha Levin Cc: pebolle, isdn, davem, gigaset307x-common, LKML, netdev, syzkaller Hi Tilman, On 11/29/2015 10:30 AM, Tilman Schmidt wrote: > 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: >> 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? > >> [ 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? > >> [ 413.539598] Modules linked in:3470693efef57268844f02f5de3ab392d8cf5e209671ddd87163cb964c510659 > > This message does not tell me anything. What does that hex string after > the colon mean? > >> [ 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. > >> [ 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 ... > >> [ 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-29 18:22 ` Peter Hurley @ 2015-11-29 18:38 ` Tilman Schmidt 2015-11-29 18:47 ` Tilman Schmidt 1 sibling, 0 replies; 26+ messages in thread From: Tilman Schmidt @ 2015-11-29 18:38 UTC (permalink / raw) To: Peter Hurley, Sasha Levin Cc: pebolle, isdn, davem, gigaset307x-common, LKML, netdev, syzkaller [-- Attachment #1: Type: text/plain, Size: 853 bytes --] Am 29.11.2015 um 19:22 schrieb Peter Hurley: > On 11/29/2015 10:30 AM, Tilman Schmidt wrote: >> >> 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. [...] > > 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 -- Tilman Schmidt E-Mail: tilman@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-29 18:22 ` Peter Hurley 2015-11-29 18:38 ` Tilman Schmidt @ 2015-11-29 18:47 ` Tilman Schmidt 2015-11-29 20:26 ` Paul Bolle 1 sibling, 1 reply; 26+ messages in thread From: Tilman Schmidt @ 2015-11-29 18:47 UTC (permalink / raw) To: Peter Hurley, Sasha Levin Cc: pebolle, isdn, davem, gigaset307x-common, LKML, netdev, syzkaller [-- Attachment #1: Type: text/plain, Size: 1178 bytes --] Am 29.11.2015 um 19:22 schrieb Peter Hurley: >>> [ 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. [...] >> 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. [...] > 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. -- Tilman Schmidt E-Mail: tilman@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-29 18:47 ` Tilman Schmidt @ 2015-11-29 20:26 ` Paul Bolle 2015-11-29 23:23 ` Paul Bolle 0 siblings, 1 reply; 26+ messages in thread From: Paul Bolle @ 2015-11-29 20:26 UTC (permalink / raw) To: Tilman Schmidt, Peter Hurley, Sasha Levin Cc: isdn, davem, gigaset307x-common, LKML, netdev, syzkaller On zo, 2015-11-29 at 19:47 +0100, Tilman Schmidt wrote: > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-29 20:26 ` Paul Bolle @ 2015-11-29 23:23 ` Paul Bolle 0 siblings, 0 replies; 26+ messages in thread From: Paul Bolle @ 2015-11-29 23:23 UTC (permalink / raw) To: Tilman Schmidt, Peter Hurley, Sasha Levin Cc: isdn, davem, gigaset307x-common, LKML, netdev, syzkaller On zo, 2015-11-29 at 21:26 +0100, Paul Bolle wrote: > 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 v4l2_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) ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object @ 2015-11-29 23:23 ` Paul Bolle 0 siblings, 0 replies; 26+ messages in thread From: Paul Bolle @ 2015-11-29 23:23 UTC (permalink / raw) To: Tilman Schmidt, Peter Hurley, Sasha Levin Cc: isdn, davem, gigaset307x-common, LKML, netdev, syzkaller On zo, 2015-11-29 at 21:26 +0100, Paul Bolle wrote: > 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) ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-29 23:23 ` Paul Bolle (?) @ 2015-11-30 18:01 ` Paul Bolle 2015-11-30 18:30 ` Tilman Schmidt 2015-12-02 23:48 ` Peter Hurley -1 siblings, 2 replies; 26+ messages in thread From: Paul Bolle @ 2015-11-30 18:01 UTC (permalink / raw) To: Tilman Schmidt, Peter Hurley, Sasha Levin Cc: isdn, davem, gigaset307x-common, LKML, netdev, syzkaller On ma, 2015-11-30 at 00:23 +0100, Paul Bolle wrote: > 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 <sasha.levin@oracle.com> Not-yet-signed-off-by: Paul Bolle <pebolle@tiscali.nl> --- drivers/isdn/gigaset/ser-gigaset.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c index 375be509e95f..f8ffa253496e 100644 --- a/drivers/isdn/gigaset/ser-gigaset.c +++ b/drivers/isdn/gigaset/ser-gigaset.c @@ -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); -- 2.4.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-30 18:01 ` Paul Bolle @ 2015-11-30 18:30 ` Tilman Schmidt 2015-11-30 21:07 ` Paul Bolle 2015-12-02 23:48 ` Peter Hurley 1 sibling, 1 reply; 26+ messages in thread From: Tilman Schmidt @ 2015-11-30 18:30 UTC (permalink / raw) To: Paul Bolle, Peter Hurley, Sasha Levin Cc: isdn, davem, gigaset307x-common, LKML, netdev, syzkaller [-- Attachment #1: Type: text/plain, Size: 890 bytes --] Am 30.11.2015 um 19:01 schrieb 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. 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 -- Tilman Schmidt E-Mail: tilman@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-30 18:30 ` Tilman Schmidt @ 2015-11-30 21:07 ` Paul Bolle 2015-12-01 9:30 ` Tilman Schmidt 0 siblings, 1 reply; 26+ messages in thread From: Paul Bolle @ 2015-11-30 21:07 UTC (permalink / raw) To: Tilman Schmidt, Peter Hurley, Sasha Levin Cc: isdn, davem, gigaset307x-common, LKML, netdev, syzkaller On ma, 2015-11-30 at 19:30 +0100, Tilman Schmidt wrote: > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-30 21:07 ` Paul Bolle @ 2015-12-01 9:30 ` Tilman Schmidt 2015-12-01 10:05 ` Paul Bolle 0 siblings, 1 reply; 26+ messages in thread From: Tilman Schmidt @ 2015-12-01 9:30 UTC (permalink / raw) To: Paul Bolle, Peter Hurley Cc: Sasha Levin, isdn, davem, gigaset307x-common, LKML, netdev, syzkaller [-- Attachment #1: Type: text/plain, Size: 537 bytes --] Am 30.11.2015 um 22:07 schrieb Paul Bolle: > 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.) -- Tilman Schmidt E-Mail: tilman@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-12-01 9:30 ` Tilman Schmidt @ 2015-12-01 10:05 ` Paul Bolle 0 siblings, 0 replies; 26+ messages in thread From: Paul Bolle @ 2015-12-01 10:05 UTC (permalink / raw) To: Tilman Schmidt, Peter Hurley Cc: Sasha Levin, isdn, davem, gigaset307x-common, LKML, netdev, syzkaller On di, 2015-12-01 at 10:30 +0100, Tilman Schmidt wrote: > Am 30.11.2015 um 22:07 schrieb Paul Bolle: > > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-11-30 18:01 ` Paul Bolle 2015-11-30 18:30 ` Tilman Schmidt @ 2015-12-02 23:48 ` Peter Hurley 2015-12-06 13:31 ` Paul Bolle 1 sibling, 1 reply; 26+ messages in thread From: Peter Hurley @ 2015-12-02 23:48 UTC (permalink / raw) To: Paul Bolle, Tilman Schmidt, Sasha Levin Cc: isdn, davem, gigaset307x-common, LKML, netdev, syzkaller On 11/30/2015 01:01 PM, Paul Bolle wrote: > On ma, 2015-11-30 at 00:23 +0100, Paul Bolle wrote: >> 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. > 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 <sasha.levin@oracle.com> > Not-yet-signed-off-by: Paul Bolle <pebolle@tiscali.nl> > --- > drivers/isdn/gigaset/ser-gigaset.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c > index 375be509e95f..f8ffa253496e 100644 > --- a/drivers/isdn/gigaset/ser-gigaset.c > +++ b/drivers/isdn/gigaset/ser-gigaset.c > @@ -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); 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 > 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); > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-12-02 23:48 ` Peter Hurley @ 2015-12-06 13:31 ` Paul Bolle 2015-12-06 15:29 ` Tilman Schmidt 0 siblings, 1 reply; 26+ messages in thread From: Paul Bolle @ 2015-12-06 13:31 UTC (permalink / raw) To: Peter Hurley, Tilman Schmidt, Sasha Levin Cc: isdn, davem, gigaset307x-common, LKML, netdev, syzkaller On wo, 2015-12-02 at 18:48 -0500, Peter Hurley wrote: > On 11/30/2015 01:01 PM, Paul Bolle wrote: > > Should (something like) this go into stable too? > > Definitely for stable since it has a userspace triggerable component. Thanks, will do. > > --- a/drivers/isdn/gigaset/ser-gigaset.c > > +++ b/drivers/isdn/gigaset/ser-gigaset.c > > @@ -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); > > Tilman, > > 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. > 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.) > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-12-06 13:31 ` Paul Bolle @ 2015-12-06 15:29 ` Tilman Schmidt 2015-12-06 20:12 ` Paul Bolle 0 siblings, 1 reply; 26+ messages in thread From: Tilman Schmidt @ 2015-12-06 15:29 UTC (permalink / raw) To: Paul Bolle, Peter Hurley, Sasha Levin Cc: isdn, davem, gigaset307x-common, LKML, netdev, syzkaller [-- Attachment #1: Type: text/plain, Size: 3207 bytes --] Am 06.12.2015 um 14:31 schrieb Paul Bolle: > On wo, 2015-12-02 at 18:48 -0500, Peter Hurley wrote: >> On 11/30/2015 01:01 PM, Paul Bolle wrote: >>> --- a/drivers/isdn/gigaset/ser-gigaset.c >>> +++ b/drivers/isdn/gigaset/ser-gigaset.c >>> @@ -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); >> >> Tilman, >> >> 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. > 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. >> 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: --- a/drivers/isdn/gigaset/ser-gigaset.c +++ b/drivers/isdn/gigaset/ser-gigaset.c @@ -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.) -- Tilman Schmidt E-Mail: tilman@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-12-06 15:29 ` Tilman Schmidt @ 2015-12-06 20:12 ` Paul Bolle 2015-12-07 9:27 ` Tilman Schmidt 0 siblings, 1 reply; 26+ messages in thread From: Paul Bolle @ 2015-12-06 20:12 UTC (permalink / raw) To: Tilman Schmidt, Peter Hurley, Sasha Levin Cc: isdn, davem, gigaset307x-common, LKML, netdev, syzkaller On zo, 2015-12-06 at 16:29 +0100, Tilman Schmidt wrote: > So the solution might be as simple as moving the kfree() call from > gigaset_freecshw() to gigaset_device_release(). Something like this: > > --- a/drivers/isdn/gigaset/ser-gigaset.c > +++ b/drivers/isdn/gigaset/ser-gigaset.c > @@ -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; > } 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-12-06 20:12 ` Paul Bolle @ 2015-12-07 9:27 ` Tilman Schmidt 2015-12-07 12:25 ` Paul Bolle 0 siblings, 1 reply; 26+ messages in thread From: Tilman Schmidt @ 2015-12-07 9:27 UTC (permalink / raw) To: Paul Bolle, Peter Hurley, Sasha Levin Cc: isdn, davem, gigaset307x-common, LKML, netdev, syzkaller [-- Attachment #1: Type: text/plain, Size: 1907 bytes --] Am 06.12.2015 um 21:12 schrieb Paul Bolle: > On zo, 2015-12-06 at 16:29 +0100, Tilman Schmidt wrote: >> So the solution might be as simple as moving the kfree() call from >> gigaset_freecshw() to gigaset_device_release(). Something like this: >> >> --- a/drivers/isdn/gigaset/ser-gigaset.c >> +++ b/drivers/isdn/gigaset/ser-gigaset.c >> @@ -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; >> } > > 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. > 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 -- Tilman Schmidt E-Mail: tilman@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-12-07 9:27 ` Tilman Schmidt @ 2015-12-07 12:25 ` Paul Bolle 2015-12-07 18:40 ` Tilman Schmidt 0 siblings, 1 reply; 26+ messages in thread From: Paul Bolle @ 2015-12-07 12:25 UTC (permalink / raw) To: Tilman Schmidt, Peter Hurley, Sasha Levin Cc: isdn, davem, gigaset307x-common, LKML, netdev, syzkaller [Re-added mailinglist that got dropped somehow.] On ma, 2015-12-07 at 10:27 +0100, Tilman Schmidt wrote: > Am 06.12.2015 um 21:12 schrieb Paul Bolle: > > 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. > > > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: gigaset: freeing an active object 2015-12-07 12:25 ` Paul Bolle @ 2015-12-07 18:40 ` Tilman Schmidt 0 siblings, 0 replies; 26+ messages in thread From: Tilman Schmidt @ 2015-12-07 18:40 UTC (permalink / raw) To: Paul Bolle, Peter Hurley, Sasha Levin Cc: isdn, davem, gigaset307x-common, LKML, netdev, syzkaller [-- Attachment #1: Type: text/plain, Size: 1065 bytes --] Am 07.12.2015 um 13:25 schrieb Paul Bolle: >>> 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. > 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. > 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 -- Tilman Schmidt E-Mail: tilman@imap.cc Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2015-12-07 18:41 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-27 15:19 gigaset: freeing an active object Sasha Levin 2015-11-27 17:57 ` Paul Bolle 2015-11-27 18:15 ` Sasha Levin 2015-11-28 0:28 ` Paul Bolle 2015-11-28 1:20 ` Peter Hurley 2015-11-28 1:27 ` Sasha Levin 2015-11-29 14:36 ` Dmitry Vyukov 2015-11-29 15:30 ` Tilman Schmidt 2015-11-29 18:22 ` Peter Hurley 2015-11-29 18:38 ` Tilman Schmidt 2015-11-29 18:47 ` Tilman Schmidt 2015-11-29 20:26 ` Paul Bolle 2015-11-29 23:23 ` Paul Bolle 2015-11-29 23:23 ` Paul Bolle 2015-11-30 18:01 ` Paul Bolle 2015-11-30 18:30 ` Tilman Schmidt 2015-11-30 21:07 ` Paul Bolle 2015-12-01 9:30 ` Tilman Schmidt 2015-12-01 10:05 ` Paul Bolle 2015-12-02 23:48 ` Peter Hurley 2015-12-06 13:31 ` Paul Bolle 2015-12-06 15:29 ` Tilman Schmidt 2015-12-06 20:12 ` Paul Bolle 2015-12-07 9:27 ` Tilman Schmidt 2015-12-07 12:25 ` Paul Bolle 2015-12-07 18:40 ` Tilman Schmidt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.