[ros-diffs] 01/01: [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

[Pierre Schweitzer]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=13b57fb5b5688a5e67175e...

commit 13b57fb5b5688a5e67175e0908f1fe7ad272045b
Author:     Pierre Schweitzer [e-mail]
AuthorDate: Sat Mar 17 11:56:25 2018 +0100
Commit:     Pierre Schweitzer [e-mail]
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(&current->CacheMapVacbListEntry);
             RemoveEntryList(&current->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(&current->Mutex, 0);
     CcRosAcquireVacbLock(current, NULL);
@@ -785,6 +777,7 @@ CcRosCreateVacb (
     }
     KeReleaseSpinLock(&SharedCacheMap->CacheMapLock, oldIrql);
     InsertTailList(&VacbLruListHead, &current->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(&current->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