* [PATCH] tty: fix tty->ldisc leak on ENODEV from driver install
@ 2011-03-23 0:45 Julian Anastasov
2011-03-23 8:12 ` Jiri Slaby
0 siblings, 1 reply; 3+ messages in thread
From: Julian Anastasov @ 2011-03-23 0:45 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-kernel
When a USB serial device is not attached I see
open() to leak 8-byte structures:
modprobe usbserial
[ -e /dev/ttyUSB0 ] && cat /dev/ttyUSB0
cat: /dev/ttyUSB0: No such device
Note that error must be ENODEV with usbserial
loaded and when name exists, not ENXIO.
kmemleak shows such output:
unreferenced object 0xc0e19860 (size 8):
comm cat, pid 1226, jiffies 4294919464 (age 287.476s)
hex dump (first 8 bytes):
44 de 2d c1 01 00 00 00 D.-.....
backtrace:
[<c1065a74>] create_object+0x109/0x1ad
[<c1063d2b>] kmem_cache_alloc+0x60/0x68
[<c113505c>] tty_ldisc_get+0x54/0x76
[<c11358c9>] tty_ldisc_init+0xa/0x20
[<c1130ab4>] initialize_tty_struct+0x2d/0x1ac
[<c1130c8c>] tty_init_dev+0x59/0x10d
[<c113136d>] tty_open+0x24a/0x3a2
[<c1069516>] chrdev_open+0xd1/0xef
[<c106d2d5>] nameidata_drop_rcu_last+0x3b/0x49
[<c1069445>] chrdev_open+0x0/0xef
[<c1065d42>] __dentry_open.clone.15+0xec/0x1c3
[<c10669a5>] nameidata_to_filp+0x2a/0x33
[<c106f7c7>] finish_open+0x6e/0xfc
[<c106fbda>] do_filp_open+0x144/0x4af
[<c1076f55>] alloc_fd+0x41/0xa5
[<c10669ef>] do_sys_open+0x41/0xc3
Looking at tty_init_dev() it seems initialize_tty_struct()
attaches tty->ldisc via tty_ldisc_init() but on
tty_driver_install_tty() failure (-ENODEV) we call free_tty_struct()
which does nothing with tty->ldisc.
The appended patch fixes the leak but I'm not
sure what tty->ldisc value we can see in release_one_tty(),
I assume if ldisc is freed tty->ldisc should be NULL, so
free_tty_struct() will not try to double-free the ldisc.
Signed-off-by: Julian Anastasov <ja@ssi.bg>
---
drivers/tty/tty_io.c | 1 +
1 file changed, 1 insertion(+)
--- linux-2.6.38/drivers/tty/tty_io.c
+++ linux-2.6.38/drivers/tty/tty_io.c
@@ -188,6 +188,7 @@ void free_tty_struct(struct tty_struct *
put_device(tty->dev);
kfree(tty->write_buf);
tty_buffer_free_all(tty);
+ kfree(tty->ldisc);
kfree(tty);
}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] tty: fix tty->ldisc leak on ENODEV from driver install
2011-03-23 0:45 [PATCH] tty: fix tty->ldisc leak on ENODEV from driver install Julian Anastasov
@ 2011-03-23 8:12 ` Jiri Slaby
2011-03-23 8:49 ` Julian Anastasov
0 siblings, 1 reply; 3+ messages in thread
From: Jiri Slaby @ 2011-03-23 8:12 UTC (permalink / raw)
To: Julian Anastasov; +Cc: Greg Kroah-Hartman, linux-kernel, Alan Cox, Jiri Slaby
Ccing Alan.
On 03/23/2011 01:45 AM, Julian Anastasov wrote:
> When a USB serial device is not attached I see
> open() to leak 8-byte structures:
>
> modprobe usbserial
> [ -e /dev/ttyUSB0 ] && cat /dev/ttyUSB0
> cat: /dev/ttyUSB0: No such device
>
> Note that error must be ENODEV with usbserial
> loaded and when name exists, not ENXIO.
>
> kmemleak shows such output:
>
> unreferenced object 0xc0e19860 (size 8):
> comm cat, pid 1226, jiffies 4294919464 (age 287.476s)
> hex dump (first 8 bytes):
> 44 de 2d c1 01 00 00 00 D.-.....
> backtrace:
> [<c1065a74>] create_object+0x109/0x1ad
> [<c1063d2b>] kmem_cache_alloc+0x60/0x68
> [<c113505c>] tty_ldisc_get+0x54/0x76
> [<c11358c9>] tty_ldisc_init+0xa/0x20
> [<c1130ab4>] initialize_tty_struct+0x2d/0x1ac
> [<c1130c8c>] tty_init_dev+0x59/0x10d
> [<c113136d>] tty_open+0x24a/0x3a2
> [<c1069516>] chrdev_open+0xd1/0xef
> [<c106d2d5>] nameidata_drop_rcu_last+0x3b/0x49
> [<c1069445>] chrdev_open+0x0/0xef
> [<c1065d42>] __dentry_open.clone.15+0xec/0x1c3
> [<c10669a5>] nameidata_to_filp+0x2a/0x33
> [<c106f7c7>] finish_open+0x6e/0xfc
> [<c106fbda>] do_filp_open+0x144/0x4af
> [<c1076f55>] alloc_fd+0x41/0xa5
> [<c10669ef>] do_sys_open+0x41/0xc3
>
> Looking at tty_init_dev() it seems initialize_tty_struct()
> attaches tty->ldisc via tty_ldisc_init() but on
> tty_driver_install_tty() failure (-ENODEV) we call free_tty_struct()
> which does nothing with tty->ldisc.
>
> The appended patch fixes the leak but I'm not
> sure what tty->ldisc value we can see in release_one_tty(),
> I assume if ldisc is freed tty->ldisc should be NULL, so
> free_tty_struct() will not try to double-free the ldisc.
>
> Signed-off-by: Julian Anastasov <ja@ssi.bg>
> ---
> drivers/tty/tty_io.c | 1 +
> 1 file changed, 1 insertion(+)
>
> --- linux-2.6.38/drivers/tty/tty_io.c
> +++ linux-2.6.38/drivers/tty/tty_io.c
> @@ -188,6 +188,7 @@ void free_tty_struct(struct tty_struct *
> put_device(tty->dev);
> kfree(tty->write_buf);
> tty_buffer_free_all(tty);
> + kfree(tty->ldisc);
We should not mess up with ldisc here. We should call something like
tty_ldisc_deinit from tty_init_dev on fail path. The deinit should drop
the reference (call put_ldisc). Or maybe even deinitialize_tty_struct ->
tty_ldisc_deinit -> put_ldisc).
Note that your way you do not drop the module refcount of ld ops.
> kfree(tty);
> }
regards,
--
js
suse labs
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] tty: fix tty->ldisc leak on ENODEV from driver install
2011-03-23 8:12 ` Jiri Slaby
@ 2011-03-23 8:49 ` Julian Anastasov
0 siblings, 0 replies; 3+ messages in thread
From: Julian Anastasov @ 2011-03-23 8:49 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Greg Kroah-Hartman, linux-kernel, Alan Cox, Jiri Slaby
Hello,
On Wed, 23 Mar 2011, Jiri Slaby wrote:
>> --- linux-2.6.38/drivers/tty/tty_io.c
>> +++ linux-2.6.38/drivers/tty/tty_io.c
>> @@ -188,6 +188,7 @@ void free_tty_struct(struct tty_struct *
>> put_device(tty->dev);
>> kfree(tty->write_buf);
>> tty_buffer_free_all(tty);
>> + kfree(tty->ldisc);
>
> We should not mess up with ldisc here. We should call something like
> tty_ldisc_deinit from tty_init_dev on fail path. The deinit should drop
> the reference (call put_ldisc). Or maybe even deinitialize_tty_struct ->
> tty_ldisc_deinit -> put_ldisc).
>
> Note that your way you do not drop the module refcount of ld ops.
Right, that is what I found before going to sleep.
>From the visible functions I see that currently only
tty_ldisc_deref does exactly what we need. But I suspect
it should be used for other purposes. I assume the authors
have a better idea what to do here. Let me know if I need
to test a better patch.
>> kfree(tty);
>> }
>
> regards,
> --
> js
> suse labs
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-03-23 8:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-23 0:45 [PATCH] tty: fix tty->ldisc leak on ENODEV from driver install Julian Anastasov
2011-03-23 8:12 ` Jiri Slaby
2011-03-23 8:49 ` Julian Anastasov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox