From: Jeffrey Altman <jaltman@your-file-system.com>
Date: Mon, 2 Jul 2012 02:19:08 +0000 (-0400)
Subject: Windows: cm_UpdateVolumeLocation misplaced lock
X-Git-Tag: upstream/1.6.2_pre2^2~45
X-Git-Url: https://git.michaelhowe.org/gitweb/?a=commitdiff_plain;h=75af57b38fd803f74319271dd6230a00e8ce0da4;p=packages%2Fo%2Fopenafs.git

Windows: cm_UpdateVolumeLocation misplaced lock

The volume->mx was obtained in the wrong place which resulted
in the potential of a panic caused by obtaining the mutex when
it was already held.

Reviewed-on: http://gerrit.openafs.org/7654
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Jeffrey Altman <jaltman@your-file-system.com>
Tested-by: Jeffrey Altman <jaltman@your-file-system.com>
(cherry picked from commit 4707174938b92df189697a2b7e463438c37c1ed0)

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

diff --git a/src/WINNT/afsd/cm_volume.c b/src/WINNT/afsd/cm_volume.c
index 3eb8f72b1..62b30284e 100644
--- a/src/WINNT/afsd/cm_volume.c
+++ b/src/WINNT/afsd/cm_volume.c
@@ -307,6 +307,8 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t *
         }
 
         _InterlockedOr(&volp->flags, CM_VOLUMEFLAG_UPDATING_VL);
+
+        /* Do not hold the volume lock across the RPC calls */
         lock_ReleaseWrite(&volp->rw);
 
         if (cellp->flags & CM_CELLFLAG_VLSERVER_INVALID)
@@ -316,45 +318,45 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t *
         code = cm_GetEntryByName(cellp, volp->namep, &vldbEntry, &nvldbEntry,
                                  &uvldbEntry,
                                  &method, userp, reqp);
-    }
-
-    /* We can end up here with code == CM_ERROR_NOSUCHVOLUME if the base volume name
-     * does not exist and is not a numeric string but there might exist a .readonly volume.
-     * If the base name doesn't exist we will not care about the .backup that might be left
-     * behind since there should be no method to access it.
-     */
-    if (code == CM_ERROR_NOSUCHVOLUME &&
-        _atoi64(volp->namep) == 0 &&
-        volp->vol[RWVOL].ID == 0 &&
-        strlen(volp->namep) < (VL_MAXNAMELEN - 9)) {
-        char name[VL_MAXNAMELEN];
-
-        snprintf(name, VL_MAXNAMELEN, "%s.readonly", volp->namep);
 
-        /* now we have volume structure locked and held; make RPC to fill it */
-        code = cm_GetEntryByName(cellp, name, &vldbEntry, &nvldbEntry,
-                                 &uvldbEntry,
-                                 &method, userp, reqp);
-    }
+        /* We can end up here with code == CM_ERROR_NOSUCHVOLUME if the base volume name
+         * does not exist and is not a numeric string but there might exist a .readonly volume.
+         * If the base name doesn't exist we will not care about the .backup that might be left
+         * behind since there should be no method to access it.
+         */
+        if (code == CM_ERROR_NOSUCHVOLUME &&
+             _atoi64(volp->namep) == 0 &&
+             volp->vol[RWVOL].ID == 0 &&
+             strlen(volp->namep) < (VL_MAXNAMELEN - 9)) {
+            char name[VL_MAXNAMELEN];
+
+            snprintf(name, VL_MAXNAMELEN, "%s.readonly", volp->namep);
+
+            /* now we have volume structure locked and held; make RPC to fill it */
+            code = cm_GetEntryByName(cellp, name, &vldbEntry, &nvldbEntry,
+                                     &uvldbEntry,
+                                     &method, userp, reqp);
+        }
 
-    /*
-     * What if there was a volume rename?  The volume name no longer exists but the
-     * volume id might.  Try to refresh the volume location information based one
-     * of the readwrite or readonly volume id.
-     */
-    if (code == CM_ERROR_NOSUCHVOLUME) {
-        if (volp->vol[RWVOL].ID != 0) {
-            code = cm_GetEntryByID(cellp, volp->vol[RWVOL].ID, &vldbEntry, &nvldbEntry,
-                                    &uvldbEntry,
-                                    &method, userp, reqp);
-        } else if (volp->vol[ROVOL].ID != 0) {
-            code = cm_GetEntryByID(cellp, volp->vol[ROVOL].ID, &vldbEntry, &nvldbEntry,
-                                    &uvldbEntry,
-                                    &method, userp, reqp);
+        /*
+         * What if there was a volume rename?  The volume name no longer exists but the
+         * volume id might.  Try to refresh the volume location information based one
+         * of the readwrite or readonly volume id.
+         */
+        if (code == CM_ERROR_NOSUCHVOLUME) {
+            if (volp->vol[RWVOL].ID != 0) {
+                code = cm_GetEntryByID(cellp, volp->vol[RWVOL].ID, &vldbEntry, &nvldbEntry,
+                                       &uvldbEntry,
+                                       &method, userp, reqp);
+            } else if (volp->vol[ROVOL].ID != 0) {
+                code = cm_GetEntryByID(cellp, volp->vol[ROVOL].ID, &vldbEntry, &nvldbEntry,
+                                       &uvldbEntry,
+                                       &method, userp, reqp);
+            }
         }
+        lock_ObtainWrite(&volp->rw);
     }
 
-    lock_ObtainWrite(&volp->rw);
     if (code == 0) {
         afs_int32 flags;
         afs_int32 nServers;