Fix locking of Working set in various places by zefklop · Pull Request #3798 · reactos/reactos

zefklop · 2021-07-05T08:43:11Z

Purpose

Avoid some race conditions, non-serialized read & writes on the VAD tree, etc.

Proposed changes

Get rid of old MmAccessFault, use MmArm3Accessfault instead (renamed to MmAccessFault)
From MmAccessFault, handle Working Set locking
- Do not lock when not coming from a trap
- Fix callers accordingly
From MmAccessFault, dispatch to old Mm if needed
Misc related fixes

modules/CMakeLists.txt

JoachimHenze · 2021-07-23T19:48:00Z

zefklop, you mentioned this PR in https://jira.reactos.org/browse/CORE-17698 . So it sounds in JIRA as if it would fix that. Understood right?
Why don't you add https://jira.reactos.org/browse/CORE-17698 and https://jira.reactos.org/browse/CORE-17690 to the JIRA issues it addresses in the PRs description then?

JoachimHenze · 2021-07-23T21:45:25Z

With your PR I can not longer reproduce https://jira.reactos.org/browse/CORE-17690 see https://jira.reactos.org/browse/CORE-17690?focusedCommentId=129089&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-129089 for a successful log with the patch.
I guess you should mention that ticket in the description also! It happifies me for CORE-17690.

JoachimHenze · 2021-07-23T21:52:02Z

But this ticket does NOT fix CORE-17595 as the tickets description currently implies. I retested with the artifacts iso. So please remove that JIRA-ID from the PRs description! It's a false promise!
https://jira.reactos.org/browse/CORE-17595?focusedCommentId=129090&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-129090

ntoskrnl/include/internal/mm.h

ntoskrnl/mm/ARM3/mdlsup.c

ntoskrnl/mm/i386/page.c

ntoskrnl/mm/marea.c

ntoskrnl/mm/section.c

ntoskrnl/mm/ARM3/pagfault.c

ntoskrnl/mm/section.c

ntoskrnl/mm/ARM3/pagfault.c

ntoskrnl/mm/section.c

ntoskrnl/mm/ARM3/virtual.c

ntoskrnl/mm/i386/page.c

tkreuzer · 2021-07-29T13:42:19Z

ntoskrnl/mm/ARM3/miarm.h

+    _In_ PEPROCESS CurrentProcess)
+{
+    BOOLEAN ret = _MiMakeSystemAddressValid(PageTableVirtualAddress, CurrentProcess);
+    KeMemoryBarrierWithoutFence();


Why would this be needed? A function call is a sequence point and everything (as visible to the current thread) is evaluated before the return. If the concern is, that this function is inlined on release builds and the write to the PTE is reordered to after access to the page, then we should rather add a memory barrier to the MI_WRITE_VALID_PTE and MI_UPDATE_VALID_PTE functions.

Correction: In standard C, there is a sequence point between the return and the start of the next full expression, so MiMakeSystemAddressValid(PageTableVirtualAddress, CurrentProcess) + *(PUCHAR)PageTableVirtualAddress wouldn't have a sequence point, but hopefully nobody would write that code :).

It's not about "being evaluated before the return", it's about compiler optimizing memory accesses before or after the call to such functions. Only volatiles are guaranteed to be read or written between sequence points.
I believe that an optimizing compiler can make the following actually access *PointerPte before the call to MiMakeSystemAddressValid and this leads to problems:

PMMPTE PointerPte = MiAddressToPte(addr); MiMakeSystemAddressValid(DifferentPointerPteWithinPDRange); MMPTE TempPte =*PointerPte;

In that case all code that maps any PTE is prone to this. Therefore I suggest adding the memory barrier to the end of MI_WRITE_VALID_P*E and MI_UPDATE_VALID_P*E, as well as to the beginning of MI_WRITE_INVALID_PTE. This would "propagate" to any place it's needed, as long as these functions are used.

LTO may inline all the functions and start reordering code which once was in different ones.
Two questions: why not putting the barrier inside the function, and shouldn't here be something more hard than a compiler barrier? If that's important, how can we be sure CPU won't reorder stuff?

why not putting the barrier inside the function ?

The compiler won't see it on caller side if it's inside the function / in a different compilation unit.

If that's important, how can we be sure CPU won't reorder stuff?

We can't. But the CPU won't trigger a PF until it's "sure" it has to, and we would have the instructions actually making the PT valid in the pipeline.

The CPU doesn't reorder reads and writes on the same CPU core/thread (exception is speculative execution and that doesn't cause faults).
If it's in a different compilation unit and not inlined, then the compiler cannot move memory accesses around it, because it doesn't know the side effects of that function. Otherwise no locking function would ever work without an explicit memory barrier after it.
So the only reason to add that memory barrier is to prevent the compiler from shuffling things around, when it inlines the function and (wrongly) determines that there is no dependency between them . So adding the memory barrier in the function does exactly what is needed.

