[Libusbx-devel] [PATCH] Fix unconditional disarming of timerfd (was: Bug in libusb timeout handling?)
I expect the attached patch to fix the likely bug reported by Hans.
As previously indicated, we should be able to either rearm the next
timer or disarm all timers in the arm_timerfd_for_next_timeout() call of
io.c, especially as we always issue it with the flying_transfers locked.
Note that this patch may result in the timerfd being disarmed twice in
handle_timerfd_trigger(). Apart from a slight overhead, I don't expect
that to be a problem. Especially, the timerfd_settime() man page does
not indicate that disarming a timerfd twice will result in an error
code. Of course, to avoid that we could try to remove the unconditional
disarm call at the beginning of handle_timerfd_trigger(), but I believe
we very much want to have it before issuing handle_timeouts_locked(), to
avoid potentially accumulating a whole bunch of timerfd events, while we
already are in the process of checking for timeouts anyway.
Also, I haven't tested this change under the conditions where I'd expect
the issue to manifest itself, so there is a possibility the patch (or
the reality of the problem) is still off.
Regards,
/Pete
>From 04ad04771e7cb6a50f6a0a5eb2481ee6989ebe0e Mon Sep 17 00:00:00 2001
From: Pete Batard
Date: Tue, 10 Jul 2012 01:59:45 +0100
Subject: [PATCH] Core: Fix unconditional disarming of timerfd
* Existing code appears to disarm the timerfd always, which
would cancel all pending timeouts as soon as one packet completes.
* This fix moves the disarming of the timerfd to
arm_timerfd_for_next_timeout(), where it is now done conditionally.
* Issue reported by Hans de Goede. For more info, see:
https://sourceforge.net/mailarchive/message.php?msg_id=29442693
---
libusb/io.c | 22 +++---
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/libusb/io.c b/libusb/io.c
index d06d375..9296ff3 100644
--- a/libusb/io.c
+++ b/libusb/io.c
@@ -1403,7 +1403,7 @@ static int arm_timerfd_for_next_timeout(struct
libusb_context *ctx)
/* if we've reached transfers of infinite timeout, then we have
no
* arming to do */
if (!timerisset(cur_tv))
- return 0;
+ return disarm_timerfd(ctx);
/* act on first transfer that is not already cancelled */
if (!(transfer->flags & USBI_TRANSFER_TIMED_OUT)) {
@@ -1418,14 +1418,9 @@ static int arm_timerfd_for_next_timeout(struct
libusb_context *ctx)
}
}
- return 0;
+ return disarm_timerfd(ctx);
}
#else
-static int disarm_timerfd(struct libusb_context *ctx)
-{
- (void)ctx;
- return 0;
-}
static int arm_timerfd_for_next_timeout(struct libusb_context *ctx)
{
(void)ctx;
@@ -1457,17 +1452,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;
- r = disarm_timerfd(ctx);
- if (r < 0)
+ r = arm_timerfd_for_next_timeout(ctx);
+ if (r < 0) {
+ usbi_mutex_unlock(&ctx->flying_transfers_lock);
return r;
+ }
}
+ usbi_mutex_unlock(&ctx->flying_transfers_lock);
if (status == LIBUSB_TRANSFER_COMPLETED
&& transfer->flags & LIBUSB_TRANSFER_SHORT_NOT_OK) {
--
1.7.11.msysgit.0
--
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
Re: [Libusbx-devel] libusb segfaults - causes pcscd to crash
On 2012.07.09 09:48, sebasti...@gmx-topmail.de wrote: > Setting the debug level to 4 had no impact on the logging behavior of pcscd. This is unfortunate, as we are missing a lot of information with regards to what libusbx is doing if level 4/debug is not in use. Especially, we still have no idea what may have been going on at the time of the new crash you experienced, which makes it difficult to know where we should look. > Only when started in foreground, pcscd provides detailed debug information of > libusb in the console, but does not write them to the log file. I still think we will need to get some debug output to be recorded one way or another. If pcscd only picks up the stderr output of libusbx with the --foreground option, this may require patching pcscd to also do the same when running as a daemon. Or you could patch usbi_log_v() in core.c to send its logging output to a file instead of stderr, and recompile the library. 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
Re: [Libusbx-devel] [PATCH] Fix unconditional disarming of timerfd
Hi, On 07/10/2012 01:01 PM, Pete Batard wrote: > I expect the attached patch to fix the likely bug reported by Hans. > > As previously indicated, we should be able to either rearm the next timer or > disarm all timers in the arm_timerfd_for_next_timeout() call of io.c, > especially as we always issue it with the flying_transfers locked. > > Note that this patch may result in the timerfd being disarmed twice in > handle_timerfd_trigger(). Apart from a slight overhead, I don't expect that > to be a problem. Especially, the timerfd_settime() man page does not indicate > that disarming a timerfd twice will result in an error code. Of course, to > avoid that we could try to remove the unconditional disarm call at the > beginning of handle_timerfd_trigger(), but I believe we very much want to > have it before issuing handle_timeouts_locked(), to avoid potentially > accumulating a whole bunch of timerfd events, while we already are in the > process of > checking for timeouts anyway. > > Also, I haven't tested this change under the conditions where I'd expect the > issue to manifest itself, so there is a possibility the patch (or the reality > of the problem) is still off. I had looking further into this / writing a patch on my todo list, looks like you beat me to it, thanks for working on this! You're patch looks good, one possible optimization would be to give arm_timerfd_for_next_timeout() an extra "int stop_timer" argument and only call disarm_timerfd() when that argument is true. In the arm_timerfd_for_next_timeout() call from usbi_handle_transfer_completion() this argument would be true, not changing anything from your last revision, but in the call from handle_timerfd_trigger() it would be false, avoiding an unnecessary call to disarm_timerfd(). As said this is pure optimization, but given that the scenario where no other packets are pending when a timeout hits is not unlikely, I think it is worth it. Regards, Hans -- 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
Re: [Libusbx-devel] [PATCH] Fix unconditional disarming of timerfd
On 2012.07.10 12:18, Hans de Goede wrote: > one possible optimization would be to > give arm_timerfd_for_next_timeout() an extra "int stop_timer" > argument and only call disarm_timerfd() when that argument is true. I like that idea. I'll get on to it and submit a v2 when I get a chance. 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
Re: [Libusbx-devel] [PATCH] Fix unconditional disarming of timerfd
Hans de Goede wrote:
> I had looking further into this / writing a patch on my todo list,
> looks like you beat me to it, thanks for working on this!
>
> You're patch looks good, one possible optimization would be to
> give arm_timerfd_for_next_timeout() an extra "int stop_timer"
> argument and only call disarm_timerfd() when that argument is true.
The proposed patch is a bit more complicated than neccessary, and
arm_timerfd_for_next_timeout() uses the return value to indicate
if a timer is now running or not. Thanks to debugging by Vincent
the bug was fixed in libusb yesterday.
I'm attaching a patch that applies on top of libusbx.git, and for
convenience it is also available via
git fetch git://git.libusb.org/libusb-stuge.git for_libusbx/timerfd_fix && \
git cherry-pick FETCH_HEAD
//Peter
>From 26ff10182876b67cc6035d4db7f2732dd3487d44 Mon Sep 17 00:00:00 2001
From: Peter Stuge
Date: Tue, 10 Jul 2012 01:46:38 +0200
Subject: [PATCH] io.c: Only disarm timerfd when no flying transfer has a
timeout
Commit 4630fc22cff8ad3e1afa9b223378c0aabe282b5c made libusb work
correctly when built to use timerfd but run on a kernel without
timerfd support. The commit unfortunately also broke the logic
during transfer completion which decides if the timerfd will be
disarmed or not, and disarm_timerfd() was called even if the
timerfd had already been armed with the next flying transfer
timeout.
Before the offending commit the timerfd would be disarmed only
when arm_timerfd_for_next_timeout() reported that there was no
flying transfer with a timeout. Let's restore that behavior.
The bug was spotted through code review both by the author and
by Hans de Goede, but was not squashed until Vincent Pellier
experienced transfers never timing out, and helped confirm
the source of the problem. Many thanks!
References http://libusb.org/ticket/73
libusb.git commit 1bd831c4e88857bff2f1670c89eda1d04da1cc54
Signed-off-by: Peter Stuge
---
libusb/io.c |8 +---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/libusb/io.c b/libusb/io.c
index d06d375..cf41ee6 100644
--- a/libusb/io.c
+++ b/libusb/io.c
@@ -1464,9 +1464,11 @@ int usbi_handle_transfer_completion(struct usbi_transfer
*itransfer,
if (usbi_using_timerfd(ctx)) {
if (r < 0)
return r;
- r = disarm_timerfd(ctx);
- if (r < 0)
- return r;
+ else if (0 == r) {
+ r = disarm_timerfd(ctx);
+ if (r < 0)
+ return r;
+ }
}
if (status == LIBUSB_TRANSFER_COMPLETED
--
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
Re: [Libusbx-devel] [PATCH] Fix unconditional disarming of timerfd
Hi, On 07/10/2012 02:40 PM, Peter Stuge wrote: > Hans de Goede wrote: >> I had looking further into this / writing a patch on my todo list, >> looks like you beat me to it, thanks for working on this! >> >> You're patch looks good, one possible optimization would be to >> give arm_timerfd_for_next_timeout() an extra "int stop_timer" >> argument and only call disarm_timerfd() when that argument is true. > > The proposed patch is a bit more complicated than neccessary, and > arm_timerfd_for_next_timeout() uses the return value to indicate > if a timer is now running or not. Thanks to debugging by Vincent > the bug was fixed in libusb yesterday. > > I'm attaching a patch that applies on top of libusbx.git, and for > convenience it is also available via Thanks! And good to see confirmed that there actually is an issue there which needs fixing. With that said I think Pete's fix is better though, doing the disarm without the flying-transfers lock held is racy, ie: Thread 1: -entering usbi_handle_transfer_completion() -calls arm_timerfd_for_next_timeout(), which returns 0 -unlock flying transfer-lock Switch to thread 2 -entering libusb_submit_transfer() -calls add_to_flying_list(), which returns 1 -sets up the timerfd for the just submitted transfer, as add_to_flying_list() returned 1 Switch back to thread 1: -check arm_timerfd_for_next_timeout() return val -retval == 0, so disarm timer_fd And now the timer_fd has been disabled even though it should be set to fire for the timeout of the transfer submitted by thread 2. Speaking more in general, the timerfd is a single resource shared by all transfers, so it should be protected from concurent accesses. So I not only think Pete's patch is better, IMHO 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. Regards, Hans -- 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
Re: [Libusbx-devel] libusb segfaults - causes pcscd to crash
Hello Pete, today, I was able to redirect the libusbx debug information (log level 4) to a file in the test environment: 'LIBUSB_DEBUG=4 /usr/sbin/pcscd --foreground --debug --apdu &> /var/log/pcscd.log' Setting the log level to 4 generates huge CPU load and the log file grows very fast (200-300 Kbyte/s). Rotating the log does not work the way it should. Although logrotate renames the old logfile to pcscd.log.1 and creates a new one, but pcscd keeps writing to the old file handle. The new log file remains empty. I’m afraid I’ll not be able to introduce this extensive logging in the productive environment. The only thing I can do is to run the command above on the test computer and wait for the segmentation fault. But up till now it never appeared on the test computer, which could lead to a very long waiting time… > On 2012.07.10 12:14, Pete Batard wrote: > I still think we will need to get some debug output to be recorded one > way or another. If pcscd only picks up the stderr output of libusbx with > the --foreground option, this may require patching pcscd to also do the > same when running as a daemon. Or you could patch usbi_log_v() in core.c > to send its logging output to a file instead of stderr, and recompile > the library. I’ll have a look at the code. Thanks for your help! - Regards Sebastian = -- 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
Re: [Libusbx-devel] Question about libusb_get_pollfds?
Hi Peter, On 2012-07-09, at 5:28 PM, Peter Stuge wrote: > Kevyn-Alexandre Paré wrote: >>> Data never becomes available. >>> The application asks the device to send data, when the application >>> knows that data is supposed to be available in the device. >> >> Agree, in my case I send data to an FPGA and I'm always expecting to >> receive data from it. The application send the data to the device >> and expect data back. > > All right, but until the application tries to receive data, the > device has no way of indicating that it has data to send. Yes > > >> My loop event doesn't need to manage these fds by it's self but >> simply call libusb_handle_events(). Then libusb will call my >> callback function, that I register with libusb_fill_bulk_transfer. >> In the callback I will manage what I want to do depending on the >> status?!?! > > Correct. You add the fds from libusb to your own set of fds, and you > call libusb_handle_events() when poll() returns because of an event > on any of the libusb fds. Thx this was the part that I didn't understand properly. > > A A Proposition could be that libusb support nice API to distinguish them??: >>> >>> No, the fd:s from libusb are opaque. >> >> Could you explain what you mean by opaque? > > Opaque is the opposite of transparant. In the context of data > structures this means that you do not know what is "inside" the data > structure. In the case of fds you know that they will be fd numbers, > here opaque means that you do not know what any event on any fd > means. This was another reason why I miss use the libusb_get_pollfds. > > >>> It's a common misunderstanding that devices can communicate with the >>> application without them being asked to transfer any data. >> >> This is not my case, I'm always sending data to the device and then >> expecting data from it. > > The device can anyway *not* send to the application *before* the > application asks for data. thx -- KA > > > //Peter > > -- > 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 -- 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
Re: [Libusbx-devel] [PATCH] Fix unconditional disarming of timerfd
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
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
---
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
Re: [Libusbx-devel] [PATCH] Fix unconditional disarming of timerfd
On 2012.07.10 16:31, Peter Stuge wrote: > 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 Don't we issue a calculate_timeout() call first thing in libusb_submit_transfer() though? My understanding is that, whether we start the timeout before or after the transfer, the actual point at which the timeout expires won't change since it's calculated first thing in the transfer call, as it should, so I fail to see any added offset either way. Especially it seems logical to want any software overhead to be part of our timeout. Otherwise, trying to determine when a timeout will actually expire will require some guesswork for our users. As such, we might as well have the timeout initiated as soon as we get the lock, in add_to_flying_list(), regardless of whether a transfer has had a chance to be set or not. This change may actually be helpful if, for instance a user sets an unrealistically small transfer timeout with regards to their system capabilities (because of system load, etc.), which results in the timeout expiring before libusb(x) gets a chance to speak to the bus. By arming the timerfd early, we may make it more explicit for such an user to realize that they have to take into account libusb(x)'s overhead... Thus, unless I'm misreading what you meant here, I don't currently see an issue with Hans' suggestion, and that's what I would vote for too. 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
