On Wed, 22 Aug 2012, 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.
...
> + if ((dc.flags & USBDEVFS_DISCONNECT_CLAIM_IF_DRIVER) &&
> + strncmp(dc.driver, intf->dev.driver->name,
> + sizeof(dc.driver)) != 0)
> + return -EBUSY;
> +
> + if ((dc.flags & USBDEVFS_DISCONNECT_CLAIM_EXCEPT_DRIVER) &&
> + strncmp(dc.driver, intf->dev.driver->name,
> + sizeof(dc.driver)) == 0)
> + return -EBUSY;
Please don't indent continuation lines by exactly one tab stop. It
makes them look like the start of a sub-block.
> On 08/22/2012 02:04 PM, Oliver Neukum wrote:
...
> >> + 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.
Right. Oliver, driver->name is used in other places too, like
dev_driver_string() in the device core (its complications concern
whether or not the device is bound to a driver, not retrieving the
driver's name).
> >> +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.
There's no reason to specify an explicit size. Sizes are needed when
structures have to match up with external data (such as USB
descriptors), but that's not true here. The only requirement is that
the compilers used to build the kernel and the application agree on the
size of unsigned int. If they didn't then things like struct dirent
wouldn't work (it contains an unsigned short).
Now if the type were unsigned long then you'd have to worry about
32-bit vs. 64-bit compatibility. In that case an explicit size would
be justified. But not for unsigned int.
Alan Stern
--
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
