From: Jeffrey Altman <jaltman@your-file-system.com>
Date: Fri, 20 Jan 2012 06:50:01 +0000 (-0500)
Subject: Rx: rxi_FreeCall conn_call_lock vs call->lock deadlock
X-Git-Tag: upstream/1.8.0_pre1^2~2653
X-Git-Url: https://git.michaelhowe.org/gitweb/?a=commitdiff_plain;h=33185db16aca40a3bdbcb66caf994924220b5012;p=packages%2Fo%2Fopenafs.git

Rx: rxi_FreeCall conn_call_lock vs call->lock deadlock

The conn->conn_call_lock is held before call->lock in the lock
hierarchy which is violated within rxi_FreeCall(). While the
deadlock is rare, it is possible and has been experienced on
both Windows and Linux.

Change the signature of rxi_FreeCall to return 1 if it frees
the call and 0 if it does not.

Due to the lock hierarchy violation use MUTEX_TRYENTER()
to attempt to obtain the conn->conn_call_lock.  If the lock
cannot be obtained set the call state to dally and
return.  If the conn_call_lock can be obtained, behave as
we did before this patchset.

Only increment the callNumber if the original call->state
was dally or hold and the conn_call_lock could be obtained.
We must not increment the callNumber otherwise.  Doing so can
result in call numbers being skipped when the conn->call slot
is reused.

Change-Id: Ic10bd2004e9b06df319c2f2efaa0b37bcb90c896
Reviewed-on: http://gerrit.openafs.org/6443
Tested-by: Jeffrey Altman <jaltman@secure-endpoints.com>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Jeffrey Altman <jaltman@secure-endpoints.com>
---

diff --git a/src/rx/rx.c b/src/rx/rx.c
index 1eff5bb54..f4037aff7 100644
--- a/src/rx/rx.c
+++ b/src/rx/rx.c
@@ -2677,16 +2677,16 @@ rxi_NewCall(struct rx_connection *conn, int channel)
  *
  * call->lock amd rx_refcnt_mutex are held upon entry.
  * haveCTLock is set when called from rxi_ReapConnections.
+ *
+ * return 1 if the call is freed, 0 if not.
  */
-static void
+static int
 rxi_FreeCall(struct rx_call *call, int haveCTLock)
 {
     int channel = call->channel;
     struct rx_connection *conn = call->conn;
+    u_char state = call->state;
 
-
-    if (call->state == RX_STATE_DALLY || call->state == RX_STATE_HOLD)
-	(*call->callNumber)++;
     /*
      * We are setting the state to RX_STATE_RESET to
      * ensure that no one else will attempt to use this
@@ -2699,10 +2699,24 @@ rxi_FreeCall(struct rx_call *call, int haveCTLock)
     MUTEX_EXIT(&rx_refcnt_mutex);
     rxi_ResetCall(call, 0);
 
-    MUTEX_ENTER(&conn->conn_call_lock);
-    if (call->conn->call[channel] == call)
-        call->conn->call[channel] = 0;
-    MUTEX_EXIT(&conn->conn_call_lock);
+    if (MUTEX_TRYENTER(&conn->conn_call_lock))
+    {
+        if (state == RX_STATE_DALLY || state == RX_STATE_HOLD)
+            (*call->callNumber)++;
+
+        if (call->conn->call[channel] == call)
+            call->conn->call[channel] = 0;
+        MUTEX_EXIT(&conn->conn_call_lock);
+    } else {
+        /*
+         * We couldn't obtain the conn_call_lock so we can't
+         * disconnect the call from the connection.  Set the
+         * call state to dally so that the call can be reused.
+         */
+        MUTEX_ENTER(&rx_refcnt_mutex);
+        call->state = RX_STATE_DALLY;
+        return 0;
+    }
 
     MUTEX_ENTER(&rx_freeCallQueue_lock);
     SET_CALL_QUEUE_LOCK(call, &rx_freeCallQueue_lock);
@@ -2752,6 +2766,7 @@ rxi_FreeCall(struct rx_call *call, int haveCTLock)
 	MUTEX_EXIT(&conn->conn_data_lock);
     }
     MUTEX_ENTER(&rx_refcnt_mutex);
+    return 1;
 }
 
 rx_atomic_t rxi_Allocsize = RX_ATOMIC_INIT(0);
@@ -6227,10 +6242,12 @@ rxi_CheckCall(struct rx_call *call)
 	    rxevent_Cancel(&call->growMTUEvent, call,
 			   RX_CALL_REFCOUNT_ALIVE);
             MUTEX_ENTER(&rx_refcnt_mutex);
-	    if (call->refCount == 0) {
-		rxi_FreeCall(call, haveCTLock);
+            /* if rxi_FreeCall returns 1 it has freed the call */
+	    if (call->refCount == 0 &&
+                rxi_FreeCall(call, haveCTLock))
+            {
                 MUTEX_EXIT(&rx_refcnt_mutex);
-		return -2;
+                return -2;
 	    }
             MUTEX_EXIT(&rx_refcnt_mutex);
 	    return -1;