From: Jeffrey Altman <jaltman@your-file-system.com>
Date: Tue, 5 Mar 2013 12:52:37 +0000 (-0500)
Subject: Windows: Avoid race during cm_FreeServerList
X-Git-Tag: openafs-stable-1_8_0pre1~1330
X-Git-Url: https://git.openafs.org/?p=openafs.git;a=commitdiff_plain;h=1b048f1f571eb02976a78a4dabafb3c677fbf9d0

Windows: Avoid race during cm_FreeServerList

cm_FreeServerList obtains cm_serverLock exclusively and in some
circumstances will call cm_FreeServer().   cm_FreeServer() will
drop the cm_serverLock if the cm_server_t.refCount is zero in order to
avoid a lock order violation when calling cm_GCConnections() since
cm_connLock is higher in the lock hierarchy.

The call to cm_FreeServer is performed after the cm_serverRef_t
to be deleted is identified but before it is removed from the list.
There is the potential for two threads calling cm_FreeServerList()
to race and for more than one thread to attempt to delete the same
cm_serverRef_t twice.

Fix this by:

1. maintain a private copy of the cm_server_t pointer, delete the
cm_serverRef_t and update the list pointers before calling cm_FreeServer().

2. obtain and release a refcnt on the next cm_serverRef_t to ensure
that it is not deleted out from underneath the thread in case the
cm_serverLock is dropped.

Change-Id: Ia7b6eed66e9ba306c07d47027262e1a8ad1e52ac
Reviewed-on: http://gerrit.openafs.org/9391
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Jeffrey Altman <jaltman@your-file-system.com>
---

diff --git a/src/WINNT/afsd/cm_server.c b/src/WINNT/afsd/cm_server.c
index 1b0b6c8..5852a75 100644
--- a/src/WINNT/afsd/cm_server.c
+++ b/src/WINNT/afsd/cm_server.c
@@ -1209,20 +1209,29 @@ void cm_InsertServerList(cm_serverRef_t** list, cm_serverRef_t* element)
         cm_serverRef_t  **currentp = list;
         cm_serverRef_t  **nextp = NULL;
         cm_serverRef_t  * next = NULL;
+        cm_server_t     * serverp = NULL;
 
         for (currentp = list; *currentp; currentp = nextp)
         {
             nextp = &(*currentp)->next;
+            /* obtain a refcnt on next in case cm_serverLock is dropped */
+            if (*nextp)
+                cm_GetServerRef(*nextp, TRUE);
             if ((*currentp)->refCount == 0 &&
                 (*currentp)->status == srv_deleted) {
                 next = *nextp;
 
                 if ((*currentp)->volID)
                     cm_RemoveVolumeFromServer((*currentp)->server, (*currentp)->volID);
-                cm_FreeServer((*currentp)->server);
+                serverp = (*currentp)->server;
                 free(*currentp);
                 nextp = &next;
+                /* cm_FreeServer will drop cm_serverLock if serverp->refCount == 0 */
+                cm_FreeServer(serverp);
             }
+            /* drop the next refcnt obtained above. */
+            if (*nextp)
+                cm_PutServerRef(*nextp, TRUE);
         }
     }
 
@@ -1491,6 +1500,7 @@ void cm_FreeServerList(cm_serverRef_t** list, afs_uint32 flags)
     cm_serverRef_t  **current;
     cm_serverRef_t  **nextp;
     cm_serverRef_t  * next;
+    cm_server_t     * serverp;
     afs_int32         refCount;
 
     lock_ObtainWrite(&cm_serverLock);
@@ -1504,15 +1514,20 @@ void cm_FreeServerList(cm_serverRef_t** list, afs_uint32 flags)
     while (*current)
     {
         nextp = &(*current)->next;
+        /* obtain a refcnt on next in case cm_serverLock is dropped */
+        if (*nextp)
+            cm_GetServerRef(*nextp, TRUE);
         refCount = cm_PutServerRef(*current, TRUE);
         if (refCount == 0) {
             next = *nextp;
 
             if ((*current)->volID)
                 cm_RemoveVolumeFromServer((*current)->server, (*current)->volID);
-            cm_FreeServer((*current)->server);
+            serverp = (*current)->server;
             free(*current);
             *current = next;
+            /* cm_FreeServer will drop cm_serverLock if serverp->refCount == 0 */
+            cm_FreeServer(serverp);
         } else {
             if (flags & CM_FREESERVERLIST_DELETE) {
                 (*current)->status = srv_deleted;
@@ -1521,6 +1536,9 @@ void cm_FreeServerList(cm_serverRef_t** list, afs_uint32 flags)
             }
             current = nextp;
         }
+        /* drop the next refcnt obtained above. */
+        if (*current)
+            cm_PutServerRef(*current, TRUE);
     }
 
   done: