On Fri, 9 Jan 2009, Oliver Neukum wrote:
> upon further thought this code is still racy.
>
> retval = usb_register_dev(usbhid->intf, &hiddev_class);
>
> here you open a window during which open can happen
>
> if (retval) {
> err_hid("Not able to get a minor for this device.");
> hid->hiddev = NULL;
> kfree(hiddev);
> return -1;
> } else {
> hid->minor = usbhid->intf->minor;
> hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = hiddev;
>
> and will fail because hiddev_table hasn't been updated
Indeed, thanks for catching that. It's a small race window and not very
probably to be hit (or users aren't that fast, right? :) ).
> The obvious fix of using a mutex to guard hiddev_table doesn't work
> because usb_open() and usb_register_dev() take minor_rwsem and we'd have
> an AB-BA deadlock. We need a lock usb_open() also takes in the right
> order and that leaves only one option, BKL. I don't like it but I see no
> alternative.
[ ... ]
> @@ -878,16 +878,19 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
> hiddev->hid = hid;
> hiddev->exist = 1;
>
> + lock_kernel();
> retval = usb_register_dev(usbhid->intf, &hiddev_class);
> if (retval) {
> err_hid("Not able to get a minor for this device.");
> hid->hiddev = NULL;
> + unlock_kernel();
> kfree(hiddev);
> return -1;
> } else {
> hid->minor = usbhid->intf->minor;
> hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = hiddev;
> }
> + unlock_kernel();
This is really not nice and I would like to avoid pulling BKL in as much
as possible.
What exactly is the lock_kernel() in usb_open() protecting from anyway?
Why isn't minor_rwsem sufficient protection?
--
Jiri Kosina
SUSE Labs
--
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
