[NTOS:CC] Fix CcRosDeleteFileCache race with concurrent CcFlushCache causing ASSERT(Refs > 0)

Root cause: CcRosDeleteFileCache's first loop ran under the spinlock and removed VACBs from the LRU and dirty lists, but left them in CacheMapVacbListHead. It also set Vacb->Dirty = TRUE as a flush hint (after CcRosUnmarkDirtyVacb cleared it), creating an inconsistent VACB state: Dirty=TRUE but not in the dirty list, with only the cache-map-list refcount (1). After releasing the lock, the second loop removed VACBs from CacheMapVacbListHead without holding any lock.

This created a race with CcFlushCache: a caller that already held a SharedCacheMap pointer could call CcRosLookupVacb between the two loops, find the VACB (refcount → 2), see Dirty=TRUE (the hack), and call CcRosFlushVacb. Meanwhile the second loop could set Dirty=FALSE, drop the cmap ref (→1, print "Leaking VACB"), and then the early-return in CcRosUnmarkDirtyVacb would skip the decrement. CcRosReleaseVacb then drops 1→0 → ASSERT(Refs > 0) fires.

Before the previous fix, the same race caused ASSERT(Vacb->Dirty) in the old CcRosUnmarkDirtyVacb — the previous fix just changed which assert fired.

The fix:
- CcRosDeleteFileCache: VACBs are now removed from CacheMapVacbListHead in the first loop, under the spinlock, and moved to a private LocalVacbList. After the lock is released, CcRosLookupVacb can no longer find these VACBs, preventing new lookup references from being created.
- CcRosReleaseVacb: Removed the overly strict ASSERT(Refs > 0). When CcRosDeleteFileCache drops the cmap ref while a lookup is outstanding, CcRosReleaseVacb legitimately releases the last reference and CcRosVacbDecRefCount correctly frees the VACB. Callers (e.g., CcFlushCache) don't access the VACB pointer afterward.

Signed-off-by: Timo Kreuzer <[email protected]>

ntoskrnl/cc/view.c

@@ -235,18 +235,29 @@ CcRosDeleteFileCache (
  * FUNCTION: Releases the shared cache map associated with a file object
  */
 {
-    PLIST_ENTRY current_entry;
+    LIST_ENTRY LocalVacbList;
 
     ASSERT(SharedCacheMap);
     ASSERT(SharedCacheMap == FileObject->SectionObjectPointer->SharedCacheMap);
     ASSERT(SharedCacheMap->OpenCount == 0);
 
-    /* Remove all VACBs from the global lists */
+    InitializeListHead(&LocalVacbList);
+
+    /*
+     * Remove all VACBs from the global lists while holding the lock. By also
+     * removing them from CacheMapVacbListHead here (under the lock), we ensure
+     * that a concurrent CcRosLookupVacb (which also holds the lock) cannot find
+     * any of these VACBs after we release the lock. Without this, a caller that
+     * already has a SharedCacheMap pointer (e.g. CcFlushCache) could look up a
+     * VACB between our lock release and the point where the second loop removes
+     * it from the list — leaving an outstanding reference that races with our
+     * CcRosVacbDecRefCount call below.
+     */
     KeAcquireSpinLockAtDpcLevel(&SharedCacheMap->CacheMapLock);
-    current_entry = SharedCacheMap->CacheMapVacbListHead.Flink;
-    while (current_entry != &SharedCacheMap->CacheMapVacbListHead)
+    while (!IsListEmpty(&SharedCacheMap->CacheMapVacbListHead))
     {
-        PROS_VACB Vacb = CONTAINING_RECORD(current_entry, ROS_VACB, CacheMapVacbListEntry);
+        PROS_VACB Vacb = CONTAINING_RECORD(SharedCacheMap->CacheMapVacbListHead.Flink,
+                                           ROS_VACB, CacheMapVacbListEntry);
 
         RemoveEntryList(&Vacb->VacbLruListEntry);
         InitializeListHead(&Vacb->VacbLruListEntry);
@@ -258,7 +269,13 @@ CcRosDeleteFileCache (
             Vacb->Dirty = TRUE;
         }
 
-        current_entry = current_entry->Flink;
+        /*
+         * Move this VACB from the shared cache map list to our private local
+         * list. After the lock is released, CcRosLookupVacb can no longer find
+         * it, so no new lookup references can be created.
+         */
+        RemoveEntryList(&Vacb->CacheMapVacbListEntry);
+        InsertTailList(&LocalVacbList, &Vacb->CacheMapVacbListEntry);
     }
 
     /* Make sure there is no trace anymore of this map */
@@ -269,9 +286,9 @@ CcRosDeleteFileCache (
     KeReleaseQueuedSpinLock(LockQueueMasterLock, *OldIrql);
 
     /* Now that we're out of the locks, free everything for real */
-    while (!IsListEmpty(&SharedCacheMap->CacheMapVacbListHead))
+    while (!IsListEmpty(&LocalVacbList))
     {
-        PROS_VACB Vacb = CONTAINING_RECORD(RemoveHeadList(&SharedCacheMap->CacheMapVacbListHead), ROS_VACB, CacheMapVacbListEntry);
+        PROS_VACB Vacb = CONTAINING_RECORD(RemoveHeadList(&LocalVacbList), ROS_VACB, CacheMapVacbListEntry);
         ULONG RefCount;
 
         InitializeListHead(&Vacb->CacheMapVacbListEntry);
@@ -616,7 +633,6 @@ CcRosReleaseVacb (
     BOOLEAN Dirty,
     BOOLEAN Mapped)
 {
-    ULONG Refs;
     ASSERT(SharedCacheMap);
 
     DPRINT("CcRosReleaseVacb(SharedCacheMap 0x%p, Vacb 0x%p)\n", SharedCacheMap, Vacb);
@@ -647,8 +663,15 @@ CcRosReleaseVacb (
         }
     }
 
-    Refs = CcRosVacbDecRefCount(Vacb);
-    ASSERT(Refs > 0);
+    /*
+     * Release the caller's reference. The return value may legitimately be 0
+     * when CcRosDeleteFileCache has concurrently removed the cache-map-list
+     * reference while we held a lookup reference: our decrement is then the
+     * last one and CcRosVacbDecRefCount frees the VACB. That is safe because
+     * callers (e.g. CcFlushCache) do not access the VACB pointer after this
+     * call. Do NOT assert the result is > 0 here.
+     */
+    CcRosVacbDecRefCount(Vacb);
 
     return STATUS_SUCCESS;
 }