https://git.reactos.org/?p=reactos.git;a=commitdiff;h=cf7969fbfaa3ff637dd57a...
commit cf7969fbfaa3ff637dd57ad64743ee9d216f8d58 Author: Pierre Schweitzer pierre@reactos.org AuthorDate: Sun Oct 28 20:48:01 2018 +0100 Commit: Pierre Schweitzer pierre@reactos.org CommitDate: Sun Oct 28 20:48:01 2018 +0100
[NTOSKRNL] Fix refcounting for BCBs
Now, we make sure that we update ref count and BCB list membership with the BCB lock held, in a row. This will avoid race conditions where the BCB was removed from the list, then referenced again, leading to inconsistencies in memory and crashes later on. This could notably be triggered while building ReactOS on ReactOS (one would call this a regression).
CORE-15235 --- ntoskrnl/cc/pin.c | 120 ++++++++++++++++++++++++++++++++---------------------- 1 file changed, 71 insertions(+), 49 deletions(-)
diff --git a/ntoskrnl/cc/pin.c b/ntoskrnl/cc/pin.c index dc7427427e..775b105a2f 100644 --- a/ntoskrnl/cc/pin.c +++ b/ntoskrnl/cc/pin.c @@ -152,6 +152,38 @@ CcpMapData( return TRUE; }
+static +VOID +CcpDereferenceBcb( + IN PROS_SHARED_CACHE_MAP SharedCacheMap, + IN PINTERNAL_BCB Bcb) +{ + ULONG RefCount; + KIRQL OldIrql; + + KeAcquireSpinLock(&SharedCacheMap->BcbSpinLock, &OldIrql); + RefCount = --Bcb->RefCount; + if (RefCount == 0) + { + RemoveEntryList(&Bcb->BcbEntry); + KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql); + + ASSERT(Bcb->PinCount == 0); + CcRosReleaseVacb(SharedCacheMap, + Bcb->Vacb, + TRUE, + Bcb->Dirty, + FALSE); + + ExDeleteResourceLite(&Bcb->Lock); + ExFreeToNPagedLookasideList(&iBcbLookasideList, Bcb); + } + else + { + KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql); + } +} + static PVOID CcpGetAppropriateBcb( @@ -191,12 +223,13 @@ CcpGetAppropriateBcb( /* Yes, and we've lost */ if (DupBcb != NULL) { + /* We will return that BCB */ + ++DupBcb->RefCount; Result = TRUE; + KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql);
if (ToPin) { - DupBcb->PinCount++; - if (BooleanFlagOn(PinFlags, PIN_EXCLUSIVE)) { Result = ExAcquireResourceExclusiveLite(&iBcb->Lock, BooleanFlagOn(PinFlags, PIN_WAIT)); @@ -206,24 +239,25 @@ CcpGetAppropriateBcb( Result = ExAcquireSharedStarveExclusive(&iBcb->Lock, BooleanFlagOn(PinFlags, PIN_WAIT)); }
- if (!Result) + if (Result) + { + DupBcb->PinCount++; + } + else { - DupBcb->PinCount--; + CcpDereferenceBcb(SharedCacheMap, DupBcb); DupBcb = NULL; } }
- if (Result) + if (DupBcb != NULL) { - /* We'll return that BCB */ - ++DupBcb->RefCount; + /* Delete the loser */ + CcRosReleaseVacb(SharedCacheMap, Vacb, TRUE, FALSE, FALSE); + ExDeleteResourceLite(&iBcb->Lock); + ExFreeToNPagedLookasideList(&iBcbLookasideList, iBcb); }
- /* Delete the loser */ - CcRosReleaseVacb(SharedCacheMap, Vacb, TRUE, FALSE, FALSE); - ExDeleteResourceLite(&iBcb->Lock); - ExFreeToNPagedLookasideList(&iBcbLookasideList, iBcb); - /* Return the winner - no need to update buffer address, it's * relative to the VACB, which is unchanged. */ @@ -249,8 +283,8 @@ CcpGetAppropriateBcb( }
InsertTailList(&SharedCacheMap->BcbList, &iBcb->BcbEntry); + KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql); } - KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql);
return iBcb; } @@ -273,11 +307,11 @@ CcpPinData(
KeAcquireSpinLock(&SharedCacheMap->BcbSpinLock, &OldIrql); NewBcb = CcpFindBcb(SharedCacheMap, FileOffset, Length, TRUE); - KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql);
if (NewBcb != NULL) { - NewBcb->PinCount++; + ++NewBcb->RefCount; + KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql);
if (BooleanFlagOn(Flags, PIN_EXCLUSIVE)) { @@ -290,11 +324,12 @@ CcpPinData(
if (!Result) { - NewBcb->PinCount--; + CcpDereferenceBcb(SharedCacheMap, NewBcb); + NewBcb = NULL; } else { - NewBcb->RefCount++; + NewBcb->PinCount++; *Bcb = NewBcb; *Buffer = (PUCHAR)NewBcb->Vacb->BaseAddress + FileOffset->QuadPart % VACB_MAPPING_GRANULARITY; } @@ -303,6 +338,8 @@ CcpPinData( } else { + KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql); + if (BooleanFlagOn(Flags, PIN_IF_BCB)) { return FALSE; @@ -374,10 +411,11 @@ CcMapData (
KeAcquireSpinLock(&SharedCacheMap->BcbSpinLock, &OldIrql); iBcb = CcpFindBcb(SharedCacheMap, FileOffset, Length, FALSE); - KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql);
if (iBcb == NULL) { + KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql); + Ret = CcpMapData(SharedCacheMap, FileOffset, Length, Flags, &Vacb, pBuffer); if (Ret) { @@ -396,6 +434,8 @@ CcMapData ( else { ++iBcb->RefCount; + KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql); + *pBcb = iBcb; *pBuffer = (PUCHAR)iBcb->Vacb->BaseAddress + FileOffset->QuadPart % VACB_MAPPING_GRANULARITY; Ret = TRUE; @@ -564,6 +604,7 @@ CcUnpinDataForThread ( IN ERESOURCE_THREAD ResourceThreadId) { PINTERNAL_BCB iBcb = Bcb; + PROS_SHARED_CACHE_MAP SharedCacheMap;
CCTRACE(CC_API_DEBUG, "Bcb=%p ResourceThreadId=%lu\n", Bcb, ResourceThreadId);
@@ -573,29 +614,8 @@ CcUnpinDataForThread ( iBcb->PinCount--; }
- if (--iBcb->RefCount == 0) - { - KIRQL OldIrql; - PROS_SHARED_CACHE_MAP SharedCacheMap; - - ASSERT(iBcb->PinCount == 0); - SharedCacheMap = iBcb->Vacb->SharedCacheMap; - CcRosReleaseVacb(SharedCacheMap, - iBcb->Vacb, - TRUE, - iBcb->Dirty, - FALSE); - - KeAcquireSpinLock(&SharedCacheMap->BcbSpinLock, &OldIrql); - if (!IsListEmpty(&iBcb->BcbEntry)) - { - RemoveEntryList(&iBcb->BcbEntry); - } - KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql); - - ExDeleteResourceLite(&iBcb->Lock); - ExFreeToNPagedLookasideList(&iBcbLookasideList, iBcb); - } + SharedCacheMap = iBcb->Vacb->SharedCacheMap; + CcpDereferenceBcb(SharedCacheMap, iBcb); }
/* @@ -629,9 +649,15 @@ CcUnpinRepinnedBcb (
CCTRACE(CC_API_DEBUG, "Bcb=%p WriteThrough=%d\n", Bcb, WriteThrough);
+ SharedCacheMap = iBcb->Vacb->SharedCacheMap; IoStatus->Status = STATUS_SUCCESS; + + KeAcquireSpinLock(&SharedCacheMap->BcbSpinLock, &OldIrql); if (--iBcb->RefCount == 0) { + RemoveEntryList(&iBcb->BcbEntry); + KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql); + IoStatus->Information = 0; if (WriteThrough) { @@ -656,21 +682,17 @@ CcUnpinRepinnedBcb ( ASSERT(iBcb->PinCount == 0); }
- SharedCacheMap = iBcb->Vacb->SharedCacheMap; CcRosReleaseVacb(iBcb->Vacb->SharedCacheMap, iBcb->Vacb, TRUE, iBcb->Dirty, FALSE);
- KeAcquireSpinLock(&SharedCacheMap->BcbSpinLock, &OldIrql); - if (!IsListEmpty(&iBcb->BcbEntry)) - { - RemoveEntryList(&iBcb->BcbEntry); - } - KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql); - ExDeleteResourceLite(&iBcb->Lock); ExFreeToNPagedLookasideList(&iBcbLookasideList, iBcb); } + else + { + KeReleaseSpinLock(&SharedCacheMap->BcbSpinLock, OldIrql); + } }
-
Pierre Schweitzer