Hi Greg, On Monday 02 August 2004 21:59, Greg KH wrote: > On Thu, Jul 29, 2004 at 06:35:26PM +0200, Duncan Sands wrote: > > +static int start_wait_urb(struct usb_device *dev, struct urb *urb, int timeout, > > int* actual_length) > > Hm, with the exception of these two lines: > > > + up(&dev->serialize); > > + wait_for_completion(&done); > > + down(&dev->serialize); > > I don't see how this is any different from usb_start_wait_urb(). You > say it's interruptable, but if it is, then why isn't > usb_start_wait_urb()? Why can't you just use it (or really the > functions that call it), by dropping the semaphore before calling it?
it isn't interruptible... yet. Anyway, making it interruptible is a side issue. The real question is: why is using usb_start_wait_urb racy, and how do the above lines fix the problem? It is all a question of whether dev->serialize is released before calling usb_submit_urb, or after. If you use usb_start_wait_urb, then dev->serialize is dropped before the call to usb_submit_urb. With my patch, it is dropped after the call. Here is an example race if you use usb_start_wait_urb: (A) you call proc_bulk to send an urb (to an endpoint E, say) (A) dev->serialize is dropped in proc_bulk (B) someone changes the current configuration. This causes usbfs's disconnect method to be called (which, incidentally, causes it to "unclaim" any interfaces it claimed). Since we have a reference to the struct usb_device, there is no need to worry about things going "boom". Suppose the new configuration has an endpoint with the same pipe as E. (C) usb_submit_urb is now called in usb_start_wait_urb. This sends the urb to the new endpoint, which is the wrong endpoint... There seems to be general agreement that this race exists. Submitting the urb before dropping the lock avoids the race. Probably there are other similar races, for example when you change the altsetting. I wrote the patch the way I did in order to avoid these races. However, (1) the race still exists in another form, and (2) who cares about this race, anyway? (1) Still racy: since proc_bulk takes the endpoint number as an argument, if the configuration changes just before you call proc_bulk, the urb will happily be sent to the wrong endpoint. Let me say more on this point: before submitting an urb, you are supposed to claim the interface. If the configuration changes, then the interface is "unclaimed". If you send the urb after the configuration changes, then you are sending it to an interface you haven't claimed. Therefore the submission should fail and the race is avoided... except that it isn't: if you haven't claimed the interface, then usbfs automatically claims it for you, and your urb is happily sent to the wrong place. Summary: to avoid the race, not only do you need my patch the way it is, you also need to stop usbfs from autoclaiming interfaces. In my opinion, it is a mistake to have usbfs automatically claim interfaces. It would be easy to change, but it would also break many programs. We could mark autoclaiming as deprecated, and remove it in a years time. That means that you get no benefit from not using usb_start_wait_urb for at least one year. (2) Who cares? It seems rather unlikely that anyone will hit this (*). Also, any driver which sometimes submits urbs after disconnect has been called presumably suffers from the same problem. Should usbfs do better? Since everyone knows that usbfs is a piece of crud, soon to be replaced by usbfs2, why should we bother with these unlikely cases? Of course, let us pity the poor person who ends up sending an urb to the wrong endpoint, and then wonders why his task is stuck in the (uninterruptible) D state because his urb is not completing (this is possible). Here is what I suggest. I send you a patch that fixes up proc_bulk and proc_control as my current patch does, but doesn't push taking dev->serialize down into the individual ioctl methods. All the best, Duncan. (*) This argument cuts no ice with me, I'm just playing the devil's advocate here. ------------------------------------------------------- This SF.Net email is sponsored by OSTG. Have you noticed the changes on Linux.com, ITManagersJournal and NewsForge in the past few weeks? Now, one more big change to announce. We are now OSTG- Open Source Technology Group. Come see the changes on the new OSTG site. www.ostg.com _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
