Add additional vlprocs safety checks

This adds additional safety checks to the vlserver's implementation of
the VL_CreateEntry, VL_ReplaceEntry, and VL_UpdateEntry RPCs. Now in all
three of these, any new volume ID that would be added to the VLDB or
that would be newly referenced in a VLDB entry is checked against
duplication in other entries. Additionally, any new volume names added
to the VLDB (either by creation, or modifying an existing volume) are
checked against duplication. This should make it impossible for clients
to make a volume ID or volume name correspond to multiple volume groups
(either conceptually or literally in the vldb).

This also alters the vlserver's implementation of the VL_GetNewVolumeId
RPC such that the vlserver increments maxvolid until the range of volume
IDs [*newvolumeid, *newvolumeid+bumpcount) is unused. 'vos' is modified
to only allocate one new volume id at a time, so we don't skip over
potentially-usable vol ids.

Reviewed-on: http://gerrit.openafs.org/158
Tested-by: Derrick Brashear <shadow@dementia.org>
Reviewed-by: Derrick Brashear <shadow@dementia.org>
(cherry picked from commit ef7e0d64e7c29675405299f36ee3b7343b31cb35)
Reviewed-on: http://gerrit.openafs.org/280

diff --git a/src/vlserver/vlprocs.c b/src/vlserver/vlprocs.c
index d855e273356d90a1c5b8e766e29ae12bc6d97f2a..211623cb863f11dc8a32c316a886e6ddb6df1634 100644 (file)
--- a/src/vlserver/vlprocs.c
+++ b/src/vlserver/vlprocs.c
@@ -178,9 +178,8 @@ SVL_CreateEntry(rxcall, newentry)
     VLog(1,
         ("OCreate Volume %d %s\n", newentry->volumeId[RWVOL],
          rxinfo(rxcall)));
