Hi Toby, On 2012.05.02 17:33, Toby Gray wrote: > Without this change the Windows backend needed to call usbi_fd_notification() > from within the backend's submit_transfer.
OK. I guess we want to sort this issue before we go with the anticipated 1.0.10 release that reinstated the missing usbi_fd_notification() calls then. > This could cause deadlock > when attempting to lock the event lock if another thread was processing > events on the just-submitted transfer. > > The deadlock comes about as the thread calling libusb_submit_transfer acquires > the transfer mutex before trying to acquire the event lock; Just making sure I get the issue straight. The first thing libusb_submit_transfer() does is indeed call on usbi_mutex_lock(&itransfer->lock), whereas, as far as Windows is concerned, libusb_lock_events(ctx) is only called when invoking usbi_fd_notification() in submit_xxxx_transfer(). So far so good. > this is the other > order of lock acquisition from an event thread handling activity on the just > submitted transfer. So that would be from libusb_handle_events_timeout_xxxx(). One of the first thing we do there is indeed libusb_try_lock_events(ctx), but I don't see a mutex lock on the transfer->lock, though, from what you explain below, it would be issued from a transfer being resubmitted or cancelled, which is OK. > 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? > 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. I'm gonna wait a little to see if others want to comment on that. Regards, /Pete ------------------------------------------------------------------------------ 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
