On Tue, Feb 07, 2023 at 12:52:16PM -0500, Alan Stern wrote:
> On Tue, Feb 07, 2023 at 09:25:55AM +0100, Troels Liebe Bentsen wrote:
> > Hi again,
> >
> > Did a bit more testing and found another lock that would be nice to remove,
> > the /dev/bus/usb/$BUS/$DEV file for the hub is also locked:
> >
> > Bus 003 Device 016: ID 1a40:0201 Terminus Technology Inc. FE 2.1 7-port Hub
> >
> > strace lsusb -v
> > ...
> > openat(AT_FDCWD, "/dev/bus/usb/003/016", O_RDWR|O_CLOEXEC...
> >
> > The openat can not be canceled from userspace and even kill -9 won't stop
> > the process until the descriptor read times out.
>
> Yes, that really should be an interruptible lock. In fact, all the
> locks connected with usbfs should be interruptible.
>
> However, it can't be eliminated entirely. This is a case where two
> things end up being mutually exclusive with each other because they both
> need to be mutually exclusive with a third thing. In other words,
> opening a hub's usbfs file and probing a hub's children both have to be
> mutually exclusive with disconnecting the hub. The three pathways all
> use the device lock for this purpose, so they are all mutually exclusive
> with each other.
>
> > Also managed to get "lsusb -v" to hang in an unkillable way even after
> > I unplugged the device and hub:
> >
> > ) = 1 ([{fd=5, revents=POLLIN}])
> > ioctl(8, USBDEVFS_DISCARDURB
>
> Making these lock calls interruptible should fix this problem, right?
Here's a patch. It should fix most of these problems.
Alan Stern
Index: usb-devel/drivers/usb/core/devio.c
===================================================================
--- usb-devel.orig/drivers/usb/core/devio.c
+++ usb-devel/drivers/usb/core/devio.c
@@ -303,13 +303,15 @@ static ssize_t usbdev_read(struct file *
{
struct usb_dev_state *ps = file->private_data;
struct usb_device *dev = ps->dev;
- ssize_t ret = 0;
+ ssize_t ret;
unsigned len;
loff_t pos;
int i;
pos = *ppos;
- usb_lock_device(dev);
+ ret = usb_lock_device_interruptible(dev);
+ if (ret < 0)
+ return ret;
if (!connected(ps)) {
ret = -ENODEV;
goto err;
@@ -1039,7 +1041,9 @@ static int usbdev_open(struct inode *ino
if (!dev)
goto out_free_ps;
- usb_lock_device(dev);
+ ret = usb_lock_device_interruptible(dev);
+ if (ret < 0)
+ goto out_put_device;
if (dev->state == USB_STATE_NOTATTACHED)
goto out_unlock_device;
@@ -1071,6 +1075,7 @@ static int usbdev_open(struct inode *ino
out_unlock_device:
usb_unlock_device(dev);
+ out_put_device:
usb_put_dev(dev);
out_free_ps:
kfree(ps);
@@ -2591,14 +2596,17 @@ static long usbdev_do_ioctl(struct file
struct usb_dev_state *ps = file->private_data;
struct inode *inode = file_inode(file);
struct usb_device *dev = ps->dev;
- int ret = -ENOTTY;
+ int ret;
if (!(file->f_mode & FMODE_WRITE))
return -EPERM;
- usb_lock_device(dev);
+ ret = usb_lock_device_interruptible(dev);
+ if (ret < 0)
+ return ret;
/* Reap operations are allowed even after disconnection */
+ ret = -ENOTTY;
switch (cmd) {
case USBDEVFS_REAPURB:
snoop(&dev->dev, "%s: REAPURB\n", __func__);
