https://git.reactos.org/?p=reactos.git;a=commitdiff;h=13b57fb5b5688a5e67175…
commit 13b57fb5b5688a5e67175e0908f1fe7ad272045b
Author: Pierre Schweitzer <pierre(a)reactos.org>
AuthorDate: Sat Mar 17 11:56:25 2018 +0100
Commit: Pierre Schweitzer <pierre(a)reactos.org>
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