USB-serial console and lockdep

USB-serial console and lockdep

[Johan Hovold]

Hi Peter,

I get this missing-lockdep-annotation warning which I haven't seen
before when booting with a usb-serial console on 3.18-rc5. It's been a
while since I last tested this, though, and the tty_ldisc_ref wasn't
introduced until 833efc0ed19c ("USB: serial: invoke dcd_change ldisc's
handler.").

[   10.575225] usbserial: USB Serial support registered for pl2303
[   10.575561] pl2303 1-2.1:1.0: pl2303 converter detected
[   10.627563] usb 1-2.1: pl2303 converter now attached to ttyUSB0
[   10.650939] INFO: trying to register non-static key.
[   10.651000] the code is fine but needs lockdep annotation.
[   10.651000] turning off the locking correctness validator.
[   10.651031] CPU: 0 PID: 68 Comm: udevd Tainted: G        W      3.18.0-rc5 #10
[   10.651092] [<c0016f04>] (unwind_backtrace) from [<c0013978>] (show_stack+0x20/0x24)
[   10.651123] [<c0013978>] (show_stack) from [<c0449794>] (dump_stack+0x24/0x28)
[   10.651184] [<c0449794>] (dump_stack) from [<c006f730>] (__lock_acquire+0x1e50/0x2004)
[   10.651214] [<c006f730>] (__lock_acquire) from [<c0070128>] (lock_acquire+0xe4/0x18c)
[   10.651245] [<c0070128>] (lock_acquire) from [<c027c6f8>] (ldsem_down_read_trylock+0x78/0x90)
[   10.651275] [<c027c6f8>] (ldsem_down_read_trylock) from [<c027a1cc>] (tty_ldisc_ref+0x24/0x58)
[   10.651306] [<c027a1cc>] (tty_ldisc_ref) from [<c0340760>] (usb_serial_handle_dcd_change+0x48/0xe8)
[   10.651367] [<c0340760>] (usb_serial_handle_dcd_change) from [<bf000484>] (pl2303_read_int_callback+0x210/0x220 [pl2303])
[   10.651428] [<bf000484>] (pl2303_read_int_callback [pl2303]) from [<c031624c>] (__usb_hcd_giveback_urb+0x80/0x140)
[   10.651458] [<c031624c>] (__usb_hcd_giveback_urb) from [<c0316fc0>] (usb_giveback_urb_bh+0x98/0xd4)
[   10.651489] [<c0316fc0>] (usb_giveback_urb_bh) from [<c0042e44>] (tasklet_hi_action+0x9c/0x108)
[   10.651519] [<c0042e44>] (tasklet_hi_action) from [<c0042380>] (__do_softirq+0x148/0x42c)
[   10.651550] [<c0042380>] (__do_softirq) from [<c00429cc>] (irq_exit+0xd8/0x114)
[   10.651580] [<c00429cc>] (irq_exit) from [<c007ae58>] (__handle_domain_irq+0x84/0xdc)
[   10.651611] [<c007ae58>] (__handle_domain_irq) from [<c000879c>] (omap_intc_handle_irq+0xd8/0xe0)
[   10.651641] [<c000879c>] (omap_intc_handle_irq) from [<c0014544>] (__irq_svc+0x44/0x7c)
[   10.651641] Exception stack(0xdf4e7f08 to 0xdf4e7f50)
[   10.651672] 7f00:                   debc0b80 df4e7f5c 00000000 00000000 debc0b80 be8da96c
[   10.651702] 7f20: 00000000 00000128 c000fc84 df4e6000 00000000 df4e7f94 00000004 df4e7f50
[   10.651702] 7f40: c038ebc0 c038d74c 600f0013 ffffffff
[   10.651733] [<c0014544>] (__irq_svc) from [<c038d74c>] (___sys_sendmsg.part.29+0x0/0x2e0)
[   10.651763] [<c038d74c>] (___sys_sendmsg.part.29) from [<c038ec08>] (SyS_sendmsg+0x18/0x1c)
[   10.651794] [<c038ec08>] (SyS_sendmsg) from [<c000fa00>] (ret_fast_syscall+0x0/0x48)
[   10.692871] console [ttyUSB0] enabled

Johan

Re: USB-serial console and lockdep

[Peter Hurley]

Hi Johan,

On 11/18/2014 11:18 AM, Johan Hovold wrote:
> I get this missing-lockdep-annotation warning which I haven't seen
> before when booting with a usb-serial console on 3.18-rc5. It's been a
> while since I last tested this, though, and the tty_ldisc_ref wasn't
> introduced until 833efc0ed19c ("USB: serial: invoke dcd_change ldisc's
> handler.").

Sorry it took me so long to finally look at this -- at least I'm looking
at it in the same year ;)  (in my tzone anyway)

Is this easily reproducible?

Because for lockdep to be trying to register the ldsem lock class
from the tty_ldisc_ref() means that no tty has yet been opened [see 1].
So how did the call to tty_port_tty_get() in pl2303_update_line_status()
return a tty?

If you can easily reproduce this, I can supply you with a debug patch
to find out what's going on.

Regards,
Peter Hurley 

> [   10.575225] usbserial: USB Serial support registered for pl2303
> [   10.575561] pl2303 1-2.1:1.0: pl2303 converter detected
> [   10.627563] usb 1-2.1: pl2303 converter now attached to ttyUSB0
> [   10.650939] INFO: trying to register non-static key.
> [   10.651000] the code is fine but needs lockdep annotation.
> [   10.651000] turning off the locking correctness validator.
> [   10.651031] CPU: 0 PID: 68 Comm: udevd Tainted: G        W      3.18.0-rc5 #10
> [   10.651092] [<c0016f04>] (unwind_backtrace) from [<c0013978>] (show_stack+0x20/0x24)
> [   10.651123] [<c0013978>] (show_stack) from [<c0449794>] (dump_stack+0x24/0x28)
> [   10.651184] [<c0449794>] (dump_stack) from [<c006f730>] (__lock_acquire+0x1e50/0x2004)
> [   10.651214] [<c006f730>] (__lock_acquire) from [<c0070128>] (lock_acquire+0xe4/0x18c)
> [   10.651245] [<c0070128>] (lock_acquire) from [<c027c6f8>] (ldsem_down_read_trylock+0x78/0x90)
> [   10.651275] [<c027c6f8>] (ldsem_down_read_trylock) from [<c027a1cc>] (tty_ldisc_ref+0x24/0x58)
> [   10.651306] [<c027a1cc>] (tty_ldisc_ref) from [<c0340760>] (usb_serial_handle_dcd_change+0x48/0xe8)
> [   10.651367] [<c0340760>] (usb_serial_handle_dcd_change) from [<bf000484>] (pl2303_read_int_callback+0x210/0x220 [pl2303])
> [   10.651428] [<bf000484>] (pl2303_read_int_callback [pl2303]) from [<c031624c>] (__usb_hcd_giveback_urb+0x80/0x140)
> [   10.651458] [<c031624c>] (__usb_hcd_giveback_urb) from [<c0316fc0>] (usb_giveback_urb_bh+0x98/0xd4)
> [   10.651489] [<c0316fc0>] (usb_giveback_urb_bh) from [<c0042e44>] (tasklet_hi_action+0x9c/0x108)
> [   10.651519] [<c0042e44>] (tasklet_hi_action) from [<c0042380>] (__do_softirq+0x148/0x42c)
> [   10.651550] [<c0042380>] (__do_softirq) from [<c00429cc>] (irq_exit+0xd8/0x114)
> [   10.651580] [<c00429cc>] (irq_exit) from [<c007ae58>] (__handle_domain_irq+0x84/0xdc)
> [   10.651611] [<c007ae58>] (__handle_domain_irq) from [<c000879c>] (omap_intc_handle_irq+0xd8/0xe0)
> [   10.651641] [<c000879c>] (omap_intc_handle_irq) from [<c0014544>] (__irq_svc+0x44/0x7c)
> [   10.651641] Exception stack(0xdf4e7f08 to 0xdf4e7f50)
> [   10.651672] 7f00:                   debc0b80 df4e7f5c 00000000 00000000 debc0b80 be8da96c
> [   10.651702] 7f20: 00000000 00000128 c000fc84 df4e6000 00000000 df4e7f94 00000004 df4e7f50
> [   10.651702] 7f40: c038ebc0 c038d74c 600f0013 ffffffff
> [   10.651733] [<c0014544>] (__irq_svc) from [<c038d74c>] (___sys_sendmsg.part.29+0x0/0x2e0)
> [   10.651763] [<c038d74c>] (___sys_sendmsg.part.29) from [<c038ec08>] (SyS_sendmsg+0x18/0x1c)
> [   10.651794] [<c038ec08>] (SyS_sendmsg) from [<c000fa00>] (ret_fast_syscall+0x0/0x48)
> [   10.692871] console [ttyUSB0] enabled


[1] Call tree for registering the ldsem lock class

tty_open
  tty_init_dev
    alloc_tty_struct
      init_ldsem (macro in include/linux/tty_ldisc.h)
        * the static lock_class_key is expanded here *
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB-serial console and lockdep

[Johan Hovold]

On Wed, Dec 31, 2014 at 09:07:59PM -0500, Peter Hurley wrote:
> Hi Johan,
> 
> On 11/18/2014 11:18 AM, Johan Hovold wrote:
> > I get this missing-lockdep-annotation warning which I haven't seen
> > before when booting with a usb-serial console on 3.18-rc5. It's been a
> > while since I last tested this, though, and the tty_ldisc_ref wasn't
> > introduced until 833efc0ed19c ("USB: serial: invoke dcd_change ldisc's
> > handler.").
> 
> Sorry it took me so long to finally look at this -- at least I'm looking
> at it in the same year ;)  (in my tzone anyway)

No worries. Wasn't a top prio of mine either. :)

Thanks for taking a look.

> Is this easily reproducible?

Yes, happens on every boot with the pl2303 driver.

> Because for lockdep to be trying to register the ldsem lock class
> from the tty_ldisc_ref() means that no tty has yet been opened [see 1].
> So how did the call to tty_port_tty_get() in pl2303_update_line_status()
> return a tty?

Because the USB console driver is using a only partially initialised,
"fake" tty struct to pass terminal settings to the underlying driver.
So no wonder things can blow up.

This particular issue can be fixed by making sure to initialise the
ldisc semaphore, but there are likely more potential problems here,
including use-after-free as the fake tty wasn't released using the
kref. I'll post two fixes as a follow up.

A more long term solution might be to rewrite all usb-serial drivers to
handle a NULL termios and pass a ktermios to set_termios similar to how
serial-core does this.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] USB: console: fix uninitialised ldisc semaphore

