[reactos] 01/01: [NTOS] Properly implement and use FsRtlAcquireFileForModWriteEx

[Jérôme Gardou]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=2ae9feb59f8adbec05ca0…

commit 2ae9feb59f8adbec05ca0ea9e556c1c990d8f7c2
Author:     Jérôme Gardou <jerome.gardou(a)reactos.org>
AuthorDate: Mon Sep 5 19:33:19 2022 +0200
Commit:     Jérôme Gardou <zefklop(a)users.noreply.github.com>
CommitDate: Wed Nov 2 19:41:04 2022 +0100

    [NTOS] Properly implement and use FsRtlAcquireFileForModWriteEx
---
 ntoskrnl/fsrtl/fastio.c           | 203 +++++++++++++++++++-------------------
 ntoskrnl/include/internal/fsrtl.h |  12 +++
 ntoskrnl/mm/section.c             |  49 ++++++++-
 3 files changed, 160 insertions(+), 104 deletions(-)

diff --git a/ntoskrnl/fsrtl/fastio.c b/ntoskrnl/fsrtl/fastio.c
index ce629f77a02..8e19b2d77a9 100644
--- a/ntoskrnl/fsrtl/fastio.c
+++ b/ntoskrnl/fsrtl/fastio.c
@@ -1795,20 +1795,69 @@ FsRtlReleaseFileForCcFlush(IN PFILE_OBJECT FileObject)
     FsRtlExitFileSystem();
 }
 
-/*
-* @implemented
-*/
+/**
+ * @brief Get the resource to acquire when Mod Writer flushes data to disk
+ *
+ * @param FcbHeader - FCB header from the file object
+ * @param EndingOffset - The end offset of the write to be done
+ * @param ResourceToAcquire - Pointer receiving the resource to acquire before doing the
write
+ *
+ * @return BOOLEAN specifying whether the resource must be acquired exclusively
+ */
+static
+BOOLEAN
+FsRtlpGetResourceForModWrite(_In_ PFSRTL_COMMON_FCB_HEADER FcbHeader,
+                             _In_ PLARGE_INTEGER EndingOffset,
+                             _Outptr_result_maybenull_ PERESOURCE* ResourceToAcquire)
+{
+    /*
+     * Decide on type of locking and type of resource based on
+     *  - Flags
+     *  - Whether we're extending ValidDataLength
+     */
+    if (FlagOn(FcbHeader->Flags, FSRTL_FLAG_ACQUIRE_MAIN_RSRC_EX))
+    {
+        /* Acquire main resource, exclusive */
+        *ResourceToAcquire = FcbHeader->Resource;
+        return TRUE;
+    }
+
+    /* We will acquire shared. Which one ? */
+    if (FlagOn(FcbHeader->Flags, FSRTL_FLAG_ACQUIRE_MAIN_RSRC_SH))
+    {
+        *ResourceToAcquire = FcbHeader->Resource;
+    }
+    else
+    {
+        *ResourceToAcquire = FcbHeader->PagingIoResource;
+    }
+
+    /* We force exclusive lock if this write modifies the valid data length */
+    return (EndingOffset->QuadPart > FcbHeader->ValidDataLength.QuadPart);
+}
+
+/**
+ * @brief Lock a file object before flushing pages to disk. 
+ *        To be called by the Modified Page Writer (MPW)
+ *
+ * @param FileObject - The file object to lock
+ * @param EndingOffset - The end offset of the write to be done
+ * @param ResourceToRelease - Pointer receiving the resource to release after the write
+ *
+ * @return Relevant NTSTATUS value
+ */
+_Check_return_
 NTSTATUS
 NTAPI
