Repository navigation
fix(libkernel): return EFAULT from kevent for an unreadable changelist or timeout or an unwritable eventlist - #12503
Conversation
…t or timeout or an unwritable eventlist kevent read the changelist and the timeout and wrote the eventlist in place, so a non-null pointer into unmapped or read-only memory crashed the host. It now copies the timeout in first, as FreeBSD's sys_kevent does, copies each change in with GuestCopyIn_nid_no_patch and copies EV_ERROR entries out one at a time. Triggered events are collected by the same single queue call as before into a host buffer (on the stack up to 64 events) and copied out with GuestCopyOut_nid_no_patch, so level-triggered events are still reported once per call. Any failed copy returns EFAULT.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I found one blocking issue in the new staging buffer. capacity is the caller-controlled nevents value after receipt slots are deducted, and when it exceeds 64 this allocates new KernelEvent[capacity] before asking the queue how many events are actually available. A guest can request INT_MAX events with a valid eventlist pointer and force an allocation proportional to roughly two billion KernelEvents; on constrained hosts that can throw std::bad_alloc out of this C ABI entry point or consume a very large virtual/committed address range. The queue may have only one pending event, so this cost is not tied to useful results.
Please bound the temporary buffer independently of the guest's nevents or handle allocation failure as a guest-visible error, and add a regression for an oversized nevents request that does not require allocating the full requested capacity. I have not run the focused guest_kqueue test locally: the full build configuration currently stops because the installed system Vulkan headers lack VK_KHR_maintenance8; your PR description reports Linux/Windows focused runs. I have not approved this PR.
|
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.
|
|
The overlap bot identifies concurrent edits in the |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
The EFAULT handling matches FreeBSD's sys_kevent / kqueue_register / kqueue_scan, and the single queue call is the right call for level-triggered events. One blocking problem, the same one @MarcZX10 raised:
core/libs/prx/libkernel/Equeue/src/Kqueue.cpp:645:new KernelEvent[capacity]is sized by the guest'snevents(up toINT_MAX, about 64 GB) before the queue says how many events exist. That throwsstd::bad_allocout of a C ABI entry point, or commits a huge range for one pending event. Bound the staging buffer independently ofnevents.keventmay return fewer events than requested, and the events left in the queue are returned by the next call, so clampingcapacity(for example to 1024) is enough. Alternatively, loop over fixed-size chunks only if you can keep the single-call requeue behaviour.- Add a case to
BadPointersFaultAndBatchesFillwith a valid eventlist and a very largenevents(for exampleINT_MAXon the 16-slot buffer's address, or the clamp boundary) showing it returns the pending events without a huge allocation.
The overlap bot lists #9575 and #10014 touching the same test insertion spot; that is only adjacency, so rebase when one lands.
The staging buffer was sized by the guest's nevents, so a request for INT_MAX events allocated about 64 GiB before the queue reported how many were pending. It's now bounded at 1024 events; kevent may return fewer than requested, and events left in the queue come back on the next call. The test covers an INT_MAX request and a 1030-event queue, and moves away from the end of StandardInput and the start of main, where boykopovar#9575 and boykopovar#10014 also add tests.
|
@MarcZX10 @adriadam10 thanks, both right. On the overlap with #9575 and #10014: I moved the test function to before Linux and Windows results for this head, with the negative control, are in the description. One more thing I found while measuring: the checked copies make a AI-assisted: yes (Claude Code) |
|
Windows run of this head (
AI-assisted: yes (Claude Code). |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — changes requested
The unbounded staging allocation is fixed and the added guest-memory fault coverage is useful. I still cannot approve the current head: the author’s own matched-path measurements show a substantial hot-poll regression, and the test addition still conflicts with open PR #10016.
What I ran
- Reviewed exact head e73a56f against current main 52c09d5.
- Ran git merge-tree --write-tree and git diff --check against current main.
- Compared the current diff with open #7088, #7121, and #10016. The #10016 test insertion still conflicts in GuestKqueue.cpp; the implementation hunks I inspected for #7088 and #7121 are in separate registration/modify paths.
- Inspected the bounded capacity logic and the author’s paired performance measurements. I did not run the local tests.
Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean |
| git diff --check | Pass |
| Bounded staging | Capacity is capped at 1,024 regardless of nevents; stack storage is used through 64 |
| Author-reported Linux tests | Full CTest 2,712/2,712, focused test passes, conventions pass; negative control segfaults with main Kqueue.cpp |
| Author/reviewer-reported Windows tests | Focused guest_kqueue passes 3/3; negative control crashes with main Kqueue.cpp |
| Hosted checks | Windows, conventions, eligibility, labels, and report pass; Linux, register-context, relinker, ASan, and write-watch jobs are skipped |
| Overlap preview | Pairwise merge-tree with #10016 conflicts in GuestKqueue.cpp |
Negative control
- The author reports that the new test crashes when run with main’s Kqueue.cpp and passes with the PR implementation on Linux and Windows. I did not reproduce that locally.
Findings
Blocking
-
Measured hot-path regression. The author reports a matched Linux measurement for four ready events: about 98 ns per call on main versus 2,773–2,861 ns on this head, roughly 28× slower. The author also explicitly concluded that #12503 should not merge with these guarded copies as-is, pending a fast-copy design decision. This is a measured cost on the operation’s polling path, not a theoretical concern. Please either address the per-event copy overhead without losing the EFAULT safety this PR adds, or get an explicit maintainer decision that accepts this measured cost before treating the change as ready.
-
Unresolved overlap with #10016. The current overlap report identifies a conflict in GuestKqueue.cpp, and my pairwise merge-tree check still reports that conflict. The body says the test was moved away from #9575/#10014, but does not mention #10016. Please coordinate with that author and move/sequence the insertion so both changes can integrate cleanly; declare Depends on if an order is required.
I manually compared the current #7088 and #7121 hunks in Kqueue.cpp: they modify registration/modify paths, while this PR stages and copies results in kevent_nid_postfix, so I found no same-line dependency there.
Sources
- Current PR head and changed files
- Repository overlap and dependency rule
- Overlapping open PR #10016
- Related registration PR #7088
- Related vnode-note PR #7121
Not verified
- I did not independently rerun the Linux/Windows suites or reproduce the timing benchmark. Test and timing figures above are author/reviewer-reported.
- The hosted product matrix is mostly skipped; the passing Windows check does not substitute for the skipped Linux and specialized jobs.
To get this to a good state
Resolve the measured copy cost with maintainers, coordinate the still-conflicting #10016 test insertion, and rerun focused tests and the paired benchmark on the integrated head.
What
keventread the changelist and the timeout and wrote the eventlist in place, so a non-null pointer into unmapped or read-only memory crashed the host. FreeBSD'ssys_keventcopies the timeout in first,kqueue_registercopies each change in, andkqueue_scancopies events out, returningEFAULTfor any failed copy.GuestCopyIn_nid_no_patchbefore the changes are applied; its values are still validated where they were.EV_ERROR/EV_RECEIPTentry is copied out withGuestCopyOut_nid_no_patch.GetTriggeredEvents/WaitForEventsUntilcall as before, now into a host buffer (on the stack up to 64 events, on the heap above that, and at most 1024 events whateverneventsasks for), and are copied out once.keventmay return fewer events than requested; any left in the queue come back on the next call. Keeping one queue call matters: level-triggered events are requeued after they're returned, so collecting in several calls would return them twice in onekevent.EFAULTif anything triggered).Tested
BadPointersFaultAndBatchesFillincore/libs/tests/GuestKqueue.cpp, run last inmain: with 12 level-triggered user events, polls with room for 16, 100 (the heap path), 9 and 5 return 12, 12, 9 and 5, and the 16-slot poll reports all 12 idents once. An unmapped changelist, timeout or eventlist fails withEFAULT; a read-only eventlist fails withEFAULTboth for triggered events and for anEV_ERRORentry, and is left unchanged. A request forINT_MAXevents on the 16-slot buffer returns the 12 pending events, and a queue of 1,030 events returns 1,024 per call.anyps5-buildCI image, Ubuntu 24.04, GCC 13, Release,BUILD_TESTING=ON), this heade73a56f8abaonmaind4cb89300fe:guest_kqueuepasses; the fullctestpasses (2,712 of 2,712), andcheck_conventions.pypasses. Withmain'sKqueue.cppthe new test segfaults.BUILD_TESTING=ON), this head: full build andlibssucceed;guest_kqueuepasses 3 of 3 alone, andguest_equeue_eventspasses alone; withmain'sKqueue.cppthe new test crashes (0xc0000005). CI's test selection for this change (2,364 tests): 1,635 pass, 726 skip, and three failures unrelated to this change:guest_raise_exception, which also fails onmainon this machine (libkernel (Windows): guest_raise_exception slowed about 3x and sometimes aborts at "address waking" since 540364a24 (#10666) #11813), and two load timeouts under--parallel 4,check_main_sync(120 s) andselect_tests_SelectArgumentTests(30 s), which pass 3 of 3 alone on this head in at most 49 s and 15 s.check_conventions.pypasses.Cost. Each checked copy is a host system call. A
keventpoll with 4 events ready goes from about 98 ns onmainto about 2,800 ns here (Linux, same build). That's measured and discussed with the same question forclock_gettimeon #12418; until that's settled this shouldn't merge.The test function sits before
CreateAndCloseand is called afterTimeout, away from the spots where #9575 and #10014 add tests; both of their current heads merge with this one without conflicts.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)