USB-serial console and lockdep

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

Hi Johan,

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

[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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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

Thanks for taking a look.

Yes, happens on every boot with the pl2303 driver.

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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

---
 drivers/usb/serial/console.c | 1 +
 1 file changed, 1 insertion(+)

index 8d7fc48b1f30..e56f394b58d8 100644
@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

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

index e56f394b58d8..4fbf7fe64860 100644
@@ -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

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

---
 drivers/usb/serial/console.c | 1 +
 1 file changed, 1 insertion(+)

index 8d7fc48b1f30..e56f394b58d8 100644
@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

---

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

index e56f394b58d8..29fa1c3d0089 100644
@@ -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

Ahh, I did not know that.

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html