USB: usblcd: disconnect fix and locking clean ups

PATCH New Johan Hovold 1 participant 11 messages Sep 26, 2019

Subsystems (2)

Stable branch USB

lore

Share & tools

lore

Johan Hovold

author

2019-09-26 09:12 UTC #0 lore raw

cover USB: usblcd: disconnect fix and locking clean ups

This series fixes a failure to stop I/O on disconnect() in the usblcd driver. Turns out there was a lot of legacy cruft in this driver which could simply be removed.

The first patch is marked for stable and could go into v5.4 while the rest is v5.5 material. Posting all at once for completeness.

I was tempted to rip out the custom ioctls() used to retrieve the driver version and bcdDevice (sic!), but decided to leave them in. I doubt anyone would miss them though so perhaps we should give it a go?

Tested using a mockup device.

Johan

Johan Hovold (4):
USB: usblcd: fix I/O after disconnect
USB: usblcd: drop redundant disconnect mutex
USB: usblcd: drop redundant lcd mutex
USB: usblcd: use pr_err()

 drivers/usb/misc/usblcd.c | 60 +++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

Signature · 1 line

2.23.0

Johan Hovold

author

2019-09-26 09:12 UTC #1 lore raw

PATCH 1/4 USB: usblcd: fix I/O after disconnect

Make sure to stop all I/O on disconnect by adding a disconnected flag which is used to prevent new I/O from being started and by stopping all ongoing I/O before returning.

This also fixes a potential use-after-free on driver unbind in case the driver data is freed before the completion handler has run.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")

Cc stable <[email protected]> # 7bbe990c989e

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

---
 drivers/usb/misc/usblcd.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

drivers/usb/misc/usblcd.c +31 -2

index 9ba4a4e68d91..aa982d3ca36b 100644
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/errno.h>
 #include <linux/mutex.h>
+#include <linux/rwsem.h>
 #include <linux/uaccess.h>
 #include <linux/usb.h>

@@ -57,6 +58,8 @@ struct usb_lcd {
 							   using up all RAM */
 	struct usb_anchor	submitted;		/* URBs to wait for
 							   before suspend */
+	struct rw_semaphore	io_rwsem;
+	unsigned long		disconnected:1;
 };
 #define to_lcd_dev(d) container_of(d, struct usb_lcd, kref)

