Check for HOSTDELETED before h_Hold_r
authorAndrew Deason <adeason@sinenomine.net>
Fri, 12 Feb 2010 23:44:31 +0000 (17:44 -0600)
committerDerrick Brashear <shadow@dementia.org>
Mon, 22 Feb 2010 18:35:24 +0000 (10:35 -0800)
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

Change-Id: I5a61831f5afdbc908b82e4cf63cf14a34a36e275
Reviewed-on: http://gerrit.openafs.org/1305
Reviewed-by: Derrick Brashear <shadow@dementia.org>
Tested-by: Derrick Brashear <shadow@dementia.org>

src/viced/callback.c
src/viced/host.c

index cbe90d7..117a48e 100644 (file)
@@ -849,10 +849,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 
@@ -1232,11 +1234,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);
@@ -1383,9 +1388,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);
@@ -1624,6 +1631,13 @@ ClearHostCallbacks_r(struct host *hp, int locked)
            ("GSS: Delete longest inactive host %p (%s:%d)\n",
              hp, afs_inet_ntoa_r(hp->host, hoststr), ntohs(hp->port)));
 
+    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;
+    }
+
     h_Hold_r(hp);
 
     /** Try a non-blocking lock. If the lock is already held return
index 6e74768..2acce8c 100644 (file)
@@ -972,6 +972,7 @@ h_Enumerate(int (*proc) (struct host*, int, void *), void *param)
     register struct host *host, **list;
     register int *flags;
     register int i, count;
+    int totalCount;
 
     H_LOCK;
     if (hostCount == 0) {
@@ -988,12 +989,18 @@ h_Enumerate(int (*proc) (struct host*, int, void *), void *param)
        ViceLog(0, ("Failed malloc in h_Enumerate (flags)\n"));
        assert(0);
     }
-    for (count = 0, host = hostList; host && count < hostCount; host = host->next, count++) {
-       list[count] = 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;
+           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);
@@ -1042,6 +1049,31 @@ h_Enumerate_r(int (*proc) (struct host *, int, void *),
        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;
+    }
+
     h_Hold_r(enumstart);
 
     /* remember hostCount, lest it change over the potential H_LOCK drop in
@@ -1050,12 +1082,25 @@ h_Enumerate_r(int (*proc) (struct host *, int, void *),
 
     for (count = 0, host = enumstart; host && count < origHostCount; host = next, flags = nflags, 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 && !H_ENUMERATE_ISSET_BAIL(flags))
            h_Hold_r(next);
-       flags = (*proc) (host, flags, param);
-       if (H_ENUMERATE_ISSET_BAIL(flags)) {
-           h_Release_r(host); /* this might free up the host */
-           break;
+
+       if (!(host->hostFlags & HOSTDELETED)) {
+           flags = (*proc) (host, flags, param);
+           if (H_ENUMERATE_ISSET_BAIL(flags)) {
+               h_Release_r(host); /* this might free up the host */
+               break;
+           }
        }
        h_Release_r(host); /* this might free up the host */
     }
@@ -2185,7 +2230,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) {