Repository navigation
Conversation
mashingaan
left a comment
There was a problem hiding this comment.
I found a pin-safety regression in the split protection path. The existing single-range path gathers every registered span and calls RequireUnpinned on all of them before the first protection change. Here each part is sent through ProtectGuestRange separately, and each recursive call checks and then protects its own spans. If a later part beyond a host-stack gap is pinned, the call can fail after an earlier part has already changed protection. Please preflight all surviving spans across every part before applying any protection, and add a focused regression for a pinned later span.
The affected call is core/libs/prx/libkernel/DirectMemory/DirectMemory.cpp:1463 at head 4c52e758e0b9f2f6c39833871cc1bb6d4bdab525.
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
4c52e75 to
e8bd827
Compare
|
Thanks, you were right. Fixed in 0a8f377: the general-path parts now share one registry mutation in The regression test is e8bd827 ( |
|
@Ragbie I have a separate flexible-memory mtypeprotect safety fix ready that conflicts with your change in docs/dev/debt/libkernel.md. Would you prefer #15040 to merge first so I can rebase mine afterward? My branch is https://github.com/nikhilpanchal1/AnyPS5/tree/fix/kernel-mtypeprotect-flexible |
✅ AMD GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (7, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (4 IDs in 624 tests)Skipped (59)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
…return 0 on Linux
…in mprotect on Linux
…er one in mprotect
e8bd827 to
f79dccf
Compare
|
@nikhilpanchal1 Yes, #15040 first, please; record Rebased onto AI-assisted: yes (Claude Code) |
What
On Linux,
mprotectandsceKernelMprotectover the host-only pages around a guest thread's stack now return 0 and leave those pages unchanged. Before, they returned -1 withEACCES. ThesceKernelMprotectdebt entry recorded the gap ("On Linux a range over the host-only pages around a guest thread's stack still fails withEACCES, where Windows returns 0"), and the #9882 discussion left the Linux return of the straddling range as a follow-up.The same debt entry records the PS5 (firmware 13.60) measurement:
mprotectandsceKernelMprotectchange the mapped parts of a range, skip its gaps and return 0. On FreeBSD 11.4, libthr places the stack guard in the stack's own mapping (thr_stack.c), so on the console the pages around a thread stack are guest memory.On Linux a guest thread's stack is a guest allocation (
AllocateGuestStack) that the host thread runs on. The guard, the spare host stack below the guest stack and the host's frames above it are host-only. 9b6e7f6 keeps the guest from changing them by refusing any range that touches them. This PR treats them as gaps instead:GuestThreadStackHostRangesreturns the host-only ranges a range meets.GuestThreadStackHostRangeOverlapsis built on it.DoMprotectcuts the request around those ranges and protects the parts beside them.EINVALwhen the range starts in a skipped page, as for an unmapped start.The debt entry now states what is left: those pages are skipped where the console's guard takes the protection.
Tested
guest_kernel_stack_protect:-1).0x11) leaves the page below the stack unchanged.0x11protect from that page into the stack splits into two general-path parts around the host guard. The pin waiter hook records whether the earlier page already changed when the pin is found, then releases the pin. The test requires the earlier page unchanged at that point, and both parts protected after the call returns 0.Linux (Ubuntu 24.04 container, GCC 13.3.0):
GuestKernelStackProtect failed at line 235(the host guard page returns -1).4c52e758e0) with the new test:GuestKernelStackProtect failed at line 254. The waiter ran (line 253 passed), but the earlier part had already been protected.b11c7bf84f:guest_kernel_stack_protectandguest_pthread_stackpass with--repeat until-fail:3. Of the 240 tests matchingmprotect|memory|stack|pthread|mmap|thread|mtype|apr|ampr|flexible|pool|mlock|protection, 239 pass.guest_mmap_executablefails in this container onmaintoo.Windows 11 Pro, Intel Core i5-14600KF, NVIDIA GPU, WinLibs GCC 15.2.0, full Release build with
libs:guest_kernel_stack_protectandguest_pthread_stackpass.ctest:99% tests passed, 6 tests failed out of 4005. None of them is caused by this branch:test_registrationandselect_tests_MissingCommandTestsfail withWinError 1314, because this user cannot create symbolic links.agc_driver_image_plane_address_on_line,agc_driver_image_sample_lod_clamp,agc_driver_image_sample_lod_clamp_shader_laneandagc_driver_direct_write_completionalso fail withDirectMemory.cppandPthread/restored tomain.check_skips.pyreports one skip,guest_fopen_errors, which skips itself here because this user cannot create symbolic links.macOS: not tested.
GuestThreadStackHostRangesreturns no ranges there, soDoMprotectkeeps its path; the GPU-bit case runs there as on Windows.The three-page read-only case is Linux-only because Windows skips the whole straddling range: the host thread stack is not in the guest allocation registry, so the stack part is not protected either. That is a separate Windows gap and goes in its own PR.
Overlaps: #7149 and #7631 also change
DoMprotect; both conflict withmainalready, and a trial merge with this branch conflicts in the same files only. #14466, #9684 and #7532 merge with this branch without conflicts (git merge-tree).Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)