From 89b22dfe8659cd2e576cc2cd2b455598db59aacc Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 5 May 2011 15:10:54 -0500 Subject: [PATCH] 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 Tested-by: Derrick Brashear --- src/afs/DOC/afs_rwlocks | 4 ++++ src/afs/LINUX/osi_proc.c | 8 +++++++ src/afs/LINUX24/osi_proc.c | 8 +++++++ src/afs/VNOPS/afs_vnop_lookup.c | 12 +++++------ src/afs/afs.h | 1 + src/afs/afs_nfsclnt.c | 6 ++++-- src/afs/afs_pag_cred.c | 24 ++++++++++++++++++++- src/afs/afs_pioctl.c | 25 +++++++++++++++------- src/afs/afs_prototypes.h | 2 ++ src/afs/afs_user.c | 46 +++++++++++++++++++++++++++++++++++++---- src/afs/afs_util.c | 2 ++ 11 files changed, 118 insertions(+), 20 deletions(-) diff --git a/src/afs/DOC/afs_rwlocks b/src/afs/DOC/afs_rwlocks index ce28ce1..ff2ee6f 100644 --- 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 --- 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 = ""; } 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 --- 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 = ""; } 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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); } -- 1.9.4