On 02/05/12 22:58, Pete Batard wrote: > On 2012.05.02 17:33, Toby Gray wrote: > This could lead to one of two deadlocks: > > 1) If the transfer completes while usbi_fd_notification() is waiting for > the event lock and the callback attempts to resubmit the transfer. > So transfer completes and we get a handle_events() call that takes the > event lock before usbi_fd_notification() gets a chance to have it. > During this handle_events, callback resubmits the transfer and attempts > to get a lock on itransfer->lock which it cannot obtain. With that lock > already being held and only to be released after usbi_fd_notification() > completes, which won't happen unless usbi_fd_notification() first gets > access to the event lock, that's a deadlock situation alright. Ouch! > >> 2) If the transfer timeout is hit while usbi_fd_notification() is waiting >> for the event lock then the attempt to cancel the transfer will deadlock. > Fairly similar to the above. The first thing cancel_transfer() tries to > do is get a lock on itransfer->lock, and we have the same issue. > > > So first of all, thanks for highlighting this problem - unless I'm > mistaken, that definitely something we need to patch. Did you actually > experience that race condition in practice?
I first saw the resubmit deadlock when running on a 800Mhz CPU. It took about 20 minutes of almost continuous bulk transfers between the host and the USB device before I saw it, so I think it's safe to say it's quite a rare situation. It was only when I started adding lots of logging around all the lock usage to libusbx and stepping through the code path in the debugger that I noticed the second deadlock when canceling timed out transfers. >> This patch fixes both of these deadlocks by having libusb_submit_transfer() >> only call usbi_fd_notification() after having released the transfer mutex. > On principle, this looks okay to me. > > Don't think we have much choice but to move the usbi_fd_notification() > calls that we had in the Windows backend until after the transfer lock > has been released to avoid the deadlock, however, the proposal is fairly > heavy impact, as it requires all backends to be patched. I think adding > an extra field to the struct usbi_transfer, that performs the same > function as your updated_fds, may be better, as it should reduce the > scope of the patch. I haven't tested it but I'm not currently seeing > much of an issue doing it that way --apart from the fact that we'll > duplicate a variable what would be better suited as a global. Still I > think if we can avoid the need to patch all backends, I don't think the > extra field will do much harm. Ok. I'll create a second version of my patch which does it this way and then send it to this list. Looking into it it seemed most sensible to just add another flag to the transfer flags; there is already at least one flag, USBI_TRANSFER_OS_HANDLES_TIMEOUT, which the backend uses to communicate some OS specific information. I'm happy to change it to an entirely new member if that's a better choice than polluting the flags. Regards, Toby ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ _______________________________________________ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