[Johan Hovold]

The USB console currently allocates a temporary fake tty which is used
to pass terminal settings to the underlying serial driver.

The tty struct is not fully initialised, something which can lead to a
lockdep warning (or worse) if a serial driver tries to acquire a
line-discipline reference:

	usbserial: USB Serial support registered for pl2303
	pl2303 1-2.1:1.0: pl2303 converter detected
	usb 1-2.1: pl2303 converter now attached to ttyUSB0
	INFO: trying to register non-static key.
	the code is fine but needs lockdep annotation.
	turning off the locking correctness validator.
	CPU: 0 PID: 68 Comm: udevd Tainted: G        W      3.18.0-rc5 #10
	[<c0016f04>] (unwind_backtrace) from [<c0013978>] (show_stack+0x20/0x24)
	[<c0013978>] (show_stack) from [<c0449794>] (dump_stack+0x24/0x28)
	[<c0449794>] (dump_stack) from [<c006f730>] (__lock_acquire+0x1e50/0x2004)
	[<c006f730>] (__lock_acquire) from [<c0070128>] (lock_acquire+0xe4/0x18c)
	[<c0070128>] (lock_acquire) from [<c027c6f8>] (ldsem_down_read_trylock+0x78/0x90)
	[<c027c6f8>] (ldsem_down_read_trylock) from [<c027a1cc>] (tty_ldisc_ref+0x24/0x58)
	[<c027a1cc>] (tty_ldisc_ref) from [<c0340760>] (usb_serial_handle_dcd_change+0x48/0xe8)
	[<c0340760>] (usb_serial_handle_dcd_change) from [<bf000484>] (pl2303_read_int_callback+0x210/0x220 [pl2303])
	[<bf000484>] (pl2303_read_int_callback [pl2303]) from [<c031624c>] (__usb_hcd_giveback_urb+0x80/0x140)
	[<c031624c>] (__usb_hcd_giveback_urb) from [<c0316fc0>] (usb_giveback_urb_bh+0x98/0xd4)
	[<c0316fc0>] (usb_giveback_urb_bh) from [<c0042e44>] (tasklet_hi_action+0x9c/0x108)
	[<c0042e44>] (tasklet_hi_action) from [<c0042380>] (__do_softirq+0x148/0x42c)
	[<c0042380>] (__do_softirq) from [<c00429cc>] (irq_exit+0xd8/0x114)
	[<c00429cc>] (irq_exit) from [<c007ae58>] (__handle_domain_irq+0x84/0xdc)
	[<c007ae58>] (__handle_domain_irq) from [<c000879c>] (omap_intc_handle_irq+0xd8/0xe0)
	[<c000879c>] (omap_intc_handle_irq) from [<c0014544>] (__irq_svc+0x44/0x7c)
	Exception stack(0xdf4e7f08 to 0xdf4e7f50)
	7f00:                   debc0b80 df4e7f5c 00000000 00000000 debc0b80 be8da96c
	7f20: 00000000 00000128 c000fc84 df4e6000 00000000 df4e7f94 00000004 df4e7f50
	7f40: c038ebc0 c038d74c 600f0013 ffffffff
	[<c0014544>] (__irq_svc) from [<c038d74c>] (___sys_sendmsg.part.29+0x0/0x2e0)
	[<c038d74c>] (___sys_sendmsg.part.29) from [<c038ec08>] (SyS_sendmsg+0x18/0x1c)
	[<c038ec08>] (SyS_sendmsg) from [<c000fa00>] (ret_fast_syscall+0x0/0x48)
	console [ttyUSB0] enabled

