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>

diff --git a/src/vlserver/vlprocs.c b/src/vlserver/vlprocs.c
index 73f565837c2bbc6e085967cf5c3910ec7706b74f..c11efe7e73ed2e6718a578082f9f2e8439cb987f 100644 (file)
--- a/src/vlserver/vlprocs.c
+++ b/src/vlserver/vlprocs.c
@@ -198,9 +198,8 @@ SVL_CreateEntry(struct rx_call *rxcall, struct vldbentry *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) {
@@ -271,9 +270,8 @@ SVL_CreateEntryN(struct rx_call *rxcall, struct nvldbentry *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) {
@@ -568,7 +566,7 @@ afs_int32
 SVL_GetNewVolumeId(struct rx_call *rxcall, afs_uint32 Maxvolidbump,
                   afs_uint32 *newvolumeid)
 {
-    register afs_int32 errorcode;
+    afs_int32 errorcode;
     afs_uint32 maxvolumeid;
     struct ubik_trans *trans;
 
@@ -582,7 +580,12 @@ SVL_GetNewVolumeId(struct rx_call *rxcall, afs_uint32 Maxvolidbump,
     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=%u %s\n", maxvolumeid, rxinfo(rxcall)));
     cheader.vital_header.MaxVolumeId = htonl(maxvolumeid);
@@ -615,6 +618,7 @@ SVL_ReplaceEntry(struct rx_call *rxcall, afs_uint32 volid, afs_int32 voltype,
     int hashnewname;
     int hashVol[MAXTYPES];
     struct nvlentry tentry;
+    afs_uint32 checkids[MAXTYPES];
 
     COUNT_REQ(VLREPLACEENTRY);
     for (typeindex = 0; typeindex < MAXTYPES; typeindex++)
@@ -648,6 +652,29 @@ SVL_ReplaceEntry(struct rx_call *rxcall, afs_uint32 volid, afs_int32 voltype,
        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 */
@@ -2694,10 +2721,41 @@ get_vldbupdateentry(struct ubik_trans *trans,
                    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/vlserver_internal.h b/src/vlserver/vlserver_internal.h
index a96223777f27ad8a13d59c684f6970f9d7b3202a..f6281d2c580059516ffda702372751ebb97eaf20 100644 (file)
--- a/src/vlserver/vlserver_internal.h
+++ b/src/vlserver/vlserver_internal.h
@@ -31,6 +31,10 @@ extern afs_int32 FindByID(struct ubik_trans *trans, afs_uint32 volid,
                          afs_int32 *error);
 extern afs_int32 FindByName(struct ubik_trans *trans, char *volname,
                            struct nvlentry *tentry, afs_int32 *error);
+extern int EntryIDExists(struct ubik_trans *trans, const afs_uint32 *ids,
+                        afs_int32 ids_len, afs_int32 *error);
+extern afs_uint32 NextUnusedID(struct ubik_trans *trans, afs_uint32 maxvolid,
+                              afs_uint32 bump, afs_int32 *error);
 extern int HashNDump(struct ubik_trans *trans, int hashindex);
 extern int HashIdDump(struct ubik_trans *trans, int hashindex);
 extern int ThreadVLentry(struct ubik_trans *trans, afs_int32 blockindex,

diff --git a/src/vlserver/vlutils.c b/src/vlserver/vlutils.c
index 5373140b3367f638586615adef61f354a5fbad24..b9ac712bd5c7e8f9b275c82df38abf773c5d738a 100644 (file)
--- a/src/vlserver/vlutils.c
+++ b/src/vlserver/vlutils.c
@@ -655,6 +655,83 @@ FindByName(struct ubik_trans *trans, char *volname, struct nvlentry *tentry,
     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(struct ubik_trans *trans, int hashindex)
 {

diff --git a/src/volser/vsprocs.c b/src/volser/vsprocs.c
index a999eb04af5b39d13b05d168c908dc3724c76fb3..298be88a2dba2b6b7e4cbf03759a4e936c7d9cb4 100644 (file)
--- a/src/volser/vsprocs.c
+++ b/src/volser/vsprocs.c
@@ -731,14 +731,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;
@@ -749,6 +741,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) {
@@ -757,31 +756,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;
     }