Hey,
Hans de Goede wrote:
> doing the disarm without the flying-transfers lock held is racy, ie:
Bringing back the old behavior was a start. Yes, race also. The
attached patch on top of the previous fix avoids the race, and is
available also via
git fetch git://git.libusb.org/libusb-stuge.git for_libusbx/timerfd_race && \
git cherry-pick FETCH_HEAD
> the setting of the timerfd from libusb_submit_transfer should
> also be moved to add_to_flying_list() and be done under the
> protected of the flying-transfers lock.
It makes sense from the point of view of critical sections, but
starting the timer before the transfer has actually been submitted
would mean that the timeout while the transfer is on the bus will
always be shorter than the user-specified timeout, offset by however
long it takes to do the submit itself - while starting the timer
*after* submitting the transfer means that timeout while in flight
will always be ever so slightly longer than the user-specified value.
I don't like that API semantic change so much, and I also think that
submitting a transfer is more likely to take time than setting up the
timer is, thus also costing a larger timeout error.
I think the smaller timeout error and most importantly having the
user-specified timeout be a lower bound rather than an upper bound
is preferable.
//Peter
>From ca77a1a541aaa3b2f0cbc4ea03e1eeca3dd5aa5a Mon Sep 17 00:00:00 2001
From: Peter Stuge <pe...@stuge.se>
Date: Tue, 10 Jul 2012 16:54:16 +0200
Subject: [PATCH] io.c: Avoid timerfd race condition between completion and
new submit
An event handler thread working on transfer completion for the last
flying transfer with a timeout can end up racing with a call to
libusb_submit_transfer() from a second thread, so that the timerfd
gets disarmed even though libusb actually again has a transfer with
a timeout.
By arming or disarming the timerfd during completion strictly
according to remaining flying transfers while also holding the
flying_transfers_lock this change ensures that a new transfer can
not be added to the flying list until the completion code path has
armed/disarmed the timerfd according to the current flying list.
Hans de Goede describes the race condition situation in
http://sourceforge.net/mailarchive/message.php?msg_id=29520709
libusb.git commit c5194b408286229ce0d94765f963890057d46ee0
Signed-off-by: Peter Stuge <pe...@stuge.se>
---
libusb/io.c | 15 +++++----------
1 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/libusb/io.c b/libusb/io.c
index cf41ee6..666f6da 100644
--- a/libusb/io.c
+++ b/libusb/io.c
@@ -1457,19 +1457,14 @@ int usbi_handle_transfer_completion(struct
usbi_transfer *itransfer,
usbi_mutex_lock(&ctx->flying_transfers_lock);
list_del(&itransfer->list);
- if (usbi_using_timerfd(ctx))
- r = arm_timerfd_for_next_timeout(ctx);
- usbi_mutex_unlock(&ctx->flying_transfers_lock);
-
if (usbi_using_timerfd(ctx)) {
- if (r < 0)
- return r;
- else if (0 == r) {
+ r = arm_timerfd_for_next_timeout(ctx);
+ if (0 == r)
r = disarm_timerfd(ctx);
- if (r < 0)
- return r;
- }
}
+ usbi_mutex_unlock(&ctx->flying_transfers_lock);
+ if (r < 0)
+ return r;
if (status == LIBUSB_TRANSFER_COMPLETED
&& transfer->flags & LIBUSB_TRANSFER_SHORT_NOT_OK) {
--
1.7.4.1.343.ga91df.dirty
------------------------------------------------------------------------------
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
