Index: /trunk/src/VBox/Devices/Network/SrvIntNetR0.cpp =================================================================== --- /trunk/src/VBox/Devices/Network/SrvIntNetR0.cpp (revision 60524) +++ /trunk/src/VBox/Devices/Network/SrvIntNetR0.cpp (revision 60525) @@ -218,7 +218,6 @@ * This is shadowed by INTNETMACTABENTRY::fActive. */ bool fActive; - /** Whether someone is currently in the destructor or has indicated that - * the end is nigh by means of IntNetR0IfAbortWait. */ - bool volatile fDestroying; + /** Whether someone has indicated that the end is nigh by means of IntNetR0IfAbortWait. */ + bool volatile fNoMoreWaits; /** The flags specified when opening this interface. */ uint32_t fOpenFlags; @@ -240,9 +239,14 @@ RTSEMEVENT volatile hRecvEvent; /** Number of threads sleeping on the event semaphore. */ - uint32_t cSleepers; + uint32_t volatile cSleepers; /** The interface handle. * When this is INTNET_HANDLE_INVALID a sleeper which is waking up * should return with the appropriate error condition. */ INTNETIFHANDLE volatile hIf; + /** The native handle of the destructor thread. This is NIL_RTNATIVETHREAD when + * the object is valid and set when intnetR0IfDestruct is in progress. This is + * used to cover an unlikely (impossible?) race between SUPDRVSESSION cleanup + * and lingering threads waiting for recv or similar. */ + RTNATIVETHREAD volatile hDestructorThread; /** Pointer to the network this interface is connected to. * This is protected by the INTNET::hMtxCreateOpenDestroy. */ @@ -771,6 +775,9 @@ DECLINLINE(int) intnetR0IfRetain(PINTNETIF pIf, PSUPDRVSESSION pSession) { + Assert(((PINTNETIF)pIf->pvObj)->hDestructorThread == NIL_RTNATIVETHREAD); + int rc = SUPR0ObjAddRefEx(pIf->pvObj, pSession, true /* fNoBlocking */); AssertRCReturn(rc, rc); + return VINF_SUCCESS; } @@ -787,6 +794,9 @@ DECLINLINE(bool) intnetR0IfRelease(PINTNETIF pIf, PSUPDRVSESSION pSession) { + Assert(((PINTNETIF)pIf->pvObj)->hDestructorThread == NIL_RTNATIVETHREAD); + int rc = SUPR0ObjRelease(pIf->pvObj, pSession); AssertRC(rc); + return rc == VINF_OBJECT_DESTROYED; } @@ -809,8 +819,18 @@ NOREF(pvUser); NOREF(hHandleTable); - PINTNETIF pIf = (PINTNETIF)pvObj; - if (pIf->hIf != INTNET_HANDLE_INVALID) /* Don't try retain it if called from intnetR0IfDestruct. */ + + PINTNETIF pIf = (PINTNETIF)pvObj; + RTNATIVETHREAD hDtorThrd; + ASMAtomicUoReadHandle(&pIf->hDestructorThread, &hDtorThrd); + if (hDtorThrd == NIL_RTNATIVETHREAD) return intnetR0IfRetain(pIf, (PSUPDRVSESSION)pvCtx); - return VINF_SUCCESS; + + /* Allow intnetR0IfDestruct to call RTHandleTableFreeWithCtx to free + the handle, but not even think about retaining a referenceas we don't + want to confuse SUPDrv and risk having the destructor called twice. */ + if (hDtorThrd == RTThreadNativeSelf()) + return VINF_SUCCESS; + + return VERR_SEM_DESTROYED; } @@ -4618,37 +4638,45 @@ } - const INTNETIFHANDLE hIfSelf = pIf->hIf; - const RTSEMEVENT hRecvEvent = pIf->hRecvEvent; - const bool fDestroying = ASMAtomicReadBool(&pIf->fDestroying); - if ( hIfSelf != hIf /* paranoia */ - || hRecvEvent == NIL_RTSEMEVENT - || fDestroying - ) - { + const RTSEMEVENT hRecvEvent = pIf->hRecvEvent; + const bool fNoMoreWaits = ASMAtomicUoReadBool(&pIf->fNoMoreWaits); + RTNATIVETHREAD hDtorThrd; + ASMAtomicReadHandle(&pIf->hDestructorThread, &hDtorThrd); + if (hDtorThrd != NIL_RTNATIVETHREAD) + { + /* See IntNetR0IfAbortWait for an explanation of hDestructorThread. */ Log(("IntNetR0IfWait: returns VERR_SEM_DESTROYED\n")); return VERR_SEM_DESTROYED; } - /* - * It is tempting to check if there is data to be read here, - * but the problem with such an approach is that it will cause - * one unnecessary supervisor->user->supervisor trip. There is - * already a slight risk for such, so no need to increase it. - */ - - /* - * Increment the number of waiters before starting the wait. - * Upon wakeup we must assert reality, checking that we're not - * already destroyed or in the process of being destroyed. This - * code must be aligned with the waiting code in intnetR0IfDestruct. - */ - ASMAtomicIncU32(&pIf->cSleepers); - int rc = RTSemEventWaitNoResume(hRecvEvent, cMillies); - if (pIf->hRecvEvent == hRecvEvent) - { - ASMAtomicDecU32(&pIf->cSleepers); - if (!pIf->fDestroying) + /* Check whether further waits have been barred by IntNetR0IfAbortWait. */ + int rc; + if ( !fNoMoreWaits + && hRecvEvent != NIL_RTSEMEVENT) + { + /* + * It is tempting to check if there is data to be read here, + * but the problem with such an approach is that it will cause + * one unnecessary supervisor->user->supervisor trip. There is + * already a slight risk for such, so no need to increase it. + */ + + /* + * Increment the number of waiters before starting the wait. + * Upon wakeup we must assert reality, checking that we're not + * already destroyed or in the process of being destroyed. This + * code must be aligned with the waiting code in intnetR0IfDestruct. + */ + ASMAtomicIncU32(&pIf->cSleepers); + rc = RTSemEventWaitNoResume(hRecvEvent, cMillies); + if (pIf->hRecvEvent == hRecvEvent) { - if (intnetR0IfRelease(pIf, pSession)) + ASMAtomicDecU32(&pIf->cSleepers); + ASMAtomicReadHandle(&pIf->hDestructorThread, &hDtorThrd); + if (hDtorThrd == NIL_RTNATIVETHREAD) + { + if (intnetR0IfRelease(pIf, pSession)) + rc = VERR_SEM_DESTROYED; + } + else rc = VERR_SEM_DESTROYED; } @@ -4657,5 +4685,9 @@ } else + { rc = VERR_SEM_DESTROYED; + intnetR0IfRelease(pIf, pSession); + } + Log4(("IntNetR0IfWait: returns %Rrc\n", rc)); return rc; @@ -4704,30 +4736,43 @@ } - const INTNETIFHANDLE hIfSelf = pIf->hIf; - const RTSEMEVENT hRecvEvent = pIf->hRecvEvent; - const bool fDestroying = ASMAtomicReadBool(&pIf->fDestroying); - if ( hIfSelf != hIf /* paranoia */ - || hRecvEvent == NIL_RTSEMEVENT - || fDestroying - ) - { + const RTSEMEVENT hRecvEvent = pIf->hRecvEvent; + RTNATIVETHREAD hDtorThrd; + ASMAtomicReadHandle(&pIf->hDestructorThread, &hDtorThrd); + if (hDtorThrd != NIL_RTNATIVETHREAD) + { + /* This can only happen if we for some reason race SUPDRVSESSION cleanup, + i.e. the object count is set to zero without yet having removed it from + the object table, so we got a spurious "reference". We must drop that + reference and let the destructor get on with its work. (Not entirely sure + if this is practically possible on any of the platforms, i.e. whether it's + we can actually close a SUPDrv handle/descriptor with active threads still + in NtDeviceIoControlFile/ioctl, but better safe than sorry.) */ Log(("IntNetR0IfAbortWait: returns VERR_SEM_DESTROYED\n")); return VERR_SEM_DESTROYED; } - /* - * Set fDestroying if requested to do so and then wake up all the sleeping - * threads (usually just one). We leave the semaphore in the signalled - * state so the next caller will return immediately. - */ - if (fNoMoreWaits) - ASMAtomicWriteBool(&pIf->fDestroying, true); - - uint32_t cSleepers = ASMAtomicReadU32(&pIf->cSleepers) + 1; - while (cSleepers-- > 0) - { - int rc = RTSemEventSignal(pIf->hRecvEvent); - AssertRC(rc); - } + /* a bit of paranoia */ + int rc = VINF_SUCCESS; + if (hRecvEvent != NIL_RTSEMEVENT) + { + /* + * Set fNoMoreWaits if requested to do so and then wake up all the sleeping + * threads (usually just one). We leave the semaphore in the signalled + * state so the next caller will return immediately. + */ + if (fNoMoreWaits) + ASMAtomicWriteBool(&pIf->fNoMoreWaits, true); + + uint32_t cSleepers = ASMAtomicReadU32(&pIf->cSleepers) + 1; + while (cSleepers-- > 0) + { + int rc2 = RTSemEventSignal(pIf->hRecvEvent); + AssertRC(rc2); + } + } + else + rc = VERR_SEM_DESTROYED; + + intnetR0IfRelease(pIf, pSession); Log4(("IntNetR0IfWait: returns %Rrc\n", VINF_SUCCESS)); @@ -4831,11 +4876,16 @@ /* + * For paranoid reasons we must now mark the interface as destroyed. + * This is so that any waiting threads can take evasive action (kind + * of theoretical case), and we can reject everyone else referencing + * the object via the handle table before we get around to removing it. + */ + ASMAtomicWriteHandle(&pIf->hDestructorThread, RTThreadNativeSelf()); + + /* * We grab the INTNET create/open/destroy semaphore to make sure nobody is - * adding or removing interface while we're in here. For paranoid reasons - * we also mark the interface as destroyed here so any waiting threads can - * take evasive action (theoretical case). + * adding or removing interfaces while we're in here. */ RTSemMutexRequest(pIntNet->hMtxCreateOpenDestroy, RT_INDEFINITE_WAIT); - ASMAtomicWriteBool(&pIf->fDestroying, true); /* @@ -4921,5 +4971,6 @@ /* - * Wakeup anyone waiting on this interface. + * Wakeup anyone waiting on this interface. (Kind of unlikely, but perhaps + * not quite impossible.) * * We *must* make sure they have woken up properly and realized @@ -5045,5 +5096,5 @@ //pIf->fPromiscuousReal = false; //pIf->fActive = false; - //pIf->fDestroying = false; + //pIf->fNoMoreWaits = false; pIf->fOpenFlags = fFlags; //pIf->cYields = 0; @@ -5055,4 +5106,5 @@ //pIf->cSleepers = 0; pIf->hIf = INTNET_HANDLE_INVALID; + pIf->hDestructorThread = NIL_RTNATIVETHREAD; pIf->pNetwork = pNetwork; pIf->pSession = pSession;