Fixes: 36697529b5bb ("tty: Replace ldisc locking with ldisc_sem")
Cc: stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Signed-off-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/usb/serial/console.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c
index 8d7fc48b1f30..e56f394b58d8 100644
--- a/drivers/usb/serial/console.c
+++ b/drivers/usb/serial/console.c
@@ -140,6 +140,7 @@ static int usb_console_setup(struct console *co, char *options)
 			tty_port_tty_set(&port->port, tty);
 			tty->driver = usb_serial_tty_driver;
 			tty->index = co->index;
+			init_ldsem(&tty->ldisc_sem);
 			if (tty_init_termios(tty)) {
 				retval = -ENOMEM;
 				goto free_tty;
-- 
2.0.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] USB: console: fix potential use after free

[Johan Hovold]

Use tty kref to release the fake tty in usb_console_setup to avoid use
after free if the underlying serial driver has acquired a reference.

Fixes: 4a90f09b20f4 ("tty: usb-serial krefs")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/console.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c
index e56f394b58d8..4fbf7fe64860 100644
--- a/drivers/usb/serial/console.c
+++ b/drivers/usb/serial/console.c
@@ -47,6 +47,13 @@ static struct console usbcons;
  */
 
 
+static void usb_console_release_fake_tty(struct kref *kref)
+{
+	struct tty_struct *tty = container_of(kref, struct tty_struct, kref);
+
+	kfree(tty);
+}
+
 /*
  * The parsing of the command line works exactly like the
  * serial.c code, except that the specifier is "ttyUSB" instead
@@ -137,7 +144,6 @@ static int usb_console_setup(struct console *co, char *options)
 				goto reset_open_count;
 			}
 			kref_init(&tty->kref);
-			tty_port_tty_set(&port->port, tty);
 			tty->driver = usb_serial_tty_driver;
 			tty->index = co->index;
 			init_ldsem(&tty->ldisc_sem);
@@ -145,6 +151,7 @@ static int usb_console_setup(struct console *co, char *options)
 				retval = -ENOMEM;
 				goto free_tty;
 			}
+			tty_port_tty_set(&port->port, tty);
 		}
 
 		/* only call the device specific open if this
@@ -162,7 +169,7 @@ static int usb_console_setup(struct console *co, char *options)
 			serial->type->set_termios(tty, port, &dummy);
 
 			tty_port_tty_set(&port->port, NULL);
-			kfree(tty);
+			kref_put(&tty->kref, usb_console_release_fake_tty);
 		}
 		set_bit(ASYNCB_INITIALIZED, &port->port.flags);
 	}
@@ -179,7 +186,7 @@ static int usb_console_setup(struct console *co, char *options)
  fail:
 	tty_port_tty_set(&port->port, NULL);
  free_tty:
-	kfree(tty);
+	kref_put(&tty->kref, usb_console_release_fake_tty);
  reset_open_count:
 	port->port.count = 0;
 	usb_autopm_put_interface(serial->interface);
-- 
2.0.5

[PATCH v2 1/2] USB: console: fix uninitialised ldisc semaphore

[Johan Hovold]

The USB console currently allocates a temporary fake tty which is used
to pass terminal settings to the underlying serial driver.

The tty struct is not fully initialised, something which can lead to a
lockdep warning (or worse) if a serial driver tries to acquire a
line-discipline reference:

	usbserial: USB Serial support registered for pl2303
	pl2303 1-2.1:1.0: pl2303 converter detected
	usb 1-2.1: pl2303 converter now attached to ttyUSB0
	INFO: trying to register non-static key.
	the code is fine but needs lockdep annotation.
	turning off the locking correctness validator.
	CPU: 0 PID: 68 Comm: udevd Tainted: G        W      3.18.0-rc5 #10
	[<c0016f04>] (unwind_backtrace) from [<c0013978>] (show_stack+0x20/0x24)
	[<c0013978>] (show_stack) from [<c0449794>] (dump_stack+0x24/0x28)
	[<c0449794>] (dump_stack) from [<c006f730>] (__lock_acquire+0x1e50/0x2004)
	[<c006f730>] (__lock_acquire) from [<c0070128>] (lock_acquire+0xe4/0x18c)
	[<c0070128>] (lock_acquire) from [<c027c6f8>] (ldsem_down_read_trylock+0x78/0x90)
	[<c027c6f8>] (ldsem_down_read_trylock) from [<c027a1cc>] (tty_ldisc_ref+0x24/0x58)
	[<c027a1cc>] (tty_ldisc_ref) from [<c0340760>] (usb_serial_handle_dcd_change+0x48/0xe8)
	[<c0340760>] (usb_serial_handle_dcd_change) from [<bf000484>] (pl2303_read_int_callback+0x210/0x220 [pl2303])
	[<bf000484>] (pl2303_read_int_callback [pl2303]) from [<c031624c>] (__usb_hcd_giveback_urb+0x80/0x140)
	[<c031624c>] (__usb_hcd_giveback_urb) from [<c0316fc0>] (usb_giveback_urb_bh+0x98/0xd4)
	[<c0316fc0>] (usb_giveback_urb_bh) from [<c0042e44>] (tasklet_hi_action+0x9c/0x108)
	[<c0042e44>] (tasklet_hi_action) from [<c0042380>] (__do_softirq+0x148/0x42c)
	[<c0042380>] (__do_softirq) from [<c00429cc>] (irq_exit+0xd8/0x114)
	[<c00429cc>] (irq_exit) from [<c007ae58>] (__handle_domain_irq+0x84/0xdc)
	[<c007ae58>] (__handle_domain_irq) from [<c000879c>] (omap_intc_handle_irq+0xd8/0xe0)
	[<c000879c>] (omap_intc_handle_irq) from [<c0014544>] (__irq_svc+0x44/0x7c)
	Exception stack(0xdf4e7f08 to 0xdf4e7f50)
	7f00:                   debc0b80 df4e7f5c 00000000 00000000 debc0b80 be8da96c
	7f20: 00000000 00000128 c000fc84 df4e6000 00000000 df4e7f94 00000004 df4e7f50
	7f40: c038ebc0 c038d74c 600f0013 ffffffff
	[<c0014544>] (__irq_svc) from [<c038d74c>] (___sys_sendmsg.part.29+0x0/0x2e0)
	[<c038d74c>] (___sys_sendmsg.part.29) from [<c038ec08>] (SyS_sendmsg+0x18/0x1c)
	[<c038ec08>] (SyS_sendmsg) from [<c000fa00>] (ret_fast_syscall+0x0/0x48)
	console [ttyUSB0] enabled

Fixes: 36697529b5bb ("tty: Replace ldisc locking with ldisc_sem")
Cc: stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Signed-off-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/usb/serial/console.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c
index 8d7fc48b1f30..e56f394b58d8 100644
--- a/drivers/usb/serial/console.c
+++ b/drivers/usb/serial/console.c
@@ -140,6 +140,7 @@ static int usb_console_setup(struct console *co, char *options)
 			tty_port_tty_set(&port->port, tty);
 			tty->driver = usb_serial_tty_driver;
 			tty->index = co->index;
+			init_ldsem(&tty->ldisc_sem);
 			if (tty_init_termios(tty)) {
 				retval = -ENOMEM;
 				goto free_tty;
-- 
2.0.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: console: fix potential use after free

[Johan Hovold]

Use tty kref to release the fake tty in usb_console_setup to avoid use
after free if the underlying serial driver has acquired a reference.

Note that using the tty destructor release_one_tty requires some more
state to be initialised.

Fixes: 4a90f09b20f4 ("tty: usb-serial krefs")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---

Changes since v1:

 - Use tty_kref_put to release tty also in setup()
 - Some more state must be initialised to avoid NULL derefs when the tty
   destructor is run after the underlying driver puts its reference.


 drivers/usb/serial/console.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c
index e56f394b58d8..29fa1c3d0089 100644
--- a/drivers/usb/serial/console.c
+++ b/drivers/usb/serial/console.c
@@ -46,6 +46,8 @@ static struct console usbcons;
  * ------------------------------------------------------------
  */
 
