Let a pidfd name any relative kill can name - #392
Conversation
Pid-directed kill resolves a guest pid through the fork-family registry, so a guest can signal its parent. The three pidfd call sites still went through the child table alone, which only holds descendants, so kill(getppid(), 0) succeeded while pidfd_open(getppid()) returned ESRCH and a pidfd on any non-descendant relative stayed unreachable. Linux draws no such line. Lift the two-step lookup out of sc_kill into proc_resolve_guest_pid and route pidfd_create, sys_pidfd_open and sys_pidfd_send_signal through it, so every caller that turns a guest-named pid into a host pid reaches the same set of processes. Routing pidfd_create matters beyond the open: it is what arms the exit monitor, so without it a pidfd on a sibling opens but never becomes readable. test-pidfd-parent covers both halves. A child opens a pidfd on its parent and signals through it, and a second child opens a pidfd on its sibling and polls it for the sibling's exit; pipe handshakes order the open before the exit so a slow fork cannot turn the test red. It fails on the parent commit, and reverting any one of the three call sites on its own also fails it. A qemu-aarch64 run confirms the Linux behavior it pins.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/syscall/proc.c">
<violation number="1" location="src/syscall/proc.c:1817">
P2: The registry fallback trusts a record after only a same-binary-name check (registry_collect compares proc_pidpath against this process's path), not the record's original process identity. If the host pid in a record is reused by another process that also runs this elfuse binary (any other elfuse instance of the same uid), the reuse passes the check, and proc_resolve_guest_pid returns the recycled host pid. sys_pidfd_open / pidfd_send_signal / kill then address a process that never carried the guest pid, with an exit monitor watching the wrong process. Validate each registry hit against the recorded process start time (proc_pidinfo start time) before returning its host pid, per the registry-record validation guidance (matching the same rule already recommended for fork-family records).</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
| : -1; | ||
| } | ||
|
|
||
| pid_t proc_resolve_guest_pid(int64_t guest_pid) |
There was a problem hiding this comment.
P2: The registry fallback trusts a record after only a same-binary-name check (registry_collect compares proc_pidpath against this process's path), not the record's original process identity. If the host pid in a record is reused by another process that also runs this elfuse binary (any other elfuse instance of the same uid), the reuse passes the check, and proc_resolve_guest_pid returns the recycled host pid. sys_pidfd_open / pidfd_send_signal / kill then address a process that never carried the guest pid, with an exit monitor watching the wrong process. Validate each registry hit against the recorded process start time (proc_pidinfo start time) before returning its host pid, per the registry-record validation guidance (matching the same rule already recommended for fork-family records).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/syscall/proc.c, line 1817:
<comment>The registry fallback trusts a record after only a same-binary-name check (registry_collect compares proc_pidpath against this process's path), not the record's original process identity. If the host pid in a record is reused by another process that also runs this elfuse binary (any other elfuse instance of the same uid), the reuse passes the check, and proc_resolve_guest_pid returns the recycled host pid. sys_pidfd_open / pidfd_send_signal / kill then address a process that never carried the guest pid, with an exit monitor watching the wrong process. Validate each registry hit against the recorded process start time (proc_pidinfo start time) before returning its host pid, per the registry-record validation guidance (matching the same rule already recommended for fork-family records).</comment>
<file context>
@@ -1814,6 +1814,12 @@ pid_t proc_namespace_host_pid(int64_t guest_pid)
: -1;
}
+pid_t proc_resolve_guest_pid(int64_t guest_pid)
+{
+ pid_t host_pid = proc_guest_to_host_pid(guest_pid);
</file context>
There was a problem hiding this comment.
This validation is already in place, so there is nothing to change here.
registry_collect is not the only filter. It reads through registry_read_locked → registry_parse_cb, which parses each record as hostpid guestpid pgid startus and drops it unless the live process's start time matches the recorded one:
uint64_t live_us;
if (!host_start_us((pid_t) hp, &live_us) || live_us != (uint64_t) st)
return;A record naming a reused host pid therefore never reaches the proc_pidpath comparison; that check is a second filter layered on top, not the only one. This landed in #388 as 6ff3f96 ("Tie a registry record to its process start time") and 6e21ed8 ("Check the start time on the reverse lookup too"). proc_resolve_guest_pid inherits it unchanged, since the registry half is proc_namespace_host_pid and the child-table half holds live descendants.
pidfd_create resolves the target a second time, after the caller has already resolved it. A non-descendant that exits in that window leaves the new pidfd with no monitor behind it, and nothing else ever marks it readable, so a guest polling for the target's exit waits forever. Linux returns a pidfd that reads as exited instead. Mark the fd exited whenever no monitor was armed, which covers both the lost target and a monitor thread that failed to start. A pidfd on the caller itself is the one target that legitimately has no monitor: a guest cannot observe its own exit through it. It now returns before the resolve rather than falling into the completion path, and test-pidfd-parent pins that a self pidfd does not poll readable.
| * Linux hands out a pidfd that reads as exited rather than one that waits | ||
| * forever, so complete it here. | ||
| */ | ||
| if (!monitor_ok) |
There was a problem hiding this comment.
This completion now runs for a target that never resolved, and clone3(CLONE_PIDFD | CLONE_VM) reaches pidfd_create with exactly that: sys_clone_vm allocates the child tid in the thread table and never calls proc_register_child or publishes to the registry, so proc_resolve_guest_pid returns -1 for a child that is running. The guest then gets a pidfd that reads as exited from the moment clone3 returns. Gate the completion on the target having resolved, and handle the unresolvable non-self case separately.
There was a problem hiding this comment.
Confirmed and fixed in 655d6a0. Reproduced first: clone3(CLONE_PIDFD | CLONE_VM) then ppoll on the pidfd while the child slept gave poll=1 revents=0x1 — the fd read as exited immediately, exactly as described.
Rather than gate the completion, pidfd_create no longer resolves at all: the caller passes the host pid to watch, and 0 means the target lives in this host process and has nothing to watch, so the fd is left unreadable as before. sys_pidfd_open passes what it already resolved, and the clone3 path passes proc_resolve_guest_pid(ret), which is the child's host pid for a fork child and -1 for a CLONE_VM child.
That also removes the second lookup cubic's earlier finding was about, so the window it described no longer exists. A target that died in the meantime is handled where it belongs: the monitor thread's kill(hpid, 0) ESRCH check and the kevent failure path both complete the fd.
Covered by a new case in test-pidfd, which asserts the CLONE_VM child's pidfd is not readable while it runs; restoring the old behavior turns it red. It went to test-pidfd rather than the cross-checked test because a CLONE_VM child resumes on a fresh stack, where the compiler's copy of the clone3 return value is undefined — the if (ret == 0) branch is only reliable under elfuse, and against qemu the child took the parent branch and died. The same reason your existing CLONE_VM tests sit in QEMU_SKIP, I think.
Unrelated observation from writing it: a VM-clone child calling exit (93) directly takes the whole host process down with it, before stdio is flushed — the parent's output disappears entirely and the process still exits 0. I parked the child in a nanosleep loop instead. Happy to file that separately if it is not already known.
| * forever, so complete it here. | ||
| */ | ||
| if (!monitor_ok) | ||
| proc_pidfd_notify_exit(target_pid); |
There was a problem hiding this comment.
proc_pidfd_notify_exit matches by guest pid rather than by fd, so a malloc or pthread_create failure here also completes every pidfd already watching that same live target, not just the one being created. The premise in the comment above is only half true today: pidfd_monitor_thread returns without notifying when kqueue() fails, which leaves exactly the forever-unreadable fd this block exists to prevent. Worth a follow-up that completes only this entry and closes the monitor-side gap.
There was a problem hiding this comment.
Both halves are right, and a follow-up suits them better than this PR — they are pre-existing, and fixing them properly means changing what proc_pidfd_notify_exit keys on, which touches the reap path and the CLONE_PIDFD path as well as this one.
What I will do there: complete a single entry by fd for the creation-failure case, and make pidfd_monitor_thread notify when kqueue() fails rather than returning silently, so the block's premise stops being half true.
655d6a0 does narrow it slightly in passing — the completion now runs only for a target that was actually given a host pid to watch, so the self and CLONE_VM cases no longer reach it at all. The over-broad match on a malloc/pthread_create failure stands as you describe.
I will open it once this merges, unless you would rather have it here.
| return pidfd_create(g, pid); | ||
|
|
||
| if (proc_guest_to_host_pid(pid) > 0) | ||
| if (proc_resolve_guest_pid(pid) > 0) |
There was a problem hiding this comment.
pidfd_open with a non-positive pid lands on -LINUX_ESRCH here, but the kernel rejects it earlier: pidfd_open returns EINVAL for pid <= 0. proc_resolve_guest_pid already refuses those, so only the errno the guest sees differs.
| if (proc_resolve_guest_pid(pid) > 0) | |
| if (pid <= 0) | |
| return -LINUX_EINVAL; | |
| if (proc_resolve_guest_pid(pid) > 0) |
There was a problem hiding this comment.
Applied in 655d6a0, with the guard first so nothing is looked up before it.
test-pidfd-targets now asserts pidfd_open(0) and pidfd_open(-1) both return EINVAL; dropping the guard turns it red. Both also pass against qemu-aarch64, so the errno matches what the kernel gives rather than only what I read.
The previous commit completed any pidfd that got no monitor, which is wrong for a target that has a guest tid but no host process of its own. clone3(CLONE_PIDFD | CLONE_VM) is exactly that: sys_clone_vm allocates a thread-table slot and neither registers a child nor publishes to the registry, so the target never resolves and the guest got a pidfd that read as exited from the moment clone3 returned. Let the caller say what to watch instead of resolving a second time. pidfd_create now takes the host pid, and a host pid of 0 means the target lives in this process and has nothing to watch: it is left unreadable, as before. A target that has already exited needs no special case either, since the monitor thread finds it gone and completes the fd on the spot, so the race this closes stays closed. pidfd_open also rejects a non-positive pid with EINVAL, matching the kernel, which refuses it before looking anything up. test-pidfd-targets covers the self and EINVAL cases; test-pidfd covers the CLONE_VM one, since a CLONE_VM child resumes on a fresh stack where the clone3 return value the child branch reads is undefined on a real kernel, and that test does not run against qemu.
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
ppoll skips a negative fd and leaves revents alone, and clone3 writes the pidfd only once pidfd_create succeeds. The new CLONE_VM check therefore passed whether the guest got a working pidfd or none at all: with pidfd_create forced to fail, the "stays unreadable" assertion still reported OK. Assert the fd was written before polling it, the way the CLONE_PIDFD case above already does. The same forced failure now turns the check red.
|
Thank @xalestar for contributing! |
2 participants
Summary
Follow-up to #388, which routed pid-directed
killthrough the fork-family registry so a guest can signal its parent. The three pidfd call sites still resolved throughproc_guest_to_host_pidalone, so after #388kill(getppid(), 0)succeeded whilepidfd_open(getppid())returned ESRCH, and a pidfd on any relative that is not a descendant stayed unreachable. Linux draws no such line. This routes all three through the same resolver.What
proc_resolve_guest_pid()(src/syscall/proc.c) holds the two-step lookup that was previously private tosc_kill: the child table answers for descendants, the fork-family registry for every other relative.sc_killand the three sites insrc/syscall/proc-pidfd.c—pidfd_create,sys_pidfd_open,sys_pidfd_send_signal— now share it, so every caller that turns a guest-named pid into a host pid reaches the same set of processes.Routing
pidfd_creatematters beyond the open. That resolve is what arms the exit monitor, so with only the open fixed, a pidfd on a sibling would open and then never become readable — the process could exit andpoll()would wait forever. The test below covers that separately.Why
Reported by @jserv on #388 (src/syscall/proc.h:506).
Test plan
tests/test-pidfd-parent.ccovers both halves, registered intests/test-matrix.shnext totest-kill-parent:sys_pidfd_openandsys_pidfd_send_signalresolve an ancestorpidfd_createarms the exit monitor for a process that is neither ancestor nor descendantThe sibling half is ordered by two pipe handshakes rather than by sleeps: the sibling blocks on a pipe and exits only after the watcher reports its pidfd open, so a slow fork cannot turn the test red. An earlier sleep-ordered draft was observably flaky; this one passed 10/10 consecutive runs under elfuse.
Mutation check — reverting any one of the three call sites on its own turns the test red, each with a distinct message:
pidfd_createsys_pidfd_openpidfd_openfails for both the parent and the siblingsys_pidfd_send_signalCommands run, all clean:
The new test fails on the merge commit of #388 and passes here. Cross-checked against a real kernel with
tests/qemu-runner.sh(qemu-aarch64), where it passes 3/3.Backward compatibility
No ABI or flag changes.
proc_guest_to_host_pidkeeps its meaning and its other callers; the registry fallback only adds pids that previously resolved to ESRCH, so nothing that worked before resolves differently now.Related issue
No separate issue — follow-up to review feedback on #388. Rebased on main at 669ea25.
Summary by cubic
Lets a pidfd name any relative
killcan name: the three pidfd call sites now resolve guest pids through the same fork-family registry askill, so pidfds on a parent or sibling work as on Linux. A pidfd on the caller itself or a CLONE_VM child stays unreadable, and a target that exits during setup still reads as exited.Details
pidfd_create,sys_pidfd_open, andsys_pidfd_send_signalthroughproc_resolve_guest_pid, the two-step lookupsc_killalready used.pidfd_createnow takes the host pid from its caller instead of resolving again, closing the race where a target vanished between the caller's resolve and the open's re-resolve.sys_pidfd_openrejects a non-positive pid with EINVAL before any lookup.tests/test-pidfd-targets.ccovering pidfds on a parent, a sibling, the caller itself, and non-positive pids, plus a CLONE_VM case intests/test-pidfd.cthat asserts the pidfd was written before polling it (ppoll skips a negative fd and would pass vacuously), with pipe handshakes that order the open before the exit so the tests are not flaky.Written for commit dbb1f57. Summary will update on new commits.