-FsRtlAcquireFileForModWriteEx(IN PFILE_OBJECT FileObject,
-                              IN PLARGE_INTEGER EndingOffset,
-                              IN PERESOURCE *ResourceToRelease)
+FsRtlAcquireFileForModWriteEx(_In_ PFILE_OBJECT FileObject,
+                              _In_ PLARGE_INTEGER EndingOffset,
+                              _Outptr_result_maybenull_ PERESOURCE *ResourceToRelease)
 {
     PFSRTL_COMMON_FCB_HEADER FcbHeader;
     PDEVICE_OBJECT DeviceObject, BaseDeviceObject;
     PFAST_IO_DISPATCH FastDispatch;
     PERESOURCE ResourceToAcquire = NULL;
-    BOOLEAN Exclusive = FALSE;
+    BOOLEAN Exclusive;
     BOOLEAN Result;
     NTSTATUS Status = STATUS_SUCCESS;
 
@@ -1837,133 +1886,89 @@ FsRtlAcquireFileForModWriteEx(IN PFILE_OBJECT FileObject,
         }
     }
 
-    Status = STATUS_SUCCESS;
-
-    /* No FastIo handler, use algorithm from Nagar p.550. */
-    if (!FcbHeader->Resource)
-    {
-        *ResourceToRelease = NULL;
-        return STATUS_SUCCESS;
-    }
-
-    /* Default condition - shared acquiring of Paging IO Resource */
-    ResourceToAcquire = FcbHeader->PagingIoResource;
+    /* Check what and how we should acquire */
+    Exclusive = FsRtlpGetResourceForModWrite(FcbHeader, EndingOffset,
&ResourceToAcquire);
 
-    /* Decide on type of locking and type of resource based on historical magic
-       well explain by Nagar in p. 550-551 */
-    if ((EndingOffset->QuadPart > FcbHeader->ValidDataLength.QuadPart
&&
-         FcbHeader->FileSize.QuadPart != FcbHeader->ValidDataLength.QuadPart) ||
-         (FcbHeader->Flags & FSRTL_FLAG_ACQUIRE_MAIN_RSRC_EX))
-    {
-        /* Either exclusive flag is set or write operation is extending
-           the valid data length. Prefer exclusive acquire then */
-        Exclusive = TRUE;
-        ResourceToAcquire = FcbHeader->Resource;
-    }
-    else if (!FcbHeader->PagingIoResource ||
-             (FcbHeader->Flags & FSRTL_FLAG_ACQUIRE_MAIN_RSRC_SH))
-    {
-        /* Acquire main resource shared if flag is specified or
-           if PagingIo resource is missing */
-        Exclusive = FALSE;
-        ResourceToAcquire = FcbHeader->Resource;
-    }
-
-    /* Acquire the resource in the loop, since the above code is unsafe */
+    /* Acquire the resource and loop until we're sure we got this right. */
     while (TRUE)
     {
-        Result = FALSE;
+        BOOLEAN OldExclusive;
+        PERESOURCE OldResourceToAcquire;
 
-        if (Exclusive)
-            Result = ExAcquireResourceExclusiveLite(ResourceToAcquire, FALSE);
-        else
-            Result = ExAcquireSharedWaitForExclusive(ResourceToAcquire, FALSE);
+        if (ResourceToAcquire == NULL)
+        {
+            /* 
+             * There's nothing to acquire, we can simply return success
+             */
 
-        if (!Result) {
-            Status = STATUS_CANT_WAIT;
             break;
         }
 
-        /* Do the magic ifs again */
-        if ((EndingOffset->QuadPart > FcbHeader->ValidDataLength.QuadPart) ||
-             (FcbHeader->Flags & FSRTL_FLAG_ACQUIRE_MAIN_RSRC_EX))
+        if (Exclusive)
         {
-            /* Check what we have */
-            if (Exclusive)
-            {
-                /* Asked for exclusive, got exclusive! */
-                break;
-            }
-            else
-            {
-                /* Asked for exclusive, got shared. Release it and retry. */
-                ExReleaseResourceLite(ResourceToAcquire);
-                Exclusive = TRUE;
-                ResourceToAcquire = FcbHeader->Resource;
-            }
+            Result = ExAcquireResourceExclusiveLite(ResourceToAcquire, FALSE);
         }
-        else if (FcbHeader->Flags & FSRTL_FLAG_ACQUIRE_MAIN_RSRC_SH)
+        else
         {
-            if (Exclusive)
-            {
-                /* Asked for shared, got exclusive - convert */
-                ExConvertExclusiveToSharedLite(ResourceToAcquire);
-                break;
-            }
-            else if (ResourceToAcquire != FcbHeader->Resource)
-            {
-                /* Asked for main resource, got something else */
-                ExReleaseResourceLite(ResourceToAcquire);
-                ResourceToAcquire = FcbHeader->Resource;
-                Exclusive = TRUE;
-            }
+            Result = ExAcquireSharedWaitForExclusive(ResourceToAcquire, FALSE);
         }
-        else if (FcbHeader->PagingIoResource &&
-                 ResourceToAcquire != FcbHeader->PagingIoResource)
+
+        if (!Result)
         {
-            /* There is PagingIo resource, but other resource was acquired */
-            ResourceToAcquire = FcbHeader->PagingIoResource;
-            if (!ExAcquireSharedWaitForExclusive(ResourceToAcquire, FALSE))
-            {
-                Status = STATUS_CANT_WAIT;
-                ExReleaseResourceLite(FcbHeader->Resource);
-            }
+            return STATUS_CANT_WAIT;
+        }
+
+        /* Does this still hold true? */
+        OldExclusive = Exclusive;
+        OldResourceToAcquire = ResourceToAcquire;
+        Exclusive = FsRtlpGetResourceForModWrite(FcbHeader, EndingOffset,
&ResourceToAcquire);
 
+        if ((OldExclusive == Exclusive) && (OldResourceToAcquire ==
ResourceToAcquire))
+        {
+            /* We're good */
             break;
         }
-        else if (Exclusive)
+
+        /* Can we fix this situation? */
+        if ((OldResourceToAcquire == ResourceToAcquire) && !Exclusive)
         {
-            /* Asked for shared got exclusive - convert */
+            /* We can easily do so */
             ExConvertExclusiveToSharedLite(ResourceToAcquire);
             break;
         }
-    }
 
-    /* If the resource was acquired successfully - pass it to the caller */
-    if (NT_SUCCESS(Status))
-        *ResourceToRelease = ResourceToAcquire;
+        /* Things have changed since we acquired the lock. Start again */
+        ExReleaseResourceLite(OldResourceToAcquire);
+    }
 
