From 660720d1f54a867e21f78b6ec4c024235e4c37b7 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 29 Mar 2012 10:30:47 -0500 Subject: [PATCH] rx: dec rx_nWaiting on clearing RX_CALL_WAIT_PROC Currently, a couple of callers (rxi_ResetCall, and rxi_AttachServerProc) will decrement rx_nWaiting only if RX_CALL_WAIT_PROC is set for a call, and the call is on a queue (presumably rx_incomingCallQueue). This can cause an imbalance in rx_nWaiting if these code paths are reached when, in another thread, rx_GetCall has removed the call from its queue, but it has not yet cleared RX_CALL_WAIT_PROC (this can happen while it is waiting for call->lock). In this situation, rx_GetCall will remove the call from its queue, wait, and e.g. rxi_ResetCall will clear RX_CALL_WAIT_PROC; neither will decrement rx_nWaiting. This is possible if a new call is started on a call channel with an extant call that is waiting for a thread; we will rxi_ResetCall in rxi_ReceivePacket, but rx_GetCall may be running at the same time. This race may also be possible via rxi_AttachServerProc via rxi_UpdatePeerReach -> TryAttach -> rxi_AttachServerProc while rx_GetCall is running, but I'm not sure. To avoid this, decrement rx_nWaiting based on RX_CALL_WAIT_PROC alone, regardless of whether or not the call is on a queue. This mirrors the incrementing rx_nWaiting behavior, where rx_nWaiting is only incremented if RX_CALL_WAIT_PROC is unset for a call, so this should guarantee that rx_nWaiting does not become unbalanced. Change-Id: I7dba4ba5f7cc33270c2d0f486b850fc0391927d1 Reviewed-on: http://gerrit.openafs.org/6986 Reviewed-by: Alistair Ferguson Reviewed-by: Derrick Brashear Reviewed-by: Jeffrey Altman Tested-by: BuildBot --- src/rx/rx.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/rx/rx.c b/src/rx/rx.c index 77de74d..1bf61e6 100644 --- a/src/rx/rx.c +++ b/src/rx/rx.c @@ -4832,10 +4832,9 @@ rxi_AttachServerProc(struct rx_call *call, if (call->flags & RX_CALL_WAIT_PROC) { /* Conservative: I don't think this should happen */ call->flags &= ~RX_CALL_WAIT_PROC; + rx_atomic_dec(&rx_nWaiting); if (queue_IsOnQueue(call)) { queue_Remove(call); - - rx_atomic_dec(&rx_nWaiting); } } call->state = RX_STATE_ACTIVE; @@ -5311,6 +5310,9 @@ rxi_ResetCall(struct rx_call *call, int newcall) osi_rxWakeup(&call->twind); #endif + if (flags & RX_CALL_WAIT_PROC) { + rx_atomic_dec(&rx_nWaiting); + } #ifdef RX_ENABLE_LOCKS /* The following ensures that we don't mess with any queue while some * other thread might also be doing so. The call_queue_lock field is @@ -5325,9 +5327,6 @@ rxi_ResetCall(struct rx_call *call, int newcall) MUTEX_ENTER(call->call_queue_lock); if (queue_IsOnQueue(call)) { queue_Remove(call); - if (flags & RX_CALL_WAIT_PROC) { - rx_atomic_dec(&rx_nWaiting); - } } MUTEX_EXIT(call->call_queue_lock); CLEAR_CALL_QUEUE_LOCK(call); @@ -5335,8 +5334,6 @@ rxi_ResetCall(struct rx_call *call, int newcall) #else /* RX_ENABLE_LOCKS */ if (queue_IsOnQueue(call)) { queue_Remove(call); - if (flags & RX_CALL_WAIT_PROC) - rx_atomic_dec(&rx_nWaiting); } #endif /* RX_ENABLE_LOCKS */ -- 1.9.4