USB: serial: suppress driver bind attributes

PATCH In review Johan Hovold 2 participants 5 messages Jan 16, 2020 – Jan 17, 2020

Subsystems (3)

Stable branch USB USB serial

lore

Share & tools

lore

Johan Hovold

author maintainer

2020-01-16 16:07 UTC #0 lore raw

PATCH 1/1 USB: serial: suppress driver bind attributes

USB-serial drivers must not be unbound from their ports before the corresponding USB driver is unbound from the parent interface so suppress the bind and unbind attributes.

Unbinding a serial driver while it's port is open is a sure way to trigger a crash as any driver state is released on unbind while port hangup is handled on the parent USB interface level. Drivers for multiport devices where ports share a resource such as an interrupt endpoint also generally cannot handle individual ports going away.

Cc stable <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

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

drivers/usb/serial/usb-serial.c +3 -0

index 8f066bb55d7d..dc7a65b9ec98 100644
@@ -1317,6 +1317,9 @@ static int usb_serial_register(struct usb_serial_driver *driver)
 		return -EINVAL;
 	}
 
+	/* Prevent individual ports from being unbound. */
+	driver->driver.suppress_bind_attrs = true;
+
 	usb_serial_operations_init(driver);
 
 	/* Add this device to our list of devices */

Signature · 1 line

2.24.1

Greg KH

2020-01-16 16:18 UTC #1 lore raw

Quoted 21 lines · Johan Hovold

USB-serial drivers must not be unbound from their ports before the corresponding USB driver is unbound from the parent interface so suppress the bind and unbind attributes.
Unbinding a serial driver while it's port is open is a sure way to trigger a crash as any driver state is released on unbind while port hangup is handled on the parent USB interface level. Drivers for multiport devices where ports share a resource such as an interrupt endpoint also generally cannot handle individual ports going away.
Cc stable <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/usb-serial.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/usb/serial/usb-serial.c +0 -0
index 8f066bb55d7d..dc7a65b9ec98 100644

drivers/usb/serial/usb-serial.c static int usb_serial_register(struct usb_serial_driver *driver) · line 1321

Quoted 5 lines

drivers/usb/serial/usb-serial.c +2 -0

@@ -1317,6 +1317,9 @@ static int usb_serial_register(struct usb_serial_driver *driver)
 		return -EINVAL;
 	}
 
+	/* Prevent individual ports from being unbound. */
+	driver->driver.suppress_bind_attrs = true;

We can still unbind the usb driver though, right? If so, this is fine with me.

thanks,

greg k-h

Johan Hovold

author maintainer

2020-01-16 16:24 UTC #2 lore raw

Quoted 25 lines · Greg Kroah-Hartman

Quoted 24 lines · Johan Hovold
USB-serial drivers must not be unbound from their ports before the corresponding USB driver is unbound from the parent interface so suppress the bind and unbind attributes.
Unbinding a serial driver while it's port is open is a sure way to trigger a crash as any driver state is released on unbind while port hangup is handled on the parent USB interface level. Drivers for multiport devices where ports share a resource such as an interrupt endpoint also generally cannot handle individual ports going away.
Cc stable <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/usb-serial.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/usb/serial/usb-serial.c +0 -0
index 8f066bb55d7d..dc7a65b9ec98 100644
@@ -1317,6 +1317,9 @@ static int usb_serial_register(struct usb_serial_driver *driver)
 		return -EINVAL;
 	}
 

Quoted 5 lines

drivers/usb/serial/usb-serial.c static int usb_serial_register(struct usb_serial_driver *driver) · line 1321
Quoted 2 lines
drivers/usb/serial/usb-serial.c +2 -0
+	/* Prevent individual ports from being unbound. */
+	driver->driver.suppress_bind_attrs = true;
We can still unbind the usb driver though, right? If so, this is fine with me.

Right, this is only about disabling individual ports, something which essentially no subdriver can handle while a port is open (e.g. port-data set to NULL while port is still open... boom).

Johan

Greg KH

Reviewed-by Greg Kroah-Hartman

2020-01-16 19:06 UTC #3 lore raw

Quoted 32 lines · Johan Hovold

Quoted 25 lines · Greg Kroah-Hartman
Quoted 24 lines · Johan Hovold
USB-serial drivers must not be unbound from their ports before the corresponding USB driver is unbound from the parent interface so suppress the bind and unbind attributes.
Unbinding a serial driver while it's port is open is a sure way to trigger a crash as any driver state is released on unbind while port hangup is handled on the parent USB interface level. Drivers for multiport devices where ports share a resource such as an interrupt endpoint also generally cannot handle individual ports going away.
Cc stable <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/usb-serial.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/usb/serial/usb-serial.c +0 -0
index 8f066bb55d7d..dc7a65b9ec98 100644
@@ -1317,6 +1317,9 @@ static int usb_serial_register(struct usb_serial_driver *driver)
 		return -EINVAL;
 	}
 
Quoted 5 lines
drivers/usb/serial/usb-serial.c static int usb_serial_register(struct usb_serial_driver *driver) · line 1321
Quoted 2 lines
drivers/usb/serial/usb-serial.c +2 -0
+	/* Prevent individual ports from being unbound. */
+	driver->driver.suppress_bind_attrs = true;
We can still unbind the usb driver though, right? If so, this is fine with me.

Quoted 3 lines

Right, this is only about disabling individual ports, something which essentially no subdriver can handle while a port is open (e.g. port-data set to NULL while port is still open... boom).

Ok, thanks for verifying, and sorry for writing that bug in the first place :)

Reviewed-by Greg Kroah-Hartman <[email protected]>

Johan Hovold

author maintainer

2020-01-17 10:13 UTC #4 lore raw

Quoted 33 lines · Greg Kroah-Hartman

Quoted 32 lines · Johan Hovold
Quoted 30 lines · Greg Kroah-Hartman
Quoted 21 lines · Johan Hovold
USB-serial drivers must not be unbound from their ports before the corresponding USB driver is unbound from the parent interface so suppress the bind and unbind attributes.
Unbinding a serial driver while it's port is open is a sure way to trigger a crash as any driver state is released on unbind while port hangup is handled on the parent USB interface level. Drivers for multiport devices where ports share a resource such as an interrupt endpoint also generally cannot handle individual ports going away.
Cc stable <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/usb-serial.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/usb/serial/usb-serial.c +0 -0
index 8f066bb55d7d..dc7a65b9ec98 100644
drivers/usb/serial/usb-serial.c static int usb_serial_register(struct usb_serial_driver *driver) · line 1321
Quoted 5 lines
drivers/usb/serial/usb-serial.c +2 -0
@@ -1317,6 +1317,9 @@ static int usb_serial_register(struct usb_serial_driver *driver)
 		return -EINVAL;
 	}
 
+	/* Prevent individual ports from being unbound. */
+	driver->driver.suppress_bind_attrs = true;
We can still unbind the usb driver though, right? If so, this is fine with me.

Quoted 8 lines

Quoted 3 lines
Right, this is only about disabling individual ports, something which essentially no subdriver can handle while a port is open (e.g. port-data set to NULL while port is still open... boom).
Ok, thanks for verifying, and sorry for writing that bug in the first place :)
Reviewed-by Greg Kroah-Hartman <[email protected]>

Heh, thanks for reviewing.

Johan