@@ -142,6 +145,13 @@ static ssize_t lcd_read(struct file *file, char __user * buffer,
 
 	dev = file->private_data;
 
+	down_read(&dev->io_rwsem);
+
+	if (dev->disconnected) {
+		retval = -ENODEV;
+		goto out_up_io;
+	}
+
 	/* do a blocking bulk read to get data from the device */
 	retval = usb_bulk_msg(dev->udev,
 			      usb_rcvbulkpipe(dev->udev,

@@ -158,6 +168,9 @@ static ssize_t lcd_read(struct file *file, char __user * buffer,
 			retval = bytes_read;
 	}
 
+out_up_io:
+	up_read(&dev->io_rwsem);
+
 	return retval;
 }

@@ -237,11 +250,18 @@ static ssize_t lcd_write(struct file *file, const char __user * user_buffer,
 	if (r < 0)
 		return -EINTR;
 
+	down_read(&dev->io_rwsem);
+
+	if (dev->disconnected) {
+		retval = -ENODEV;
+		goto err_up_io;
+	}
+
 	/* create a urb, and a buffer for it, and copy the data to the urb */
 	urb = usb_alloc_urb(0, GFP_KERNEL);
 	if (!urb) {
 		retval = -ENOMEM;
-		goto err_no_buf;
+		goto err_up_io;
 	}
 
 	buf = usb_alloc_coherent(dev->udev, count, GFP_KERNEL,

@@ -278,6 +298,7 @@ static ssize_t lcd_write(struct file *file, const char __user * user_buffer,
 	   the USB core will eventually free it entirely */
 	usb_free_urb(urb);
 
+	up_read(&dev->io_rwsem);
 exit:
 	return count;
 error_unanchor:

@@ -285,7 +306,8 @@ static ssize_t lcd_write(struct file *file, const char __user * user_buffer,
 error:
 	usb_free_coherent(dev->udev, count, buf, urb->transfer_dma);
 	usb_free_urb(urb);
-err_no_buf:
+err_up_io:
+	up_read(&dev->io_rwsem);
 	up(&dev->limit_sem);
 	return retval;
 }

@@ -325,6 +347,7 @@ static int lcd_probe(struct usb_interface *interface,
 
 	kref_init(&dev->kref);
 	sema_init(&dev->limit_sem, USB_LCD_CONCURRENT_WRITES);
+	init_rwsem(&dev->io_rwsem);
 	init_usb_anchor(&dev->submitted);
 
 	dev->udev = usb_get_dev(interface_to_usbdev(interface));

@@ -422,6 +445,12 @@ static void lcd_disconnect(struct usb_interface *interface)
 	/* give back our minor */
 	usb_deregister_dev(interface, &lcd_class);
 
+	down_write(&dev->io_rwsem);
+	dev->disconnected = 1;
+	up_write(&dev->io_rwsem);
+
+	usb_kill_anchored_urbs(&dev->submitted);
+
 	/* decrement our usage count */
 	kref_put(&dev->kref, lcd_delete);

Signature · 1 line

2.23.0

Johan Hovold

author

2019-09-26 09:12 UTC #2 lore raw

PATCH 1/4 USB: usblcd: fix I/O after disconnect

In reply to Johan Hovold

Make sure to stop all I/O on disconnect by adding a disconnected flag which is used to prevent new I/O from being started and by stopping all ongoing I/O before returning.

This also fixes a potential use-after-free on driver unbind in case the driver data is freed before the completion handler has run.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")

Cc stable <[email protected]> # 7bbe990c989e

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

---
 drivers/usb/misc/usblcd.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

drivers/usb/misc/usblcd.c +31 -2

index 9ba4a4e68d91..aa982d3ca36b 100644
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/errno.h>
 #include <linux/mutex.h>
+#include <linux/rwsem.h>
 #include <linux/uaccess.h>
 #include <linux/usb.h>

@@ -57,6 +58,8 @@ struct usb_lcd {
 							   using up all RAM */
 	struct usb_anchor	submitted;		/* URBs to wait for
 							   before suspend */
+	struct rw_semaphore	io_rwsem;
+	unsigned long		disconnected:1;
 };
 #define to_lcd_dev(d) container_of(d, struct usb_lcd, kref)

@@ -142,6 +145,13 @@ static ssize_t lcd_read(struct file *file, char __user * buffer,
 
 	dev = file->private_data;
 
+	down_read(&dev->io_rwsem);
+
+	if (dev->disconnected) {
+		retval = -ENODEV;
+		goto out_up_io;
+	}
+
 	/* do a blocking bulk read to get data from the device */
 	retval = usb_bulk_msg(dev->udev,
 			      usb_rcvbulkpipe(dev->udev,

@@ -158,6 +168,9 @@ static ssize_t lcd_read(struct file *file, char __user * buffer,
 			retval = bytes_read;
 	}
 
+out_up_io:
+	up_read(&dev->io_rwsem);
+
 	return retval;
 }

@@ -237,11 +250,18 @@ static ssize_t lcd_write(struct file *file, const char __user * user_buffer,
 	if (r < 0)
 		return -EINTR;
 
+	down_read(&dev->io_rwsem);
+
+	if (dev->disconnected) {
+		retval = -ENODEV;
+		goto err_up_io;
+	}
+
 	/* create a urb, and a buffer for it, and copy the data to the urb */
 	urb = usb_alloc_urb(0, GFP_KERNEL);
 	if (!urb) {
 		retval = -ENOMEM;
-		goto err_no_buf;
+		goto err_up_io;
 	}
 
 	buf = usb_alloc_coherent(dev->udev, count, GFP_KERNEL,

@@ -278,6 +298,7 @@ static ssize_t lcd_write(struct file *file, const char __user * user_buffer,
 	   the USB core will eventually free it entirely */
 	usb_free_urb(urb);
 
+	up_read(&dev->io_rwsem);
 exit:
 	return count;
 error_unanchor:

@@ -285,7 +306,8 @@ static ssize_t lcd_write(struct file *file, const char __user * user_buffer,
 error:
 	usb_free_coherent(dev->udev, count, buf, urb->transfer_dma);
 	usb_free_urb(urb);
-err_no_buf:
+err_up_io:
+	up_read(&dev->io_rwsem);
 	up(&dev->limit_sem);
 	return retval;
 }

@@ -325,6 +347,7 @@ static int lcd_probe(struct usb_interface *interface,
 
 	kref_init(&dev->kref);
 	sema_init(&dev->limit_sem, USB_LCD_CONCURRENT_WRITES);
+	init_rwsem(&dev->io_rwsem);
 	init_usb_anchor(&dev->submitted);
 
 	dev->udev = usb_get_dev(interface_to_usbdev(interface));

@@ -422,6 +445,12 @@ static void lcd_disconnect(struct usb_interface *interface)
 	/* give back our minor */
 	usb_deregister_dev(interface, &lcd_class);
 
+	down_write(&dev->io_rwsem);
+	dev->disconnected = 1;
+	up_write(&dev->io_rwsem);
+
+	usb_kill_anchored_urbs(&dev->submitted);
+
 	/* decrement our usage count */
 	kref_put(&dev->kref, lcd_delete);

Signature · 1 line

2.23.0

Johan Hovold

author

2019-09-26 09:12 UTC #3 lore raw

PATCH 2/4 USB: usblcd: drop redundant disconnect mutex

In reply to Johan Hovold

Drop the redundant disconnect mutex which was introduced after the open-disconnect race had been addressed generally in USB core by commit d4ead16f50f9 ("USB: prevent char device open/deregister race").

Specifically, the rw-semaphore in core guarantees that all calls to open() will have completed and that no new calls to open() will occur after usb_deregister_dev() returns. Hence there is no need use the driver data as an inverted disconnected flag.

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

---
 drivers/usb/misc/usblcd.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

drivers/usb/misc/usblcd.c +1 ~~-17~~

index aa982d3ca36b..b898650a5570 100644
@@ -37,9 +37,6 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
-static DEFINE_MUTEX(open_disc_mutex);
-
-
 struct usb_lcd {
 	struct usb_device	*udev;			/* init: probe_lcd */
 	struct usb_interface	*interface;		/* the interface for

@@ -95,17 +92,10 @@ static int lcd_open(struct inode *inode, struct file *file)
 		return -ENODEV;
 	}
 
-	mutex_lock(&open_disc_mutex);
 	dev = usb_get_intfdata(interface);
-	if (!dev) {
-		mutex_unlock(&open_disc_mutex);
-		mutex_unlock(&lcd_mutex);
-		return -ENODEV;
-	}
 
 	/* increment our usage count for the device */
 	kref_get(&dev->kref);
-	mutex_unlock(&open_disc_mutex);
 
 	/* grab a power reference */
 	r = usb_autopm_get_interface(interface);

@@ -388,7 +378,6 @@ static int lcd_probe(struct usb_interface *interface,
 		/* something prevented us from registering this driver */
 		dev_err(&interface->dev,
 			"Not able to get a minor for this device.\n");
-		usb_set_intfdata(interface, NULL);
 		goto error;
 	}

@@ -434,14 +423,9 @@ static int lcd_resume(struct usb_interface *intf)
 
 static void lcd_disconnect(struct usb_interface *interface)
 {
-	struct usb_lcd *dev;
+	struct usb_lcd *dev = usb_get_intfdata(interface);
 	int minor = interface->minor;
 
-	mutex_lock(&open_disc_mutex);
-	dev = usb_get_intfdata(interface);
-	usb_set_intfdata(interface, NULL);
-	mutex_unlock(&open_disc_mutex);
-
 	/* give back our minor */
 	usb_deregister_dev(interface, &lcd_class);

Signature · 1 line

2.23.0

Johan Hovold

author

2019-09-26 09:12 UTC #4 lore raw

PATCH 2/4 USB: usblcd: drop redundant disconnect mutex

In reply to Johan Hovold

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

---
 drivers/usb/misc/usblcd.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

drivers/usb/misc/usblcd.c +1 ~~-17~~

index aa982d3ca36b..b898650a5570 100644
@@ -37,9 +37,6 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
-static DEFINE_MUTEX(open_disc_mutex);
-
-
 struct usb_lcd {
 	struct usb_device	*udev;			/* init: probe_lcd */
 	struct usb_interface	*interface;		/* the interface for

@@ -95,17 +92,10 @@ static int lcd_open(struct inode *inode, struct file *file)
 		return -ENODEV;
 	}
 
-	mutex_lock(&open_disc_mutex);
 	dev = usb_get_intfdata(interface);
-	if (!dev) {
-		mutex_unlock(&open_disc_mutex);
-		mutex_unlock(&lcd_mutex);
-		return -ENODEV;
-	}
 
 	/* increment our usage count for the device */
 	kref_get(&dev->kref);
-	mutex_unlock(&open_disc_mutex);
 
 	/* grab a power reference */
 	r = usb_autopm_get_interface(interface);

@@ -388,7 +378,6 @@ static int lcd_probe(struct usb_interface *interface,
 		/* something prevented us from registering this driver */
 		dev_err(&interface->dev,
 			"Not able to get a minor for this device.\n");
-		usb_set_intfdata(interface, NULL);
 		goto error;
 	}

@@ -434,14 +423,9 @@ static int lcd_resume(struct usb_interface *intf)
 
 static void lcd_disconnect(struct usb_interface *interface)
 {
-	struct usb_lcd *dev;
+	struct usb_lcd *dev = usb_get_intfdata(interface);
 	int minor = interface->minor;
 
-	mutex_lock(&open_disc_mutex);
-	dev = usb_get_intfdata(interface);
-	usb_set_intfdata(interface, NULL);
-	mutex_unlock(&open_disc_mutex);
-
 	/* give back our minor */
 	usb_deregister_dev(interface, &lcd_class);

Signature · 1 line

2.23.0

Johan Hovold

author

2019-09-26 09:12 UTC #5 lore raw

PATCH 3/4 USB: usblcd: drop redundant lcd mutex

In reply to Johan Hovold

Drop the redundant lcd mutex introduced by commit 925ce689bb31 ("USB:
autoconvert trivial BKL users to private mutex") which replaced an earlier BKL use.

The lock serialised calls to open() against other open() and a custom ioctl() returning the bcdDevice (sic!), but neither is needed.

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

---
 drivers/usb/misc/usblcd.c | 7 -------
 1 file changed, 7 deletions(-)

drivers/usb/misc/usblcd.c +0 -7

index b898650a5570..732eb1f81368 100644
@@ -30,7 +30,6 @@
 #define IOCTL_GET_DRV_VERSION	2
 
 
-static DEFINE_MUTEX(lcd_mutex);
 static const struct usb_device_id id_table[] = {
 	{ .idVendor = 0x10D2, .match_flags = USB_DEVICE_ID_MATCH_VENDOR, },
 	{ },

@@ -81,12 +80,10 @@ static int lcd_open(struct inode *inode, struct file *file)
 	struct usb_interface *interface;
 	int subminor, r;
 
-	mutex_lock(&lcd_mutex);
 	subminor = iminor(inode);
 
 	interface = usb_find_interface(&lcd_driver, subminor);
 	if (!interface) {
-		mutex_unlock(&lcd_mutex);
 		printk(KERN_ERR "USBLCD: %s - error, can't find device for minor %d\n",
 		       __func__, subminor);
 		return -ENODEV;

@@ -101,13 +98,11 @@ static int lcd_open(struct inode *inode, struct file *file)
 	r = usb_autopm_get_interface(interface);
 	if (r < 0) {
 		kref_put(&dev->kref, lcd_delete);
-		mutex_unlock(&lcd_mutex);
 		return r;
 	}
 
 	/* save our object in the file's private structure */
 	file->private_data = dev;
-	mutex_unlock(&lcd_mutex);
 
 	return 0;
 }

@@ -176,14 +171,12 @@ static long lcd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 	switch (cmd) {
 	case IOCTL_GET_HARD_VERSION:
-		mutex_lock(&lcd_mutex);
 		bcdDevice = le16_to_cpu((dev->udev)->descriptor.bcdDevice);
 		sprintf(buf, "%1d%1d.%1d%1d",
 			(bcdDevice & 0xF000)>>12,
 			(bcdDevice & 0xF00)>>8,
 			(bcdDevice & 0xF0)>>4,
 			(bcdDevice & 0xF));
-		mutex_unlock(&lcd_mutex);
 		if (copy_to_user((void __user *)arg, buf, strlen(buf)) != 0)
 			return -EFAULT;
 		break;

Signature · 1 line

2.23.0

Johan Hovold

author

2019-09-26 09:12 UTC #6 lore raw

cover USB: usblcd: disconnect fix and locking clean ups

In reply to Johan Hovold

This series fixes a failure to stop I/O on disconnect() in the usblcd driver. Turns out there was a lot of legacy cruft in this driver which could simply be removed.

The first patch is marked for stable and could go into v5.4 while the rest is v5.5 material. Posting all at once for completeness.

Tested using a mockup device.

Johan

Johan Hovold (4):
USB: usblcd: fix I/O after disconnect
USB: usblcd: drop redundant disconnect mutex
USB: usblcd: drop redundant lcd mutex
USB: usblcd: use pr_err()

 drivers/usb/misc/usblcd.c | 60 +++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

Signature · 1 line

2.23.0

Johan Hovold

author

2019-09-26 09:17 UTC #7 lore raw

Quoted 3 lines · Johan Hovold

This series fixes a failure to stop I/O on disconnect() in the usblcd driver. Turns out there was a lot of legacy cruft in this driver which could simply be removed.

My apologies for the double post.

Johan

Johan Hovold

author

2019-09-26 09:12 UTC #8 lore raw

PATCH 3/4 USB: usblcd: drop redundant lcd mutex

In reply to Johan Hovold

Drop the redundant lcd mutex introduced by commit 925ce689bb31 ("USB:
autoconvert trivial BKL users to private mutex") which replaced an earlier BKL use.

The lock serialised calls to open() against other open() and a custom ioctl() returning the bcdDevice (sic!), but neither is needed.

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

---
 drivers/usb/misc/usblcd.c | 7 -------
 1 file changed, 7 deletions(-)

drivers/usb/misc/usblcd.c +0 -7

index b898650a5570..732eb1f81368 100644
@@ -30,7 +30,6 @@
 #define IOCTL_GET_DRV_VERSION	2
 
 
-static DEFINE_MUTEX(lcd_mutex);
 static const struct usb_device_id id_table[] = {
 	{ .idVendor = 0x10D2, .match_flags = USB_DEVICE_ID_MATCH_VENDOR, },
 	{ },

@@ -81,12 +80,10 @@ static int lcd_open(struct inode *inode, struct file *file)
 	struct usb_interface *interface;
 	int subminor, r;
 
-	mutex_lock(&lcd_mutex);
 	subminor = iminor(inode);
 
 	interface = usb_find_interface(&lcd_driver, subminor);
 	if (!interface) {
-		mutex_unlock(&lcd_mutex);
 		printk(KERN_ERR "USBLCD: %s - error, can't find device for minor %d\n",
 		       __func__, subminor);
 		return -ENODEV;

@@ -101,13 +98,11 @@ static int lcd_open(struct inode *inode, struct file *file)
 	r = usb_autopm_get_interface(interface);
 	if (r < 0) {
 		kref_put(&dev->kref, lcd_delete);
-		mutex_unlock(&lcd_mutex);
 		return r;
 	}
 
 	/* save our object in the file's private structure */
 	file->private_data = dev;
-	mutex_unlock(&lcd_mutex);
 
 	return 0;
 }

@@ -176,14 +171,12 @@ static long lcd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 	switch (cmd) {
 	case IOCTL_GET_HARD_VERSION:
-		mutex_lock(&lcd_mutex);
 		bcdDevice = le16_to_cpu((dev->udev)->descriptor.bcdDevice);
 		sprintf(buf, "%1d%1d.%1d%1d",
 			(bcdDevice & 0xF000)>>12,
 			(bcdDevice & 0xF00)>>8,
 			(bcdDevice & 0xF0)>>4,
 			(bcdDevice & 0xF));
-		mutex_unlock(&lcd_mutex);
 		if (copy_to_user((void __user *)arg, buf, strlen(buf)) != 0)
 			return -EFAULT;
 		break;

Signature · 1 line

2.23.0

Johan Hovold

author

2019-09-26 09:12 UTC #9 lore raw

PATCH 4/4 USB: usblcd: use pr_err()

In reply to Johan Hovold

Replace the one remaining printk with pr_err().

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

---
 drivers/usb/misc/usblcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/usb/misc/usblcd.c +1 -1

index 732eb1f81368..61e9e987fe4a 100644
@@ -84,7 +84,7 @@ static int lcd_open(struct inode *inode, struct file *file)
 
 	interface = usb_find_interface(&lcd_driver, subminor);
 	if (!interface) {
-		printk(KERN_ERR "USBLCD: %s - error, can't find device for minor %d\n",
+		pr_err("USBLCD: %s - error, can't find device for minor %d\n",
 		       __func__, subminor);
 		return -ENODEV;
 	}

Signature · 1 line

2.23.0

Johan Hovold

author

2019-09-26 09:12 UTC #10 lore raw

PATCH 4/4 USB: usblcd: use pr_err()

In reply to Johan Hovold

Replace the one remaining printk with pr_err().

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

---
 drivers/usb/misc/usblcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/usb/misc/usblcd.c +1 -1

index 732eb1f81368..61e9e987fe4a 100644
@@ -84,7 +84,7 @@ static int lcd_open(struct inode *inode, struct file *file)
 
 	interface = usb_find_interface(&lcd_driver, subminor);
 	if (!interface) {
-		printk(KERN_ERR "USBLCD: %s - error, can't find device for minor %d\n",
+		pr_err("USBLCD: %s - error, can't find device for minor %d\n",
 		       __func__, subminor);
 		return -ENODEV;
 	}

Signature · 1 line

2.23.0