From 70798bd662cd17ea9782bf970dab988bab2002d2 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Fri, 12 Feb 2010 17:44:31 -0600 Subject: [PATCH] Check for HOSTDELETED before h_Hold_r A few places h_Hold_r a host and later drop and reacquire H_LOCK without checking if the hostFlags contains HOSTDELETED. This can cause a race with h_TossStuff_r where we later reference a host that is about to be freed or already has been freed. Add checks for HOSTDELETED in these places, and skip over the deleted hosts. FIXES 126454 Reviewed-on: http://gerrit.openafs.org/1305 Reviewed-by: Derrick Brashear Tested-by: Derrick Brashear (cherry picked from commit b8c3c6add90ea3face9a16ff04a1024be3d8f32d) Change-Id: I9e67465b3dac6db8626fee03823ed63a680beaad Reviewed-on: http://gerrit.openafs.org/1366 Tested-by: Andrew Deason Reviewed-by: Derrick Brashear --- src/viced/callback.c | 37 +++++++++++++++++++-------- src/viced/host.c | 61 +++++++++++++++++++++++++++++++++++++++----- 2 files changed, 80 insertions(+), 18 deletions(-) diff --git a/src/viced/callback.c b/src/viced/callback.c index 4f351c0d17..4b5d04d31f 100644 --- a/src/viced/callback.c +++ b/src/viced/callback.c @@ -940,10 +940,12 @@ BreakCallBack(struct host *xhost, AFSFid * fid, int flag) ntohs(thishost->port))); cb->status = CB_DELAYED; } else { - h_Hold_r(thishost); - cba[ncbas].hp = thishost; - cba[ncbas].thead = cb->thead; - ncbas++; + if (!(thishost->hostFlags & HOSTDELETED)) { + h_Hold_r(thishost); + cba[ncbas].hp = thishost; + cba[ncbas].thead = cb->thead; + ncbas++; + } TDel(cb); HDel(cb); CDel(cb, 1); /* Usually first; so this delete @@ -1324,11 +1326,14 @@ BreakVolumeCallBacks(afs_uint32 volume) register struct CallBack *cbnext; for (cb = itocb(fe->firstcb); cb; cb = cbnext) { host = h_itoh(cb->hhead); - h_Hold_r(host); - cbnext = itocb(cb->cnext); - if (!tthead || (TNorm(tthead) < TNorm(cb->thead))) { - tthead = cb->thead; + + if (!(host->hostFlags & HOSTDELETED)) { + h_Hold_r(host); + if (!tthead || (TNorm(tthead) < TNorm(cb->thead))) { + tthead = cb->thead; + } } + cbnext = itocb(cb->cnext); TDel(cb); HDel(cb); FreeCB(cb); @@ -1475,9 +1480,11 @@ BreakLaterCallBacks(void) cbnext = itocb(cb->cnext); host = h_itoh(cb->hhead); if (cb->status == CB_DELAYED) { - h_Hold_r(host); - if (!tthead || (TNorm(tthead) < TNorm(cb->thead))) { - tthead = cb->thead; + if (!(host->hostFlags & HOSTDELETED)) { + h_Hold_r(host); + if (!tthead || (TNorm(tthead) < TNorm(cb->thead))) { + tthead = cb->thead; + } } TDel(cb); HDel(cb); @@ -1727,6 +1734,14 @@ ClearHostCallbacks_r(struct host *hp, int locked) ViceLog(5, ("GSS: Delete longest inactive host %s\n", afs_inet_ntoa_r(hp->host, hoststr))); + + if ((hp->hostFlags & HOSTDELETED)) { + /* hp could go away after reacquiring H_LOCK in h_NBLock_r, so we can't + * really use it; its callbacks will get cleared anyway when + * h_TossStuff_r gets its hands on it */ + return 1; + } + if (!(held = h_Held_r(hp))) h_Hold_r(hp); diff --git a/src/viced/host.c b/src/viced/host.c index efd7e5f696..77f66c9aff 100644 --- a/src/viced/host.c +++ b/src/viced/host.c @@ -1054,6 +1054,7 @@ h_Enumerate(int (*proc) (), char *param) register struct host *host, **list; register int *held; register int i, count; + int totalCount; H_LOCK; if (hostCount == 0) { @@ -1070,13 +1071,19 @@ h_Enumerate(int (*proc) (), char *param) ViceLog(0, ("Failed malloc in h_Enumerate\n")); assert(0); } - for (count = 0, host = hostList; host && count < hostCount; host = host->next, count++) { - list[count] = host; - if (!(held[count] = h_Held_r(host))) - h_Hold_r(host); + for (totalCount = count = 0, host = hostList; + host && totalCount < hostCount; + host = host->next, totalCount++) { + + if (!(host->hostFlags & HOSTDELETED)) { + list[count] = host; + if (!(held[count] = h_Held_r(host))) + h_Hold_r(host); + count++; + } } - if (count != hostCount) { - ViceLog(0, ("h_Enumerate found %d of %d hosts\n", count, hostCount)); + if (totalCount != hostCount) { + ViceLog(0, ("h_Enumerate found %d of %d hosts\n", totalCount, hostCount)); } else if (host != NULL) { ViceLog(0, ("h_Enumerate found more than %d hosts\n", hostCount)); ShutDownAndCore(PANIC); @@ -1117,14 +1124,52 @@ h_Enumerate_r(int (*proc) (), struct host *enumstart, char *param) if (hostCount == 0) { return; } + + host = enumstart; + enumstart = NULL; + + /* find the first non-deleted host, so we know where to actually start + * enumerating */ + for (count = 0; host && count < hostCount; count++) { + if (!(host->hostFlags & HOSTDELETED)) { + enumstart = host; + break; + } + host = host->next; + } + if (!enumstart) { + /* we didn't find a non-deleted host... */ + + if (host && count >= hostCount) { + /* ...because we found a loop */ + ViceLog(0, ("h_Enumerate_r found more than %d hosts\n", hostCount)); + ShutDownAndCore(PANIC); + } + + /* ...because the hostList is full of deleted hosts */ + return; + } + if (enumstart && !(held = h_Held_r(enumstart))) h_Hold_r(enumstart); + /* remember hostCount, lest it change over the potential H_LOCK drop in * h_Release_r */ origHostCount = hostCount; for (count = 0, host = enumstart; host && count < origHostCount; host = next, held = nheld, count++) { next = host->next; + + /* find the next non-deleted host */ + while (next && (next->hostFlags & HOSTDELETED)) { + next = next->next; + /* inc count for the skipped-over host */ + if (++count > origHostCount) { + ViceLog(0, ("h_Enumerate_r found more than %d hosts\n", origHostCount)); + ShutDownAndCore(PANIC); + } + } + if (next && !(nheld = h_Held_r(next))) h_Hold_r(next); held = (*proc) (host, held, param); @@ -2170,7 +2215,9 @@ h_FindClient_r(struct rx_connection *tcon) client = (struct client *)rx_GetSpecific(tcon, rxcon_client_key); if (client && client->sid == rxr_CidOf(tcon) - && client->VenusEpoch == rxr_GetEpoch(tcon)) { + && client->VenusEpoch == rxr_GetEpoch(tcon) + && !(client->host->hostFlags & HOSTDELETED)) { + client->refCount++; h_Hold_r(client->host); if (!client->deleted && client->prfail != 2) {