Skip to content

Commit a05c6d9

Browse files
samhansenFuchsia Internal LUCI
authored andcommitted
[usb][ums] UMS integration test race condition
There currently exists a race in the ums-function (backing VirtualBus) driver. The worker thread assumes DdkUnbind() will always be invoked while it's awaiting a cnd_t condvar. The unbind hook sets active=False, and under the assumption it will unblock the waiting worker thread, calls cnd_signal(). Normally this wakes up the thread, which makes an active=True check and bails, exiting the thread. In the very unlikely event that the unbind hook is called while the worker thread is servicing a prior request's completion callback (a very fast operation), the cnd_signal() will go unnoticed (because there's no thread to actually wake up), the thread's while(1) loop will whip back around and block on cnd_wait(). However, in this state, there's nothing left to actually signal the cnd_t and the subsequent call to thrd_join() will hang as the thread is stuck in cnd_wait(). This adds the active state to the worker thread's loop check causing the thread to bail on whipback if the unbind hook is called while the worker thread happens to be doing actual work. Bug: 98566 Bug: 83563 Change-Id: If5a67b03087ab2135595612742506a495be0a574 Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/679993 Reviewed-by: Ruby Zhuang <[email protected]> Commit-Queue: Sam Hansen <[email protected]>
1 parent 2c709c4 commit a05c6d9

File tree

2 files changed

+3
-2
lines changed

2 files changed

+3
-2
lines changed

src/devices/block/drivers/ums-function/ums-function.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -624,7 +624,8 @@ static zx_handle_t vmo = 0;
624624

625625
static int usb_ums_thread(void* ctx) {
626626
usb_ums_t* ums = (usb_ums_t*)ctx;
627-
while (1) {
627+
ZX_ASSERT(ums->active);
628+
while (ums->active) {
628629
mtx_lock(&ums->mtx);
629630
if (!(ums->cbw_req_complete || ums->csw_req_complete || ums->data_req_complete ||
630631
(!ums->active))) {

src/devices/block/drivers/usb-mass-storage/tests/ums-test.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,7 @@ TEST_F(UmsTest, DISABLED_UncachedWriteShouldBePersistedToBlockDevice) {
317317
}
318318

319319
// TODO(fxbug.dev/98566) re-enable when the race is resolved.
320-
TEST_F(UmsTest, DISABLED_BlkdevTest) {
320+
TEST_F(UmsTest, BlkdevTest) {
321321
char errmsg[1024];
322322
fdio_spawn_action_t actions[1];
323323
actions[0] = {};

0 commit comments

Comments
 (0)