Hi,
Thanks for the review!
On 08/22/2012 02:04 PM, Oliver Neukum wrote:
On Wednesday 22 August 2012 13:42:47 Hans de Goede wrote:
Apps which deal with devices which also have a kernel driver, need to do
the following:
1) Check which driver is attached, so as to not detach the wrong driver
(ie detaching usbfs while another instance of the app is using the device)
2) Detach the kernel driver
3) Claim the interface
Where moving from one step to the next for both 1-2 and 2-3 consists of
a (small) race window. So currently such apps are racy and people just live
with it.
This patch adds a new ioctl which makes it possible for apps to do this
in a race free manner. For flexibility apps can choose to:
1) Specify the driver to disconnect
2) Specify to disconnect any driver except for the one named by the app
3) Disconnect any driver
Note that if there is no driver attached, the ioctl will just act like the
regular claim-interface ioctl, this is by design, as returning an error for
this condition would open a new bag of race-conditions.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/usb/core/devio.c | 35 +++++++++++++++++++++++++++++++++++
include/linux/usbdevice_fs.h | 14 ++++++++++++++
2 files changed, 49 insertions(+)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index ebb8a9d..829edce 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1928,6 +1928,38 @@ static int proc_get_capabilities(struct dev_state *ps, void __user *arg)
return 0;
}
+static int proc_disconnect_claim(struct dev_state *ps, void __user *arg)
+{
+ struct usbdevfs_disconnect_claim dc;
+ struct usb_interface *intf;
+
+ if (copy_from_user(&dc, arg, sizeof(dc)))
+ return -EFAULT;
+
+ intf = usb_ifnum_to_if(ps->dev, dc.interface);
+ if (!intf)
+ return -EINVAL;
+
+ if (intf->dev.driver) {
+ struct usb_driver *driver = to_usb_driver(intf->dev.driver);
+
+ if ((dc.flags & USBDEVFS_DISCONNECT_CLAIM_IF_DRIVER) &&
+ strncmp(dc.driver, intf->dev.driver->name,
+ sizeof(dc.driver)) != 0)
You have no idea what is in the memory behind dev.driver->name which you
will happily compare to and thus access. Potentially you violate the DMA coherency
rules here.
Actually what is there is valid, kernel mapped (readable) memory containing a 0 terminated
string. See for example also the implementation of proc_getdriver() in devio.c:
if (!intf || !intf->dev.driver)
ret = -ENODATA;
else {
strncpy(gd.driver, intf->dev.driver->name,
sizeof(gd.driver));
ret = (copy_to_user(arg, &gd, sizeof(gd)) ? -EFAULT : 0);
}
Exactly the same usage of intf->dev.driver->name except as a strncpy source
rather then a strncmp argument.
+ return -EBUSY;
+
+ if ((dc.flags & USBDEVFS_DISCONNECT_CLAIM_EXCEPT_DRIVER) &&
+ strncmp(dc.driver, intf->dev.driver->name,
+ sizeof(dc.driver)) == 0)
+ return -EBUSY;
Both flags could be set. You should catch that case.
I already thought about that, if both flags are set userspace will always
get an -EBUSY return. I see that as userspace's problem, if they don't like
it they should not set both flags. I don't think adding a separate check
and returning -EINVAL is worth the trouble.
+
+ dev_dbg(&intf->dev, "disconnect by usbfs\n");
+ usb_driver_release_interface(driver, intf);
+ }
+
+ return claimintf(ps, dc.interface);
So you may return an error and yet execute a part of the command.
Good point, but no, claimintf will always succeed here, it will only fail if:
1) dc.interface does not point to a valid interface which we already checked
2) usb_driver_claim_interface() returns -EBUSY, which it won't since we just
detached the driver.
And the USB device lock is held by do_ioctl while we're called so nothing can
change things underneath us.
I guess this could be made more clear by changing the end of the function to:
/* claimintf cannot fail here, as all conditions it checks are met */
claimintf(ps, dc.interface);
return 0;
}
+}
+
/*
* NOTE: All requests here that have interface numbers as parameters
* are assuming that somehow the configuration has been prevented from
@@ -2101,6 +2133,9 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
case USBDEVFS_GET_CAPABILITIES:
ret = proc_get_capabilities(ps, p);
break;
+ case USBDEVFS_DISCONNECT_CLAIM:
+ ret = proc_disconnect_claim(ps, p);
+ break;
}
usb_unlock_device(dev);
if (ret >= 0)
diff --git a/include/linux/usbdevice_fs.h b/include/linux/usbdevice_fs.h
index 3b74666..4abe28e 100644
--- a/include/linux/usbdevice_fs.h
+++ b/include/linux/usbdevice_fs.h
@@ -131,6 +131,19 @@ struct usbdevfs_hub_portinfo {
#define USBDEVFS_CAP_NO_PACKET_SIZE_LIM 0x04
#define USBDEVFS_CAP_BULK_SCATTER_GATHER 0x08
+/* USBDEVFS_DISCONNECT_CLAIM flags & struct */
+
+/* disconnect-and-claim if the driver matches the driver field */
+#define USBDEVFS_DISCONNECT_CLAIM_IF_DRIVER 0x01
+/* disconnect-and-claim except when the driver matches the driver field */
+#define USBDEVFS_DISCONNECT_CLAIM_EXCEPT_DRIVER 0x02
+
+struct usbdevfs_disconnect_claim {
+ unsigned int interface;
+ unsigned int flags;
+ char driver[USBDEVFS_MAXDRIVERNAME + 1];
+};
You export this to user space. Please, please use u32 and u8.
I can understand where you're coming from, but:
1) If you search for interface in include/linux/usbdevice_fs.h, all
declarations of it are unsigned int, so using u32 would be inconsistent
2) If you look at the existing struct usbdevfs_getdriver it contains:
char driver[USBDEVFS_MAXDRIVERNAME + 1];
3) If you look at the existing struct usbdevfs_urb it contains:
unsigned int flags;
So making the requested change would be inconsistent. With that said I'm fine
either way, but it seems better to keep things as is.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
