libafs: Implement unixuser RW locks

Currently code dealing with changing unixuser structs does not obtain
any locks protecting the contents of the unixuser struct, though some
functions like afs_GetUser have a parameter indicating what type of
lock should be obtained. This can result in the token data for a user
being changed at the same time another thread tries to use the token
data.

To ensure mutual exclusion of such operations, add a lock field to the
unixuser struct, and actually lock it according to the intentions of
the relevant code.

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

diff --git a/src/afs/DOC/afs_rwlocks b/src/afs/DOC/afs_rwlocks
index ce28ce1..ff2ee6f 100644 (file)
--- a/src/afs/DOC/afs_rwlocks
+++ b/src/afs/DOC/afs_rwlocks
@@ -31,6 +31,10 @@ entries can be locked while holding afs_xdcache.
 Bugs: afs_xvcache locked before afs_xdcache in afs_remove, afs_symlink,
 etc in the file afs_vnodeops.c
 
+5.1.  unixusers. unixuser structs are locked before afs_xvcache in PSetTokens
+via afs_NotifyUser and via afs_ResetUserConns. They are also locked before
+afs_xvcache in afs_Analyze via afs_BlackListOnce.
+
 6.  afs_xvcache.  Must be able to load new cache entries while holding
 locks on others.  Note this means you can't lock a cache entry while
 holding either of this lock, unless, as in afs_create, the cache entry

diff --git a/src/afs/LINUX/osi_proc.c b/src/afs/LINUX/osi_proc.c
index 140a990..a5e0564 100644 (file)
--- a/src/afs/LINUX/osi_proc.c
+++ b/src/afs/LINUX/osi_proc.c
@@ -187,6 +187,11 @@ static int uu_show(struct seq_file *m, void *p)
        return 0;
     }
 
+    tu->refCount++;
+    ReleaseReadLock(&afs_xuser);
+
+    afs_LockUser(tu, READ_LOCK, 0);
+
     if (tu->cell == -1) {
        cellname = "<default>";
     } else {
@@ -234,6 +239,9 @@ static int uu_show(struct seq_file *m, void *p)
     }
     seq_printf(m, "\n");
 
+    afs_PutUser(tu, READ_LOCK);
+    ObtainReadLock(&afs_xuser);
+
     return 0;
 }
 

diff --git a/src/afs/LINUX24/osi_proc.c b/src/afs/LINUX24/osi_proc.c
index 4e485ff..9e4e12d 100644 (file)
--- a/src/afs/LINUX24/osi_proc.c
+++ b/src/afs/LINUX24/osi_proc.c
@@ -183,6 +183,11 @@ static int uu_show(struct seq_file *m, void *p)
        return 0;
     }
 
+    tu->refCount++;
+    ReleaseReadLock(&afs_xuser);
+
+    afs_LockUser(tu, READ_LOCK, 0);
+
     if (tu->cell == -1) {
        cellname = "<default>";
     } else {
@@ -226,6 +231,9 @@ static int uu_show(struct seq_file *m, void *p)
     }
     seq_printf(m, "\n");
 
+    afs_PutUser(tu, READ_LOCK);
+    ObtainReadLock(&afs_xuser);
+
     return 0;
 }
 

diff --git a/src/afs/VNOPS/afs_vnop_lookup.c b/src/afs/VNOPS/afs_vnop_lookup.c
index a36c70f..1edeb2c 100644 (file)
--- a/src/afs/VNOPS/afs_vnop_lookup.c
+++ b/src/afs/VNOPS/afs_vnop_lookup.c
@@ -528,19 +528,19 @@ afs_getsysname(struct vrequest *areq, struct vcache *adp,
     if (!afs_nfsexporter)
        strcpy(bufp, (*sysnamelist)[0]);
     else {
-       au = afs_GetUser(areq->uid, adp->f.fid.Cell, 0);
+       au = afs_GetUser(areq->uid, adp->f.fid.Cell, READ_LOCK);
        if (au->exporter) {
            error = EXP_SYSNAME(au->exporter, (char *)0, sysnamelist, num, 0);
            if (error) {
                strcpy(bufp, "@sys");
-               afs_PutUser(au, 0);
+               afs_PutUser(au, READ_LOCK);
                return -1;
            } else {
                strcpy(bufp, (*sysnamelist)[0]);
            }
        } else
            strcpy(bufp, afs_sysname);
-       afs_PutUser(au, 0);
+       afs_PutUser(au, READ_LOCK);
     }
     return 0;
 }
