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>
afs_int32 roindex = 0;
afs_uint32 roserver = 0;
struct rx_connection *aconn;
+ int islocked = 0;
memset(&storeEntry, 0, sizeof(struct nvldbentry));
PrintError("", vcode);
return -1;
}
+ islocked = 1;
/* make sure the VLDB entry hasn't changed since we started */
memset(&checkEntry, 0, sizeof(checkEntry));
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;
}
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: