https://git.reactos.org/?p=reactos.git;a=commitdiff;h=13b57fb5b5688a5e67175e0908f1fe7ad272045b
commit 13b57fb5b5688a5e67175e0908f1fe7ad272045b Author: Pierre Schweitzer <[email protected]> AuthorDate: Sat Mar 17 11:56:25 2018 +0100 Commit: Pierre Schweitzer <[email protected]> CommitDate: Sat Mar 17 11:56:25 2018 +0100 [NTOSKRNL] Misc fixes to VACB reference counting This fixes various bugs linked to VACB counting: - VACB not released when it should be - Reference count expectations not being accurate For the record, VACB should always have at least a reference count of 1, unless you are to free it and removed it from any linked list. This commit also adds a bunch of asserts that should help triggering invalid reference counting. It should also fix numerous ASSERT currently triggered and may help fixing random behaviours in Cc. CORE-14285 CORE-14401 CORE-14293 --- ntoskrnl/cc/fs.c | 9 ++++++-- ntoskrnl/cc/pin.c | 8 +++++++ ntoskrnl/cc/view.c | 51 ++++++++++++++++++++++-------------------- ntoskrnl/include/internal/cc.h | 21 +++++++++++++++++ 4 files changed, 63 insertions(+), 26 deletions(-) diff --git a/ntoskrnl/cc/fs.c b/ntoskrnl/cc/fs.c index 38c9ae527c..05a9a89475 100644 --- a/ntoskrnl/cc/fs.c +++ b/ntoskrnl/cc/fs.c @@ -222,14 +222,18 @@ CcPurgeCacheSection ( break; } - /* Still in use, it cannot be purged, fail */ - if (Vacb->ReferenceCount != 0 && !Vacb->Dirty) + /* Still in use, it cannot be purged, fail + * Allow one ref: VACB is supposed to be always 1-referenced + */ + if ((Vacb->ReferenceCount > 1 && !Vacb->Dirty) || + (Vacb->ReferenceCount > 2 && Vacb->Dirty)) { Success = FALSE; break; } /* This VACB is in range, so unlink it and mark for free */ + ASSERT(Vacb->ReferenceCount == 1 || Vacb->Dirty); RemoveEntryList(&Vacb->VacbLruListEntry); if (Vacb->Dirty) { @@ -246,6 +250,7 @@ CcPurgeCacheSection ( Vacb = CONTAINING_RECORD(RemoveHeadList(&FreeList), ROS_VACB, CacheMapVacbListEntry); + CcRosVacbDecRefCount(Vacb); CcRosInternalFreeVacb(Vacb); } diff --git a/ntoskrnl/cc/pin.c b/ntoskrnl/cc/pin.c index 20b409662f..c9167834c0 100644 --- a/ntoskrnl/cc/pin.c +++ b/ntoskrnl/cc/pin.c @@ -382,7 +382,15 @@ CcUnpinRepinnedBcb ( iBcb->Pinned = FALSE; CcRosAcquireVacbLock(iBcb->Vacb, NULL); iBcb->Vacb->PinCount--; + ASSERT(iBcb->Vacb->PinCount == 0); } + + CcRosReleaseVacb(iBcb->Vacb->SharedCacheMap, + iBcb->Vacb, + TRUE, + iBcb->Dirty, + FALSE); + ExDeleteResourceLite(&iBcb->Lock); ExFreeToNPagedLookasideList(&iBcbLookasideList, iBcb); } diff --git a/ntoskrnl/cc/view.c b/ntoskrnl/cc/view.c index cf2d2c5cb7..4651abe670 100644 --- a/ntoskrnl/cc/view.c +++ b/ntoskrnl/cc/view.c @@ -65,7 +65,7 @@ KSPIN_LOCK CcDeferredWriteSpinLock; LIST_ENTRY CcCleanSharedCacheMapList; #if DBG -static void CcRosVacbIncRefCount_(PROS_VACB vacb, const char* file, int line) +VOID CcRosVacbIncRefCount_(PROS_VACB vacb, PCSTR file, INT line) { ++vacb->ReferenceCount; if (vacb->SharedCacheMap->Trace) @@ -74,7 +74,7 @@ static void CcRosVacbIncRefCount_(PROS_VACB vacb, const char* file, int line) file, line, vacb, vacb->ReferenceCount, vacb->Dirty, vacb->PageOut); } } -static void CcRosVacbDecRefCount_(PROS_VACB vacb, const char* file, int line) +VOID CcRosVacbDecRefCount_(PROS_VACB vacb, PCSTR file, INT line) { ASSERT(vacb->ReferenceCount != 0); --vacb->ReferenceCount; @@ -85,11 +85,6 @@ static void CcRosVacbDecRefCount_(PROS_VACB vacb, const char* file, int line) file, line, vacb, vacb->ReferenceCount, vacb->Dirty, vacb->PageOut); } } -#define CcRosVacbIncRefCount(vacb) CcRosVacbIncRefCount_(vacb,__FILE__,__LINE__) -#define CcRosVacbDecRefCount(vacb) CcRosVacbDecRefCount_(vacb,__FILE__,__LINE__) -#else -#define CcRosVacbIncRefCount(vacb) (++((vacb)->ReferenceCount)) -#define CcRosVacbDecRefCount(vacb) (--((vacb)->ReferenceCount)) #endif NTSTATUS @@ -350,10 +345,11 @@ retry: CcRosVacbDecRefCount(current); /* Check if we can free this entry now */ - if (current->ReferenceCount == 0) + if (current->ReferenceCount < 2) { ASSERT(!current->Dirty); ASSERT(!current->MappedCount); + ASSERT(current->ReferenceCount == 1); RemoveEntryList(¤t->CacheMapVacbListEntry); RemoveEntryList(¤t->VacbLruListEntry); @@ -395,6 +391,7 @@ retry: current = CONTAINING_RECORD(current_entry, ROS_VACB, CacheMapVacbListEntry); + CcRosVacbDecRefCount(current); CcRosInternalFreeVacb(current); } @@ -434,6 +431,8 @@ CcRosReleaseVacb ( CcRosVacbIncRefCount(Vacb); } + ASSERT(Vacb->ReferenceCount != 0); + CcRosReleaseVacbLock(Vacb); return STATUS_SUCCESS; @@ -575,16 +574,15 @@ CcRosMarkDirtyFile ( KeBugCheck(CACHE_MANAGER); } - if (!Vacb->Dirty) - { - CcRosMarkDirtyVacb(Vacb); - } - - CcRosReleaseVacbLock(Vacb); + CcRosReleaseVacb(SharedCacheMap, Vacb, Vacb->Valid, TRUE, FALSE); return STATUS_SUCCESS; } +/* + * Note: this is not the contrary function of + * CcRosMapVacbInKernelSpace() + */ NTSTATUS NTAPI CcRosUnmapVacb ( @@ -605,28 +603,22 @@ CcRosUnmapVacb ( return STATUS_UNSUCCESSFUL; } - if (NowDirty && !Vacb->Dirty) - { - CcRosMarkDirtyVacb(Vacb); - } - ASSERT(Vacb->MappedCount != 0); Vacb->MappedCount--; - CcRosVacbDecRefCount(Vacb); if (Vacb->MappedCount == 0) { CcRosVacbDecRefCount(Vacb); } - CcRosReleaseVacbLock(Vacb); + CcRosReleaseVacb(SharedCacheMap, Vacb, Vacb->Valid, NowDirty, FALSE); return STATUS_SUCCESS; } static NTSTATUS -CcRosMapVacb( +CcRosMapVacbInKernelSpace( PROS_VACB Vacb) { ULONG i; @@ -721,7 +713,7 @@ CcRosCreateVacb ( current->MappedCount = 0; current->DirtyVacbListEntry.Flink = NULL; current->DirtyVacbListEntry.Blink = NULL; - current->ReferenceCount = 1; + current->ReferenceCount = 0; current->PinCount = 0; KeInitializeMutex(¤t->Mutex, 0); CcRosAcquireVacbLock(current, NULL); @@ -785,6 +777,7 @@ CcRosCreateVacb ( } KeReleaseSpinLock(&SharedCacheMap->CacheMapLock, oldIrql); InsertTailList(&VacbLruListHead, ¤t->VacbLruListEntry); + CcRosVacbIncRefCount(current); KeReleaseGuardedMutex(&ViewLock); MI_SET_USAGE(MI_USAGE_CACHE); @@ -806,7 +799,7 @@ CcRosCreateVacb ( } #endif - Status = CcRosMapVacb(current); + Status = CcRosMapVacbInKernelSpace(current); if (!NT_SUCCESS(Status)) { RemoveEntryList(¤t->CacheMapVacbListEntry); @@ -849,6 +842,8 @@ CcRosGetVacb ( { return Status; } + + CcRosVacbIncRefCount(current); } KeAcquireGuardedMutex(&ViewLock); @@ -941,6 +936,13 @@ CcRosInternalFreeVacb ( NULL); MmUnlockAddressSpace(MmGetKernelAddressSpace()); + if (Vacb->PinCount != 0 || Vacb->ReferenceCount != 0) + { + DPRINT1("Invalid free: %ld, %ld\n", Vacb->ReferenceCount, Vacb->PinCount); + } + + ASSERT(Vacb->PinCount == 0); + ASSERT(Vacb->ReferenceCount == 0); ExFreeToNPagedLookasideList(&VacbLookasideList, Vacb); return STATUS_SUCCESS; } @@ -1092,6 +1094,7 @@ CcRosDeleteFileCache ( { current_entry = RemoveTailList(&FreeList); current = CONTAINING_RECORD(current_entry, ROS_VACB, CacheMapVacbListEntry); + CcRosVacbDecRefCount(current); CcRosInternalFreeVacb(current); } diff --git a/ntoskrnl/include/internal/cc.h b/ntoskrnl/include/internal/cc.h index e91f1b4993..ad939a12a9 100644 --- a/ntoskrnl/include/internal/cc.h +++ b/ntoskrnl/include/internal/cc.h @@ -519,3 +519,24 @@ IsPointInRange( } #define CcBugCheck(A, B, C) KeBugCheckEx(CACHE_MANAGER, BugCheckFileId | ((ULONG)(__LINE__)), A, B, C) + +#if DBG +#define CcRosVacbIncRefCount(vacb) CcRosVacbIncRefCount_(vacb,__FILE__,__LINE__) +#define CcRosVacbDecRefCount(vacb) CcRosVacbDecRefCount_(vacb,__FILE__,__LINE__) + +VOID +CcRosVacbIncRefCount_( + PROS_VACB vacb, + PCSTR file, + INT line); + +VOID +CcRosVacbDecRefCount_( + PROS_VACB vacb, + PCSTR file, + INT line); + +#else +#define CcRosVacbIncRefCount(vacb) (++((vacb)->ReferenceCount)) +#define CcRosVacbDecRefCount(vacb) (--((vacb)->ReferenceCount)) +#endif