-    return Status;
+    /* If we're here, this means that we succeeded */
+    *ResourceToRelease = ResourceToAcquire;
+    return STATUS_SUCCESS;
 }
 
-/*
-* @implemented
-*/
+/**
+ * @brief Unlock a file object after flushing pages to disk. 
+ *        To be called by the Modified Page Writer (MPW) after a succesful call to
+ *        FsRtlAcquireFileForModWriteEx
+ *
+ * @param FileObject - The file object to unlock
+ * @param ResourceToRelease - The resource to release
+ */
 VOID
 NTAPI
-FsRtlReleaseFileForModWrite(IN PFILE_OBJECT FileObject,
-                            IN PERESOURCE ResourceToRelease)
+FsRtlReleaseFileForModWrite(_In_ PFILE_OBJECT FileObject,
+                            _In_ PERESOURCE ResourceToRelease)
 {
     PDEVICE_OBJECT DeviceObject, BaseDeviceObject;
     PFAST_IO_DISPATCH FastDispatch;
-    NTSTATUS Status = STATUS_SUCCESS;
+    NTSTATUS Status = STATUS_UNSUCCESSFUL;
 
     /* Get Device Object and Fast Calls */
     DeviceObject = IoGetRelatedDeviceObject(FileObject);
     BaseDeviceObject = IoGetBaseFileSystemDeviceObject(FileObject);
     FastDispatch = DeviceObject->DriverObject->FastIoDispatch;
 
-    /* Check if Fast Calls are supported and check ReleaseFileForNtCreateSection */
+    /* Check if Fast Calls are supported and check ReleaseForModWrite */
     if (FastDispatch &&
         FastDispatch->ReleaseForModWrite)
     {
diff --git a/ntoskrnl/include/internal/fsrtl.h b/ntoskrnl/include/internal/fsrtl.h
index 5e106c38c1b..01425de1c56 100644
--- a/ntoskrnl/include/internal/fsrtl.h
+++ b/ntoskrnl/include/internal/fsrtl.h
@@ -159,3 +159,15 @@ FsRtlReleaseFileForCcFlush(IN PFILE_OBJECT FileObject);
 NTSTATUS
 NTAPI
 FsRtlAcquireFileForCcFlushEx(IN PFILE_OBJECT FileObject);
+
+_Check_return_
+NTSTATUS
+NTAPI
+FsRtlAcquireFileForModWriteEx(_In_ PFILE_OBJECT FileObject,
+                              _In_ PLARGE_INTEGER EndingOffset,
+                              _Outptr_result_maybenull_ PERESOURCE *ResourceToRelease);
+
+VOID
+NTAPI
+FsRtlReleaseFileForModWrite(IN PFILE_OBJECT FileObject,
+                            IN PERESOURCE ResourceToRelease);
diff --git a/ntoskrnl/mm/section.c b/ntoskrnl/mm/section.c
index 21a9fc75560..61930738642 100644
--- a/ntoskrnl/mm/section.c
+++ b/ntoskrnl/mm/section.c
@@ -4965,17 +4965,56 @@ MmCheckDirtySegment(
 
         if (FlagOn(*Segment->Flags, MM_DATAFILE_SEGMENT))
         {
+            PERESOURCE ResourceToRelease = NULL;
+            KIRQL OldIrql;
+
             /* We have to write it back to the file. Tell the FS driver who we are */
             if (PageOut)
-                IoSetTopLevelIrp((PIRP)FSRTL_MOD_WRITE_TOP_LEVEL_IRP);
+            {
+                LARGE_INTEGER EndOffset = *Offset;
 
-            /* Go ahead and write the page */
-            DPRINT("Writing page at offset %I64d for file %wZ, Pageout: %s\n",
-                    Offset->QuadPart, &Segment->FileObject->FileName,
PageOut ? "TRUE" : "FALSE");
-            Status = MiWritePage(Segment, Offset->QuadPart, Page);
+                ASSERT(IoGetTopLevelIrp() == NULL);
+
+                /* We need to disable all APCs */
+                KeRaiseIrql(APC_LEVEL, &OldIrql);
+
+                EndOffset.QuadPart += PAGE_SIZE;
+                Status = FsRtlAcquireFileForModWriteEx(Segment->FileObject,
+                                                       &EndOffset,
+                                                       &ResourceToRelease);
+                if (NT_SUCCESS(Status))
+                {
+                    IoSetTopLevelIrp((PIRP)FSRTL_MOD_WRITE_TOP_LEVEL_IRP);
+                }
+                else
+                {
+                    /* Make sure we will not try to release anything */
+                    ResourceToRelease = NULL;
+                }
+            }
+            else
+            {
+                /* We don't have to lock. Say this is success */
+                Status = STATUS_SUCCESS;
+            }
+
+            /* Go ahead and write the page, if previous locking succeeded */
+            if (NT_SUCCESS(Status))
+            {
+                DPRINT("Writing page at offset %I64d for file %wZ, Pageout:
%s\n",
+                        Offset->QuadPart, &Segment->FileObject->FileName,
PageOut ? "TRUE" : "FALSE");
+                Status = MiWritePage(Segment, Offset->QuadPart, Page);
+            }
 
             if (PageOut)
+            {
                 IoSetTopLevelIrp(NULL);
+                if (ResourceToRelease != NULL)
+                {
+                    FsRtlReleaseFileForModWrite(Segment->FileObject,
ResourceToRelease);
+                }
+                KeLowerIrql(OldIrql);
+            }
         }
         else
         {