vos: avoid double release of a volume lock 26/14426/2
authorMark Vitale <mvitale@sinenomine.net>
Thu, 5 Nov 2020 23:16:51 +0000 (18:16 -0500)
committerBenjamin Kaduk <kaduk@mit.edu>
Fri, 13 Nov 2020 15:54:46 +0000 (10:54 -0500)
To update a volume entry in the VLDB, vos commands typically lock the
volume entry via VL_SetLock, then call VL_UpdateEntryN, then release the
lock via VL_ReleaseLock.  However, some vos commands exploit the
optional lock release flags of VL_UpdateEntryN to combine the update and
unlock operations into a single RPC.  This approach requires extra care
to ensure that VL_ReleaseLock is issued for a failed VL_UpdateEntryN,
but NOT for a successful VL_UpdateEntryN.

Unfortunately, the following commands have success paths that fall
through to the error path, resulting in a double release of the volume
lock:
 - vos convertROtoRW
 - vos release

A second VL_ReleaseLock of a volume entry that has already been unlocked
via VL_UpdateEntryN is essentially a harmless no-op (other than negating
any benefit of exploiting the VL_UpdateEntryN lock flags).  However, if
there is a race with another volume operation on the same volume, it is
possible for this bug to release the volume lock of a different volume
operation.

This problem has been present in 'vos release' since OpenAFS 1.0.  This
problem has been present in 'vos convertROtoRW' since the command's
introduction in commit 8af8241e94284522feb77d75aee8ea3deb73f3cc
vol-ro-to-rw-tool-20030314.

Properly maintain state to avoid unlocking a volume (with
VL_ReleaseLock) that has already been unlocked (via VL_UpdateEntryN).

Thanks to Andrew Deason for discovering the issue and suggesting the
fix.

Change-Id: I757b4619b9431d1ca980f755349806993add14a5
Reviewed-on: https://gerrit.openafs.org/14426
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>

src/volser/vsprocs.c

index a2c00ca..cc5d2bd 100644 (file)
@@ -1278,6 +1278,7 @@ UV_ConvertRO(afs_uint32 server, afs_uint32 partition, afs_uint32 volid,
     afs_int32 roindex = 0;
     afs_uint32 roserver = 0;
     struct rx_connection *aconn;
+    int islocked = 0;
 
     memset(&storeEntry, 0, sizeof(struct nvldbentry));
 
@@ -1291,6 +1292,7 @@ UV_ConvertRO(afs_uint32 server, afs_uint32 partition, afs_uint32 volid,
        PrintError("", vcode);
        return -1;
     }
+    islocked = 1;
 
     /* make sure the VLDB entry hasn't changed since we started */
     memset(&checkEntry, 0, sizeof(checkEntry));
@@ -1402,15 +1404,19 @@ UV_ConvertRO(afs_uint32 server, afs_uint32 partition, afs_uint32 volid,
        fprintf(STDERR,
                "Warning: volume converted, but vldb update failed with code %d!\n",
                code);
+       goto error_exit;
     }
+    islocked = 0;   /* unlocked by the successful VLDB_ReplaceEntry above */
 
   error_exit:
-    vcode = UV_LockRelease(entry->volumeId[RWVOL]);
-    if (vcode) {
-       fprintf(STDERR,
-               "Unable to unlock volume %lu, code %d\n",
-               (unsigned long)entry->volumeId[RWVOL],vcode);
-       PrintError("", vcode);
+    if (islocked) {
+       vcode = UV_LockRelease(entry->volumeId[RWVOL]);
+       if (vcode) {
+           fprintf(STDERR,
+                   "Unable to unlock volume %lu, code %d\n",
+                   (unsigned long)entry->volumeId[RWVOL],vcode);
+           PrintError("", vcode);
+       }
     }
     return code;
 }
@@ -4134,6 +4140,7 @@ UV_ReleaseVolume(afs_uint32 afromvol, afs_uint32 afromserver,
        VLDB_ReplaceEntry(afromvol, RWVOL, &storeEntry,
                          LOCKREL_OPCODE | LOCKREL_AFSID | LOCKREL_TIMESTAMP);
     ONERROR(vcode, afromvol, " Could not update VLDB entry for volume %u\n");
+    islocked = 0;      /* lock released by successful VLDB_ReplaceEntry above */
     VDONE;
 
   rfail: