From c4dfacc9341cd47805de5cd2d2de151199344a67 Mon Sep 17 00:00:00 2001 From: Michael Meffie Date: Mon, 9 Nov 2009 11:03:10 -0500 Subject: [PATCH] cm: address race condition in afs_QueueVCB Access the vcache callback member after taking the xvcb lock to avoid the server object from being freed in FlushServer on another thread. Eventually, we should have a ref count on avc->server. FIXES 125596 Change-Id: I760819b1632d0e8188eaa34531239951aab980d3 Reviewed-on: http://gerrit.openafs.org/800 Reviewed-by: Simon Wilkinson Reviewed-by: Derrick Brashear Tested-by: Derrick Brashear --- src/afs/afs_vcache.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/afs/afs_vcache.c b/src/afs/afs_vcache.c index 7c4ee99..a68af8e 100644 --- a/src/afs/afs_vcache.c +++ b/src/afs/afs_vcache.c @@ -212,14 +212,7 @@ afs_FlushVCache(struct vcache *avc, int *slept) vn_reinit(AFSTOV(avc)); #endif afs_FreeAllAxs(&(avc->Access)); - - /* we can't really give back callbacks on RO files, since the - * server only tracks them on a per-volume basis, and we don't - * know whether we still have some other files from the same - * volume. */ - if ((avc->f.states & CRO) == 0 && avc->callback) { - afs_QueueVCB(avc); - } + afs_QueueVCB(avc); ObtainWriteLock(&afs_xcbhash, 460); afs_DequeueCallback(avc); /* remove it from queued callbacks list */ avc->f.states &= ~(CStatd | CUnique); @@ -491,23 +484,34 @@ afs_FlushVCBs(afs_int32 lockit) * Called when the xvcache lock is already held. * * \param avc vcache entry - * \return 0 for success < 0 otherwise. + * \return 1 if queued, 0 otherwise */ static afs_int32 afs_QueueVCB(struct vcache *avc) { + int queued = 0; struct server *tsp; struct afs_cbr *tcbp; AFS_STATCNT(afs_QueueVCB); + + MObtainWriteLock(&afs_xvcb, 274); + + /* we can't really give back callbacks on RO files, since the + * server only tracks them on a per-volume basis, and we don't + * know whether we still have some other files from the same + * volume. */ + if (!((avc->f.states & CRO) == 0 && avc->callback)) { + goto done; + } + /* The callback is really just a struct server ptr. */ tsp = (struct server *)(avc->callback); /* we now have a pointer to the server, so we just allocate * a queue entry and queue it. */ - MObtainWriteLock(&afs_xvcb, 274); tcbp = afs_AllocCBR(); tcbp->fid = avc->f.fid.Fid; @@ -519,10 +523,12 @@ afs_QueueVCB(struct vcache *avc) tcbp->pprev = &tsp->cbrs; afs_InsertHashCBR(tcbp); + queued = 1; + done: /* now release locks and return */ MReleaseWriteLock(&afs_xvcb); - return 0; + return queued; } @@ -3258,8 +3264,7 @@ afs_DisconGiveUpCallbacks(void) { /* Somehow, walk the set of vcaches, with each one coming out as tvc */ for (i = 0; i < VCSIZE; i++) { for (tvc = afs_vhashT[i]; tvc; tvc = tvc->hnext) { - if ((tvc->f.states & CRO) == 0 && tvc->callback) { - afs_QueueVCB(tvc); + if (afs_QueueVCB(tvc)) { tvc->callback = NULL; nq++; } -- 1.9.4