@@ -604,16 +604,16 @@ Next_AtSys(struct vcache *avc, struct vrequest *areq,
        *sysnamelist = afs_sysnamelist;
 
        if (afs_nfsexporter) {
-           au = afs_GetUser(areq->uid, avc->f.fid.Cell, 0);
+           au = afs_GetUser(areq->uid, avc->f.fid.Cell, READ_LOCK);
            if (au->exporter) {
                error =
                    EXP_SYSNAME(au->exporter, (char *)0, sysnamelist, &num, 0);
                if (error) {
-                   afs_PutUser(au, 0);
+                   afs_PutUser(au, READ_LOCK);
                    return 0;
                }
            }
-           afs_PutUser(au, 0);
+           afs_PutUser(au, READ_LOCK);
        }
        if (++(state->index) >= num || !(*sysnamelist)[(unsigned int)state->index])
            return 0;           /* end of list */

diff --git a/src/afs/afs.h b/src/afs/afs.h
index ac06998..f1dabf0 100644 (file)
--- a/src/afs/afs.h
+++ b/src/afs/afs.h
@@ -375,6 +375,7 @@ struct unixuser {
     struct tokenJar *tokens;
     struct afs_exporter *exporter;     /* more info about the exporter for the remote user */
     void *cellinfo;             /* pointer to cell info (PAG manager only) */
+    afs_rwlock_t lock;
 };
 
 #define CVEC_LEN 3 /* per-user connection pool */

diff --git a/src/afs/afs_nfsclnt.c b/src/afs/afs_nfsclnt.c
index cc72f7b..fbb5006 100644 (file)
--- a/src/afs/afs_nfsclnt.c
+++ b/src/afs/afs_nfsclnt.c
@@ -285,7 +285,9 @@ afs_nfsclient_reqhandler(struct afs_exporter *exporter,
     }
     if (au)
        afs_PutUser(au, READ_LOCK);
-    au = afs_GetUser(pag, -1, WRITE_LOCK);
+    /* do not get a lock on au; afs_nfsclient_getcreds may write-lock the
+     * same unixuser */
+    au = afs_GetUser(pag, -1, 0);
     if (!(au->exporter)) {     /* Created new unixuser struct */
        np->refCount++;         /* so it won't disappear */
        au->exporter = (struct afs_exporter *)np;
@@ -296,7 +298,7 @@ afs_nfsclient_reqhandler(struct afs_exporter *exporter,
     }
     *pagparam = pag;
     *outexporter = (struct afs_exporter *)np;
-    afs_PutUser(au, WRITE_LOCK);
+    afs_PutUser(au, 0);
 /*    ReleaseWriteLock(&afs_xnfsreq);  */
     return 0;
 }

diff --git a/src/afs/afs_pag_cred.c b/src/afs/afs_pag_cred.c
index bbe0425..d6ba2bb 100644 (file)
--- a/src/afs/afs_pag_cred.c
+++ b/src/afs/afs_pag_cred.c
@@ -108,6 +108,11 @@ afspag_PUnlog(char *ain, afs_int32 ainSize, afs_ucred_t **acred)
     ObtainWriteLock(&afs_xuser, 823);
     for (tu = afs_users[i]; tu; tu = tu->next) {
        if (tu->uid == uid) {
+           tu->refCount++;
+           ReleaseWriteLock(&afs_xuser);
+
+           afs_LockUser(tu, WRITE_LOCK, 368);
+
            tu->states &= ~UHasTokens;
            tu->viceId = UNDEFVID;
            afs_FreeTokens(&tu->tokens);
@@ -117,6 +122,10 @@ afspag_PUnlog(char *ain, afs_int32 ainSize, afs_ucred_t **acred)
             */
            tu->tokenTime = 0;
 #endif /* UKERNEL */
+
+           afs_PutUser(tu, WRITE_LOCK);
+
+           ObtainWriteLock(&afs_xuser, 369);
        }
     }
     ReleaseWriteLock(&afs_xuser);
@@ -254,6 +263,11 @@ SPAGCB_GetCreds(struct rx_call *a_call, afs_int32 a_uid,
        if (tu->uid == a_uid && tu->cellinfo &&
            (tu->states & UHasTokens) && !(tu->states & UTokensBad)) {
 
+           tu->refCount++;
+           ReleaseWriteLock(&afs_xuser);
+
+           afs_LockUser(tu, READ_LOCK, 0);
+
            token = afs_FindToken(tu->tokens, RX_SECIDX_KAD);
 
            tci = &a_creds->CredInfos_val[i];
@@ -268,20 +282,28 @@ SPAGCB_GetCreds(struct rx_call *a_call, afs_int32 a_uid,
            cellname = ((struct afspag_cell *)(tu->cellinfo))->cellname;
            clen = strlen(cellname) + 1;
            tci->cellname = afs_osi_Alloc(clen);
-           if (!tci->cellname)
+           if (!tci->cellname) {
+               afs_PutUser(tu, READ_LOCK);
+               ObtainWriteLock(&afs_xuser, 370);
                goto out;
+           }
            memcpy(tci->cellname, cellname, clen);
 
            tci->st.st_len = token->rxkad.ticketLen;
            tci->st.st_val = afs_osi_Alloc(token->rxkad.ticketLen);
            if (!tci->st.st_val) {
+               afs_PutUser(tu, READ_LOCK);
                afs_osi_Free(tci->cellname, clen);
+               ObtainWriteLock(&afs_xuser, 371);
                goto out;
            }
            memcpy(tci->st.st_val,
                   token->rxkad.ticket, token->rxkad.ticketLen);
            if (tu->states & UPrimary)
                tci->states |= UPrimary;
+
+           afs_PutUser(tu, READ_LOCK);
+           ObtainWriteLock(&afs_xuser, 372);
        }
     }
 

diff --git a/src/afs/afs_pioctl.c b/src/afs/afs_pioctl.c
index fced77b..c4f3a7b 100644 (file)
--- a/src/afs/afs_pioctl.c
+++ b/src/afs/afs_pioctl.c
@@ -1761,12 +1761,13 @@ DECL_PIOCTL(PGetUserCell)
        if (tu->uid == areq->uid && (tu->states & UPrimary)) {
            tu->refCount++;
            ReleaseWriteLock(&afs_xuser);
+           afs_LockUser(tu, READ_LOCK, 0);
            break;
        }
     }
     if (tu) {
        tcell = afs_GetCell(tu->cell, READ_LOCK);
-       afs_PutUser(tu, WRITE_LOCK);
+       afs_PutUser(tu, READ_LOCK);
        if (!tcell)
            return ESRCH;
        else {
@@ -2266,6 +2267,10 @@ getNthCell(afs_int32 uid, afs_int32 iterator) {
        tu->refCount++;
     }
     ReleaseReadLock(&afs_xuser);
+    if (tu) {
+       afs_LockUser(tu, READ_LOCK, 0);
+    }
+
 
     return tu;
 }
@@ -2416,10 +2421,13 @@ DECL_PIOCTL(PUnlog)
     ObtainWriteLock(&afs_xuser, 227);
     for (tu = afs_users[i]; tu; tu = tu->next) {
        if (tu->uid == areq->uid) {
-           tu->states &= ~UHasTokens;
-           afs_FreeTokens(&tu->tokens);
            tu->refCount++;
            ReleaseWriteLock(&afs_xuser);
+
+           afs_LockUser(tu, WRITE_LOCK, 366);
+
+           tu->states &= ~UHasTokens;
+           afs_FreeTokens(&tu->tokens);
            afs_NotifyUser(tu, UTokensDropped);
            /* We have to drop the lock over the call to afs_ResetUserConns,
             * since it obtains the afs_xvcache lock.  We could also keep
@@ -2430,7 +2438,7 @@ DECL_PIOCTL(PUnlog)
             * every user conn that existed when we began this call.
             */
            afs_ResetUserConns(tu);
-           tu->refCount--;
+           afs_PutUser(tu, WRITE_LOCK);
            ObtainWriteLock(&afs_xuser, 228);
 #ifdef UKERNEL
            /* set the expire times to 0, causes
@@ -5477,12 +5485,15 @@ DECL_PIOCTL(PNFSNukeCreds)
     for (i = 0; i < NUSERS; i++) {
        for (tu = afs_users[i]; tu; tu = tu->next) {
            if (tu->exporter && EXP_CHECKHOST(tu->exporter, addr)) {
-               tu->states &= ~UHasTokens;
-               afs_FreeTokens(&tu->tokens);
                tu->refCount++;
                ReleaseWriteLock(&afs_xuser);
+
+               afs_LockUser(tu, WRITE_LOCK, 367);
+
+               tu->states &= ~UHasTokens;
+               afs_FreeTokens(&tu->tokens);
                afs_ResetUserConns(tu);
-               tu->refCount--;
+               afs_PutUser(tu, WRITE_LOCK);
                ObtainWriteLock(&afs_xuser, 228);
 #ifdef UKERNEL
                /* set the expire times to 0, causes

diff --git a/src/afs/afs_prototypes.h b/src/afs/afs_prototypes.h
index efcf622..11d49fc 100644 (file)
--- a/src/afs/afs_prototypes.h
+++ b/src/afs/afs_prototypes.h
@@ -975,6 +975,8 @@ extern struct unixuser *afs_FindUser(afs_int32 auid, afs_int32 acell,
                                     afs_int32 locktype);
 extern struct unixuser *afs_GetUser(afs_int32 auid, afs_int32 acell,
                                    afs_int32 locktype);
+extern void afs_LockUser(struct unixuser *au, afs_int32 locktype,
+                         unsigned int src_indicator);
 extern void afs_NotifyUser(struct unixuser *auser, int event);
 
 #if AFS_GCPAGS

diff --git a/src/afs/afs_user.c b/src/afs/afs_user.c
index fe7fb04..2655c64 100644 (file)
--- a/src/afs/afs_user.c
+++ b/src/afs/afs_user.c
@@ -239,6 +239,7 @@ afs_FindUser(afs_int32 auid, afs_int32 acell, afs_int32 locktype)
        if (tu->uid == auid && ((tu->cell == acell) || (acell == -1))) {
            tu->refCount++;
            ReleaseWriteLock(&afs_xuser);
+           afs_LockUser(tu, locktype, 365);
            return tu;
        }
     }
@@ -427,12 +428,10 @@ afs_GetUser(afs_int32 auid, afs_int32 acell, afs_int32 locktype)
                /* Here we setup the real cell for the client */
                tu->cell = acell;
                tu->refCount++;
-               ReleaseWriteLock(&afs_xuser);
-               return tu;
+               goto done;
            } else if (tu->cell == acell || acell == -1) {
                tu->refCount++;
-               ReleaseWriteLock(&afs_xuser);
-               return tu;
+               goto done;
            }
        }
     }
@@ -442,6 +441,7 @@ afs_GetUser(afs_int32 auid, afs_int32 acell, afs_int32 locktype)
     afs_stats_cmfullperf.authent.PAGCreations++;
 #endif /* AFS_NOSTATS */
     memset(tu, 0, sizeof(struct unixuser));
+    AFS_RWLOCK_INIT(&tu->lock, "unixuser lock");
     tu->next = afs_users[i];
     afs_users[i] = tu;
     if (RmtUser) {
@@ -461,16 +461,54 @@ afs_GetUser(afs_int32 auid, afs_int32 acell, afs_int32 locktype)
     tu->viceId = UNDEFVID;
     tu->refCount = 1;
     tu->tokenTime = osi_Time();
+
+ done:
     ReleaseWriteLock(&afs_xuser);
+    afs_LockUser(tu, locktype, 364);
     return tu;
 
 }                              /*afs_GetUser */
 
+void
+afs_LockUser(struct unixuser *au, afs_int32 locktype,
+             unsigned int src_indicator)
+{
+    switch (locktype) {
+    case READ_LOCK:
+       ObtainReadLock(&au->lock);
+       break;
+    case WRITE_LOCK:
+       ObtainWriteLock(&au->lock, src_indicator);
+       break;
+    case SHARED_LOCK:
+       ObtainSharedLock(&au->lock, src_indicator);
+       break;
+    default:
+       /* noop */
+       break;
+    }
+}
 
 void
 afs_PutUser(struct unixuser *au, afs_int32 locktype)
 {
     AFS_STATCNT(afs_PutUser);
+
+    switch (locktype) {
+    case READ_LOCK:
+       ReleaseReadLock(&au->lock);
+       break;
+    case WRITE_LOCK:
+       ReleaseWriteLock(&au->lock);
+       break;
+    case SHARED_LOCK:
+       ReleaseSharedLock(&au->lock);
+       break;
+    default:
+       /* noop */
+       break;
+    }
+
     --au->refCount;
 }                              /*afs_PutUser */
 

diff --git a/src/afs/afs_util.c b/src/afs/afs_util.c
index f8d17fb..9d022fa 100644 (file)
--- a/src/afs/afs_util.c
+++ b/src/afs/afs_util.c
@@ -301,6 +301,8 @@ afs_CheckLocks(void)
 
        for (i = 0; i < NUSERS; i++) {
            for (tu = afs_users[i]; tu; tu = tu->next) {
+               if (CheckLock(&tu->lock))
+                   afs_warn("user at %p is locked\n", tu);
                if (tu->refCount)
                    afs_warn("user at %lx is held\n", (unsigned long)tu);
            }