From 0b5686001dff731ad0813213805eb15ede581a06 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Mon, 20 Jul 2009 12:31:44 -0500 Subject: [PATCH] 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 (cherry picked from commit ef7e0d64e7c29675405299f36ee3b7343b31cb35) Reviewed-on: http://gerrit.openafs.org/280 --- src/vlserver/vlprocs.c | 72 +++++++++++++++++++++++++++++++++++---- src/vlserver/vlutils.c | 77 ++++++++++++++++++++++++++++++++++++++++++ src/volser/vsprocs.c | 43 +++++++++++------------ 3 files changed, 161 insertions(+), 31 deletions(-) diff --git a/src/vlserver/vlprocs.c b/src/vlserver/vlprocs.c index d855e2733..211623cb8 100644 --- 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 034ca1287..bf1d23286 100644 --- 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 877b5a99e..c2fe6c8cd 100644 --- 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; } -- 2.39.5