-    /* XXX shouldn't we check to see if the r/o volume is duplicated? */
-    if (newentry->volumeId[RWVOL]
-       && FindByID(trans, newentry->volumeId[RWVOL], RWVOL, &tentry, &errorcode)) {    /* entry already exists, we fail */
+    if (EntryIDExists(trans, newentry->volumeId, MAXTYPES, &errorcode)) {
+       /* at least one of the specified IDs already exists; we fail */
        errorcode = VL_IDEXIST;
        goto abort;
     } else if (errorcode) {
@@ -253,9 +252,8 @@ SVL_CreateEntryN(rxcall, newentry)
     VLog(1,
         ("Create Volume %d %s\n", newentry->volumeId[RWVOL],
          rxinfo(rxcall)));
-    /* XXX shouldn't we check to see if the r/o volume is duplicated? */
-    if (newentry->volumeId[RWVOL]
-       && FindByID(trans, newentry->volumeId[RWVOL], RWVOL, &tentry, &errorcode)) {    /* entry already exists, we fail */
+    if (EntryIDExists(trans, newentry->volumeId, MAXTYPES, &errorcode)) {
+       /* at least one of the specified IDs already exists; we fail */
        errorcode = VL_IDEXIST;
        goto abort;
     } else if (errorcode) {
@@ -570,7 +568,12 @@ SVL_GetNewVolumeId(rxcall, Maxvolidbump, newvolumeid)
     if (errorcode = Init_VLdbase(&trans, LOCKWRITE, this_op))
        goto end;
 
-    *newvolumeid = maxvolumeid = ntohl(cheader.vital_header.MaxVolumeId);
+    *newvolumeid = maxvolumeid = NextUnusedID(trans,
+       ntohl(cheader.vital_header.MaxVolumeId), Maxvolidbump, &errorcode);
+    if (errorcode) {
+       goto abort;
+    }
+
     maxvolumeid += Maxvolidbump;
     VLog(1, ("GetNewVolid newmax=%d %s\n", maxvolumeid, rxinfo(rxcall)));
     cheader.vital_header.MaxVolumeId = htonl(maxvolumeid);
@@ -607,6 +610,7 @@ SVL_ReplaceEntry(rxcall, volid, voltype, newentry, releasetype)
     int hashnewname;
     int hashVol[MAXTYPES];
     struct nvlentry tentry;
+    afs_uint32 checkids[MAXTYPES];
 
     COUNT_REQ(VLREPLACEENTRY);
     for (typeindex = 0; typeindex < MAXTYPES; typeindex++)
@@ -640,6 +644,29 @@ SVL_ReplaceEntry(rxcall, volid, voltype, newentry, releasetype)
        ABORT(VL_BADENTRY);
     }
 
+    /* make sure none of the IDs we are changing to are already in use */
+    memset(&checkids, 0, sizeof(checkids));
+    for (typeindex = ROVOL; typeindex < MAXTYPES; typeindex++) {
+       if (tentry.volumeId[typeindex] != newentry->volumeId[typeindex]) {
+           checkids[typeindex] = newentry->volumeId[typeindex];
+       }
+    }
+    if (EntryIDExists(trans, checkids, MAXTYPES, &errorcode)) {
+       ABORT(VL_IDEXIST);
+    } else if (errorcode) {
+       goto abort;
+    }
+
+    /* make sure the name we're changing to doesn't already exist */
+    if (strcmp(newentry->name, tentry.name)) {
+       struct nvlentry tmp_entry;
+       if (FindByName(trans, newentry->name, &tmp_entry, &errorcode)) {
+           ABORT(VL_NAMEEXIST);
+       } else if (errorcode) {
+           goto abort;
+       }
+    }
+
     /* unhash volid entries if they're disappearing or changing.
      * Remember if we need to hash in the new value (we don't have to
      * rehash if volid stays same */
@@ -2684,10 +2711,41 @@ get_vldbupdateentry(trans, blockindex, updateentry, VlEntry)
      struct nvlentry *VlEntry;
 {
     int i, j, errorcode, serverindex;
+    struct vldbentry checkentry;
+    afs_uint32 checkids[MAXTYPES];
+
+    /* check if any specified new IDs are already present in the db. Do
+     * this check before doing anything else, so we don't get a half-
+     * updated entry. */
+    memset(&checkids, 0, sizeof(checkids));
+    if (updateentry->Mask & VLUPDATE_RWID) {
+       checkids[RWVOL] = updateentry->spares3; /* rw id */
+    }
+    if (updateentry->Mask & VLUPDATE_READONLYID) {
+       checkids[ROVOL] = updateentry->ReadOnlyId;
+    }
+    if (updateentry->Mask & VLUPDATE_BACKUPID) {
+       checkids[BACKVOL] = updateentry->BackupId;
+    }
+
+    if (EntryIDExists(trans, checkids, MAXTYPES, &errorcode)) {
+       return VL_IDEXIST;
+    } else if (errorcode) {
+       return errorcode;
+    }
 
     if (updateentry->Mask & VLUPDATE_VOLUMENAME) {
+       struct nvlentry tentry;
+
        if (InvalidVolname(updateentry->name))
            return VL_BADNAME;
+
+       if (FindByName(trans, updateentry->name, &tentry, &errorcode)) {
+           return VL_NAMEEXIST;
+       } else if (errorcode) {
+           return errorcode;
+       }
+
        if (errorcode = UnhashVolname(trans, blockindex, VlEntry))
            return errorcode;
        strncpy(VlEntry->name, updateentry->name, sizeof(VlEntry->name));

diff --git a/src/vlserver/vlutils.c b/src/vlserver/vlutils.c
index 034ca1287bbdba0a84c33f579076ef3e83b48a63..bf1d232868d69277987b47aa1110247fe9cbb3c5 100644 (file)
--- a/src/vlserver/vlutils.c
+++ b/src/vlserver/vlutils.c
@@ -678,6 +678,83 @@ FindByName(trans, volname, tentry, error)
     return 0;                  /* no such entry */
 }
 
+/**
+ * Returns whether or not any of the supplied volume IDs already exist
+ * in the vldb.
+ *
+ * @param trans    the ubik transaction
+ * @param ids      an array of volume IDs
+ * @param ids_len  the number of elements in the 'ids' array
+ * @param error    filled in with an error code in case of error
+ *
+ * @return whether any of the volume IDs are already used
+ *  @retval 1  at least one of the volume IDs is already used
+ *  @retval 0  none of the volume IDs are used, or an error occurred
+ */
+int
+EntryIDExists(struct ubik_trans *trans, const afs_uint32 *ids,
+             afs_int32 ids_len, afs_int32 *error)
+{
+    afs_int32 typeindex;
+    struct nvlentry tentry;
+
+    *error = 0;
+
+    for (typeindex = 0; typeindex < ids_len; typeindex++) {
+       if (ids[typeindex]
+           && FindByID(trans, ids[typeindex], -1, &tentry, error)) {
+
+           return 1;
+       } else if (*error) {
+           return 0;
+       }
+    }
+
+    return 0;
+}
+
+/**
+ * Finds the next range of unused volume IDs in the vldb.
+ *
+ * @param trans     the ubik transaction
+ * @param maxvolid  the current max vol ID, and where to start looking
+ *                  for an unused volume ID range
+ * @param bump      how many volume IDs we need to be unused
+ * @param error     filled in with an error code in case of error
+ *
+ * @return the next volume ID 'volid' such that the range
+ *         [volid, volid+bump) of volume IDs is unused, or 0 if there's
+ *         an error
+ */
+afs_uint32
+NextUnusedID(struct ubik_trans *trans, afs_uint32 maxvolid, afs_uint32 bump,
+            afs_int32 *error)
+{
+    struct nvlentry tentry;
+    afs_uint32 id;
+    afs_uint32 nfree;
+
+    *error = 0;
+
+     /* we simply start at the given maxvolid, keep a running tally of
+      * how many free volume IDs we've seen in a row, and return when
+      * we've seen 'bump' unused IDs in a row */
+    for (id = maxvolid, nfree = 0; nfree < bump; ++id) {
+       if (FindByID(trans, id, -1, &tentry, error)) {
+           nfree = 0;
+       } else if (*error) {
+           return 0;
+       } else {
+           ++nfree;
+       }
+    }
+
+    /* 'id' is now at the end of the [maxvolid,maxvolid+bump) range,
+     * but we need to return the first unused id, so subtract the
+     * number of current running free IDs to get the beginning */
+    return id - nfree;
+}
+
 int
 HashNDump(trans, hashindex)
      struct ubik_trans *trans;

diff --git a/src/volser/vsprocs.c b/src/volser/vsprocs.c
index 877b5a99e718df2ce5a0f6f8cbf45ca4984778b3..c2fe6c8cd5d72cc14fb6055202ffaf6ab88f4263 100644 (file)
--- a/src/volser/vsprocs.c
+++ b/src/volser/vsprocs.c
@@ -678,14 +678,6 @@ UV_CreateVolume3(afs_int32 aserver, afs_int32 apart, char *aname,
     afs_int32 lastid;
     struct nvldbentry entry, storeEntry;       /*the new vldb entry */
     struct volintInfo tstatus;
-    afs_int32 alloc_roid = 0, alloc_bkid = 0;
-
-    if (aroid && *aroid == 0) {
-       alloc_roid = 1;
-    }
-    if (abkid && *abkid == 0) {
-       alloc_bkid = 1;
-    }
 
     tid = 0;
     aconn = (struct rx_connection *)0;
@@ -696,6 +688,13 @@ UV_CreateVolume3(afs_int32 aserver, afs_int32 apart, char *aname,
 
     aconn = UV_Bind(aserver, AFSCONF_VOLUMEPORT);
 
+    if (aroid && *aroid) {
+       VPRINT1("Using RO volume ID %d.\n", *aroid);
+    }
+    if (abkid && *abkid) {
+       VPRINT1("Using BK volume ID %d.\n", *abkid);
+    }
+
     if (*anewid) {
         vcode = VLDB_GetEntryByID(*anewid, -1, &entry);
        if (!vcode) {
@@ -704,31 +703,27 @@ UV_CreateVolume3(afs_int32 aserver, afs_int32 apart, char *aname,
        }
        VPRINT1("Using volume ID %d.\n", *anewid);
     } else {
-       /* get the next 3 available ids from the VLDB */
-       int alloc_ids = 3;
+       vcode = ubik_VL_GetNewVolumeId(cstruct, 0, 1, anewid);
+       EGOTO1(cfail, vcode, "Could not get an Id for volume %s\n", aname);
 
-       /* if the RO or BK id are specified, we don't need to allocate as
-        * many IDs */
-       if (!alloc_roid) {
-           --alloc_ids;
+       if (aroid && *aroid == 0) {
+           vcode = ubik_VL_GetNewVolumeId(cstruct, 0, 1, aroid);
+           EGOTO1(cfail, vcode, "Could not get an RO Id for volume %s\n", aname);
        }
-       if (!alloc_bkid) {
-           --alloc_ids;
+
+       if (abkid && *abkid == 0) {
+           vcode = ubik_VL_GetNewVolumeId(cstruct, 0, 1, abkid);
+           EGOTO1(cfail, vcode, "Could not get a BK Id for volume %s\n", aname);
        }
-       vcode = ubik_VL_GetNewVolumeId(cstruct, 0, alloc_ids, anewid);
-       EGOTO1(cfail, vcode, "Could not get an Id for volume %s\n", aname);
     }
 
     /* rw,ro, bk id are related in the default case */
+    /* If caller specified RW id, but not RO/BK ids, have them be RW+1 and RW+2 */
     lastid = *anewid;
-    if (aroid && *aroid) {
-       VPRINT1("Using RO volume ID %d.\n", aroid);
-    } else if (aroid) {
+    if (aroid && *aroid == 0) {
        *aroid = ++lastid;
     }
-    if (abkid && *abkid) {
-       VPRINT1("Using backup volume ID %d.\n", abkid);
-    } else if (abkid) {
+    if (abkid && *abkid == 0) {
        *abkid = ++lastid;
     }