gigaset: freeing an active object

[Sasha Levin]

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

[Paul Bolle]

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

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]

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

Right, no related hardware, inside a VM.


Thanks,
Sasha

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

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

[Peter Hurley]

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

[Sasha Levin]

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

[Dmitry Vyukov]

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]

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: til...@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.

[Peter Hurley]

Hi Tilman,

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]

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);

[Tilman Schmidt]

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.

[Paul Bolle]

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

[Paul Bolle]

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)

[Paul Bolle]

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...@oracle.com>
Not-yet-signed-off-by: Paul Bolle <peb...@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

[Tilman Schmidt]

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

[Paul Bolle]

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

[Tilman Schmidt]

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

[Paul Bolle]

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

[Peter Hurley]

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.

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

[Paul Bolle]

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.

(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

[Tilman Schmidt]

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

[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. Otherwise we'd still be freeing memory
managed through reference counting.

Thanks,


Paul Bolle

[Tilman Schmidt]

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

[Paul Bolle]

[Re-added mailinglist that got dropped somehow.]

(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]

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