From: Andrew Deason Date: Mon, 20 Jul 2009 17:31:44 +0000 (-0500) Subject: Add additional vlprocs safety checks X-Git-Tag: openafs-devel-1_5_61~7 X-Git-Url: https://git.michaelhowe.org/gitweb/?a=commitdiff_plain;h=ef7e0d64e7c29675405299f36ee3b7343b31cb35;p=packages%2Fo%2Fopenafs.git 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 Reviewed-by: Derrick Brashear --- diff --git a/src/vlserver/vlprocs.c b/src/vlserver/vlprocs.c index 73f565837..c11efe7e7 100644 --- 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 a96223777..f6281d2c5 100644 --- 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 5373140b3..b9ac712bd 100644 --- 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 a999eb04a..298be88a2 100644 --- 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; }