The CPU doesn't reorder reads and writes on the same CPU core/thread

As far as I understand, this is true only for x86. Not sure if that's important for us right now though.

ntoskrnl/mm/ARM3/virtual.c

Extravert-ir · 2021-08-02T00:09:27Z

ntoskrnl/mm/ARM3/mdlsup.c

-
-                        //HACK: Pass a placeholder TrapInformation so the fault handler knows we're unlocked
-                        Status = MmAccessFault(TRUE, Address, KernelMode, (PVOID)(ULONG_PTR)0xBADBADA3BADBADA3ULL);
+                        Status = MmAccessFault(TRUE, Address, KernelMode, NULL);


Stupid remark, but you pass TRUE/FALSE values to an ULONG field.
Shouldn't we define a set of flags somewhere, because currently I see only a set of MI_IS_* set of macroses which use raw constants inside.

Yes, I have to make up some names.

Extravert-ir · 2021-08-02T00:13:51Z

ntoskrnl/mm/ARM3/miarm.h

+    _In_ PEPROCESS CurrentProcess)
+{
+    BOOLEAN ret = _MiMakeSystemAddressValid(PageTableVirtualAddress, CurrentProcess);
+    KeMemoryBarrierWithoutFence();


LTO may inline all the functions and start reordering code which once was in different ones.
Two questions: why not putting the barrier inside the function, and shouldn't here be something more hard than a compiler barrier? If that's important, how can we be sure CPU won't reorder stuff?

ntoskrnl/mm/ARM3/miarm.h

ntoskrnl/mm/ARM3/pagfault.c

ntoskrnl/mm/ARM3/mmsup.c

ntoskrnl/mm/ARM3/pagfault.c

Extravert-ir · 2021-08-05T02:19:22Z

ntoskrnl/mm/ARM3/miarm.h

+    KeMemoryBarrierWithoutFence();
+    ret = PointerPte->u.Hard.Valid;
+    KeMemoryBarrierWithoutFence();


Looks like we're trying to emulate atomic_load() from C11 here

Doesn't atomic_load involve some specific CPU instruction ? Here the Memory Barrier is for compiler only.

On x86, it doesn't do anything special except being a compiler barrier
https://godbolt.org/z/h6Yo7Y4z5

zefklop · 2021-08-05T07:58:52Z

For those trying to figure out what this memory barrier stuff is about:
https://gcc.godbolt.org/z/16qejTcGn

Adapted from a code sample from @tkreuzer

JoachimHenze · 2021-08-06T03:20:41Z

[~zefklop] I downloaded the latest artifacts again today from #3798 after the most recent changes. It was the gcc 386 dbg build, which identified itself as
ReactOS 0.4.15-x86-dev (Build 20210805-57cd3d8) (Commit 57cd3d8)
(x) and even that did fail with the strong-name bug at 2nd try.
(x) #3798 does still not fix CORE-17595.

And CORE-17642 also has been reported to be fixed already in master head.

In sum this means that this PR does currently fix no known JIRA-ticket!
I don't want to say, that it is wrong therefore, but we should not give any false promises or keep wrong relations if we ultimately intend to commit something from within here!

Extravert-ir · 2021-08-08T12:05:39Z

Can't we make MMPTE or some parts of it volatile? That should eliminate the need in explicit compiler barriers

Extravert-ir · 2021-08-15T00:08:17Z

I've got a problem while testing this PR inside a clang-cl build. It triggers an IRQL check in MiLockWorkingSet. Here is a stack trace:

kd> kp
 # ChildEBP RetAddr  
00 (Inline) -------- nt!MiLockWorkingSet+0x4d [C:\rosgit\ntoskrnl\mm\ARM3\miarm.h @ 1315] 
01 f2741e28 804a467a nt!MmProbeAndLockPages(struct _MDL * Mdl = 0xf30c4f88, char AccessMode = 0n0 '', _LOCK_OPERATION Operation = IoReadAccess (0))+0x826 [C:\rosgit\ntoskrnl\mm\ARM3\mdlsup.c @ 1179] 
02 f2741e88 f7a4c9f9 nt!IoBuildAsynchronousFsdRequest(unsigned long MajorFunction = 3, struct _DEVICE_OBJECT * DeviceObject = 0xf2f52a08 Device for "\Driver\UniATA", void * Buffer = 0xf3072fe8, unsigned long Length = 0x12, union _LARGE_INTEGER * StartingOffset = 0xf2741eb0 {0x1}, struct _IO_STATUS_BLOCK * IoStatusBlock = 0x00000000)+0x25a [C:\rosgit\ntoskrnl\io\iomgr\irp.c @ 824] 
03 f2741ef0 f7a4becd scsiport!SpiSendRequestSense(struct _SCSI_PORT_LUN_EXTENSION * LunExtension = 0xf2f52ac0, struct _SCSI_REQUEST_BLOCK * InitialSrb = 0xf2f74fc0)+0xf9 [C:\rosgit\drivers\storage\port\scsiport\scsi.c @ 555] 
04 f2741f30 f7a4b44e scsiport!SpiProcessCompletedRequest(struct _SCSI_PORT_DEVICE_EXTENSION * DeviceExtension = 0xf2d0cb30, struct _SCSI_REQUEST_BLOCK_INFO * SrbInfo = 0x00000000, unsigned char * NeedToCallStartIo = 0xf2741f57 "")+0x89d [C:\rosgit\drivers\storage\port\scsiport\scsi.c @ 951] 
05 f2741f80 804d0bf1 scsiport!ScsiPortDpcForIsr(struct _KDPC * Dpc = 0xf2d0caec, struct _DEVICE_OBJECT * DpcDeviceObject = 0xf2d0ca78 Device for "\Driver\UniATA", struct _IRP * DpcIrp = 0x00000000, void * DpcContext = 0xf2d0cb30)+0x32e [C:\rosgit\drivers\storage\port\scsiport\scsi.c @ 1381] 
06 f2741ff4 80645e7f nt!KiRetireDpcList(struct _KPRCB * Prcb = 0xffdff120)+0x1c1 [C:\rosgit\ntoskrnl\ke\dpc.c @ 633] 
07 f2741ff8 f26f5740 nt!KiRetireDpcListInDpcStack+0xa

So we're in scsiport's DpcForIsr handler, so the IRQL is DISPATCH_LEVEL. It calls IoBuildAsynchronousFsdRequest, which calls MmProbeAndLockPages with a MDL address (which itself is nonpaged), so the usage of MmProbeAndLockPages at DISPATCH_LEVEL should be fine at this case.
What's wrong then, and why it is not triggered on MSVC?

Extravert-ir

Ok, I've got a bit further. Inside MmProbeAndLockPages, this code path goes wrong way:

/* Check how we should lock */
if (MI_IS_SESSION_ADDRESS(Base))
{
    WorkingSet = &MmSessionSpace->GlobalVirtualAddress->Vm;
}
else if (MI_IS_NON_PAGED_POOL_ADDRESS(Base))
{
    UsePfnLock = TRUE;
    OldIrql = MiAcquirePfnLock();
}
else
{
    WorkingSet = &MmSystemCacheWs;
}

In this case, Base address is the one which comes from IoAllocateMdl. It was a small allocation, so it was taken from a lookaside buffer (LookasideMdlList):

reactos/ntoskrnl/io/iomgr/iomdl.c

Lines 53 to 54 in 911fc3c

    
           /* Allocate one from the lookaside list */ 
        
           Mdl = IopAllocateMdlFromLookaside(LookasideMdlList);

MI_IS_NON_PAGED_POOL_ADDRESS(Base) returns FALSE for such address, looks like this is what's wrong here
The address in my case is 0xf2e95fe8 which (according to table) belongs to 0xEB000000 - 0xF7BE0000 System PTE Space

zefklop · 2021-08-16T17:03:41Z

Ok, I've got a bit further. Inside MmProbeAndLockPages, this code path goes wrong way:
/* Check how we should lock */
if (MI_IS_SESSION_ADDRESS(Base))
{
    WorkingSet = &MmSessionSpace->GlobalVirtualAddress->Vm;
}
else if (MI_IS_NON_PAGED_POOL_ADDRESS(Base))
{
    UsePfnLock = TRUE;
    OldIrql = MiAcquirePfnLock();
}
else
{
    WorkingSet = &MmSystemCacheWs;
}
In this case, Base address is the one which comes from IoAllocateMdl. It was a small allocation, so it was taken from a lookaside buffer (LookasideMdlList):

reactos/ntoskrnl/io/iomgr/iomdl.c

Lines 53 to 54 in 911fc3c

/* Allocate one from the lookaside list */

Mdl = IopAllocateMdlFromLookaside(LookasideMdlList);

Base is the buffer passed to IoBuildAsynchronousFsdRequest. Why should that be in system PTE space ?

MI_IS_NON_PAGED_POOL_ADDRESS(Base) returns FALSE for such address, looks like this is what's wrong here
The address in my case is 0xf2e95fe8 which (according to table) belongs to 0xEB000000 - 0xF7BE0000 System PTE Space

Thanks for the analysis. Indeed, this should take PTE space into account, I'll see how to correct this.

Extravert-ir · 2021-08-17T21:32:23Z

Sorry, I was wrong at some things, don't know how had I overlooked that :)

It is reproducible with both MSVC and Clang when Special Pool is enabled. Sorry for a confusion. (I sometimes enable it without taking much attention). I guess for GCC it will trigger that too (can't check right now).

Base is the buffer passed to IoBuildAsynchronousFsdRequest. Why should that be in system PTE space ?

I've found the allocation actually, it comes from cdrom. Nothing interesting:

reactos/drivers/storage/class/cdrom/scratch.c

Lines 261 to 263 in db8dd3b

    
           DeviceExtension->ScratchContext.ScratchSense = ExAllocatePoolWithTag(NonPagedPoolNx, 
        
                                                                                sizeof(SENSE_DATA), 
        
                                                                                CDROM_TAG_SCRATCH);

What's interesting to me is that according to log, special pool has this range:

(ntoskrnl\mm\ARM3\special.c:196) Special pool start F272B000 - end F2B2A000

But the address allocated is higher than that area - 0xf300dfe8. Is it a mistake in DPRINT?

binarymaster · 2022-01-21T22:04:54Z

The branch zefklop:CORE-17595 got synced with ROS master, but without PR commits applied on top, and this led to PR close... perhaps by accident? 👀

zefklop requested review from HeisSpiter, ThFabba and tkreuzer as code owners July 5, 2021 08:43

binarymaster added the bugfix For bugfix PRs. label Jul 5, 2021

github-actions bot added drivers Kernel mode drivers and frameworks kernel&hal Code changes to the ntoskrnl and HAL labels Jul 5, 2021

zefklop force-pushed the CORE-17595 branch from 23fd8c4 to a746344 Compare July 5, 2021 08:56

github-actions bot removed the drivers Kernel mode drivers and frameworks label Jul 5, 2021

zefklop force-pushed the CORE-17595 branch 3 times, most recently from 9c8668e to ada8b0b Compare July 5, 2021 13:29

Doug-Lyons reviewed Jul 6, 2021

View reviewed changes

modules/CMakeLists.txt Show resolved Hide resolved

zefklop force-pushed the CORE-17595 branch 4 times, most recently from cf7e4e1 to 95f2a3b Compare July 23, 2021 16:36

zefklop force-pushed the CORE-17595 branch 2 times, most recently from ac7ac64 to b32d2f0 Compare July 26, 2021 10:09

tkreuzer reviewed Jul 26, 2021

View reviewed changes

ntoskrnl/include/internal/mm.h Show resolved Hide resolved

HBelusca reviewed Jul 26, 2021

View reviewed changes

zefklop mentioned this pull request Jul 26, 2021

Pfn lock fix #3850

Merged

zefklop force-pushed the CORE-17595 branch from b32d2f0 to 2577012 Compare July 26, 2021 16:05

tkreuzer reviewed Jul 26, 2021

View reviewed changes

zefklop force-pushed the CORE-17595 branch 4 times, most recently from c392406 to 2e985a8 Compare July 27, 2021 13:37

tkreuzer reviewed Jul 29, 2021

View reviewed changes

Extravert-ir requested changes Aug 2, 2021

View reviewed changes

zefklop force-pushed the CORE-17595 branch 4 times, most recently from 9dcc76e to a238b9b Compare August 4, 2021 10:11

tkreuzer reviewed Aug 4, 2021

View reviewed changes

zefklop force-pushed the CORE-17595 branch 2 times, most recently from 81eb350 to 5da1c51 Compare August 4, 2021 15:05

zefklop added the refactoring For refactoring changes. label Aug 4, 2021

zefklop self-assigned this Aug 4, 2021

zefklop force-pushed the CORE-17595 branch 2 times, most recently from 1f6a492 to ae7e0a6 Compare August 4, 2021 17:04

tkreuzer reviewed Aug 4, 2021

View reviewed changes

ntoskrnl/mm/ARM3/mmsup.c Outdated Show resolved Hide resolved

tkreuzer reviewed Aug 4, 2021

View reviewed changes

ntoskrnl/mm/ARM3/pagfault.c Outdated Show resolved Hide resolved

Extravert-ir reviewed Aug 5, 2021

View reviewed changes

zefklop force-pushed the CORE-17595 branch from ae7e0a6 to 93da934 Compare August 5, 2021 08:08

Extravert-ir approved these changes Aug 8, 2021

View reviewed changes

Extravert-ir requested changes Aug 15, 2021

View reviewed changes

komyojgkkg mentioned this pull request Nov 29, 2021

As far as I know, yes. Let's see what CI says. komyojgkkg/fluffy-umbrella#1

Open

zefklop closed this Jan 21, 2022

zefklop force-pushed the CORE-17595 branch from 02306ef to 41b8715 Compare January 21, 2022 21:30

github-actions bot removed the kernel&hal Code changes to the ntoskrnl and HAL label Jan 21, 2022

	/* Allocate one from the lookaside list */
	Mdl = IopAllocateMdlFromLookaside(LookasideMdlList);

Uh oh!

Conversation

zefklop commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Proposed changes

Uh oh!

Uh oh!

JoachimHenze commented Jul 23, 2021

Uh oh!

JoachimHenze commented Jul 23, 2021

Uh oh!

JoachimHenze commented Jul 23, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zefklop Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkreuzer Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Extravert-ir Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zefklop commented Jul 5, 2021 •

edited

Loading

zefklop Jul 29, 2021 •

edited

Loading

tkreuzer Jul 30, 2021 •

edited

Loading

Extravert-ir Aug 5, 2021 •

edited

Loading

zefklop commented Aug 5, 2021 •

edited

Loading

Extravert-ir left a comment •

edited

Loading

zefklop commented Aug 16, 2021 •

edited

Loading