Add additional vlprocs safety checks
authorAndrew Deason <adeason@sinenomine.net>
Mon, 20 Jul 2009 17:31:44 +0000 (12:31 -0500)
committerDerrick Brashear <shadow@dementia.org>
Thu, 30 Jul 2009 03:55:25 +0000 (20:55 -0700)
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>

src/vlserver/vlprocs.c
src/vlserver/vlserver_internal.h
src/vlserver/vlutils.c
src/volser/vsprocs.c

index 73f5658..c11efe7 100644 (file)
@@ -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));
index a962237..f6281d2 100644 (file)
@@ -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,
index 5373140..b9ac712 100644 (file)
@@ -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)
 {
index a999eb0..298be88 100644 (file)
@@ -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;
     }