Re: Use of usb_find_interface in open is racy

[Russ Dill <russ.dill@gmail.com>]

On Wed, Nov 18, 2009 at 8:39 AM, Greg KH <greg@kroah.com> wrote:
> On Wed, Nov 18, 2009 at 10:31:47AM -0500, Alan Stern wrote:
>> On Wed, 18 Nov 2009, Jiri Kosina wrote:
>>
>> > On Tue, 17 Nov 2009, Russ Dill wrote:
>> >
>> > > Many usb drivers that create character devices use "struct
>> > > usb_class_driver", a set of fops, and a usb_find_interface in their
>> > > open call. A prime example is drivers/usb/usb-skeleton.c. A race
>> > > occurs when userspace receives a hotplug event for the addition for
>> > > the interface and then opens the associated device file before the
>> > > device is added to the driver's klist_devices.
>> > >
>> > > The usb core senses a new usb device (usb_new_device) and calls
>> > > device_add. This eventually gets down to really_probe and the
>> > > usb-skeleton probe function, skel_probe. skel_probe calls
>> > > usb_register_dev() which registers the associated character device for
>> > > skel_class. The hotplug events for the class device get emitted.
>> > >
>> > > User space receives the hotplug event for the class device, makes the
>> > > device node and notifies another program that opens the device node.
>> > > The program opens the device node which calls into usb_open and then
>> > > skel_open. skel_open calls usb_find_interface. usb_find_interfaces
>> > > searches the klist_devices of skel_driver, finds no device associated
>> > > with the minor number and returns NULL. skel_open returns -ENODEV.
>> > >
>> > > Control returns to really_probe and really_probe calls driver_bound
>> > > which adds the device to the list of devices associated with
>> > > skel_driver (klist_devices).
>> > >
>> > > I'm not sure what the right way to solve this is. A call to
>> > > wait_for_device_probe() in the skel_open call before calling
>> > > usb_find_interface fixes the problem, but it is a rather large hammer.
>>
>> I think the proper answer is for usb_find_interface() to use
>> bus_for_each_dev() instead of driver_for_each_device().
>
> Yeah, I think so, sorry about that I think this is my fault :(
>
> I'm travelling all today, could someone make up a patch for this before
> Thursday if it's an issue?
>
> Russ, have you ever hit this problem, or did you just find it by looking
> at the code?
>

Yes, I'm utilizing devtmpfs to create device nodes and I have an app
that handles hotplug events directly (no udev). Even with that small
amount of overhead, the race does not occur every time, but it still
occurs most of the time.

My initial fix was to add the device to the driver's device list
sooner, but the change to usb_find_interface seems less intrusive and
less likely to cause problems with other code that calls
driver_for_each_device, driver_find_device, or uses
BUS_NOTIFY_BOUND_DRIVER.

I'll work up a patch with the change to usb_find_interface, do some
testing, and email it.