+static const struct tty_operations usb_console_fake_tty_ops = {
+};
 
 /*
  * The parsing of the command line works exactly like the
@@ -137,14 +139,17 @@ static int usb_console_setup(struct console *co, char *options)
 				goto reset_open_count;
 			}
 			kref_init(&tty->kref);
-			tty_port_tty_set(&port->port, tty);
 			tty->driver = usb_serial_tty_driver;
 			tty->index = co->index;
 			init_ldsem(&tty->ldisc_sem);
+			INIT_LIST_HEAD(&tty->tty_files);
+			kref_get(&tty->driver->kref);
+			tty->ops = &usb_console_fake_tty_ops;
 			if (tty_init_termios(tty)) {
 				retval = -ENOMEM;
-				goto free_tty;
+				goto put_tty;
 			}
+			tty_port_tty_set(&port->port, tty);
 		}
 
 		/* only call the device specific open if this
@@ -162,7 +167,7 @@ static int usb_console_setup(struct console *co, char *options)
 			serial->type->set_termios(tty, port, &dummy);
 
 			tty_port_tty_set(&port->port, NULL);
-			kfree(tty);
+			tty_kref_put(tty);
 		}
 		set_bit(ASYNCB_INITIALIZED, &port->port.flags);
 	}
@@ -178,8 +183,8 @@ static int usb_console_setup(struct console *co, char *options)
 
  fail:
 	tty_port_tty_set(&port->port, NULL);
- free_tty:
-	kfree(tty);
+ put_tty:
+	tty_kref_put(tty);
  reset_open_count:
 	port->port.count = 0;
 	usb_autopm_put_interface(serial->interface);
-- 
2.0.5

Re: USB-serial console and lockdep

[Peter Hurley]

On 01/03/2015 11:26 AM, Johan Hovold wrote:
> On Wed, Dec 31, 2014 at 09:07:59PM -0500, Peter Hurley wrote:
>> Hi Johan,
>>
>> On 11/18/2014 11:18 AM, Johan Hovold wrote:
>>> I get this missing-lockdep-annotation warning which I haven't seen
>>> before when booting with a usb-serial console on 3.18-rc5. It's been a
>>> while since I last tested this, though, and the tty_ldisc_ref wasn't
>>> introduced until 833efc0ed19c ("USB: serial: invoke dcd_change ldisc's
>>> handler.").
>>
>> Sorry it took me so long to finally look at this -- at least I'm looking
>> at it in the same year ;)  (in my tzone anyway)
> 
> No worries. Wasn't a top prio of mine either. :)
> 
> Thanks for taking a look.
> 
>> Is this easily reproducible?
> 
> Yes, happens on every boot with the pl2303 driver.
> 
>> Because for lockdep to be trying to register the ldsem lock class
>> from the tty_ldisc_ref() means that no tty has yet been opened [see 1].
>> So how did the call to tty_port_tty_get() in pl2303_update_line_status()
>> return a tty?
> 
> Because the USB console driver is using a only partially initialised,
> "fake" tty struct to pass terminal settings to the underlying driver.
> So no wonder things can blow up.

Ahh, I did not know that.

> This particular issue can be fixed by making sure to initialise the
> ldisc semaphore, but there are likely more potential problems here,
> including use-after-free as the fake tty wasn't released using the
> kref. I'll post two fixes as a follow up.
> 
> A more long term solution might be to rewrite all usb-serial drivers to
> handle a NULL termios and pass a ktermios to set_termios similar to how
> serial-core does this.

I agree that this definitely needs a more robust solution.
FWIW, I don't think serial-core is a particularly good model.

Regards,
Peter Hurley



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html