Skip to content

Let a pidfd name any relative kill can name #392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jserv merged 4 commits into from
Sep 21, 2026
Merged

Let a pidfd name any relative kill can name #392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
94067f8
Let a pidfd name any relative kill can name
xalestar Sep 21, 2026
aff1cc5
Complete a pidfd whose target is already gone
xalestar Sep 21, 2026
655d6a0
Complete a pidfd only if something watched it
xalestar Sep 21, 2026
dbb1f57
Check the CLONE_VM pidfd exists before polling it
xalestar Sep 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/runtime/forkipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2239,7 +2239,10 @@ int64_t sys_clone3(hv_vcpu_t vcpu,
* child and write the guest FD number to ca.pidfd.
*/
if (ret > 0 && want_pidfd && ca.pidfd != 0) {
int pfd = pidfd_create(g, ret);
/* A CLONE_VM child has no host pid of its own, so it resolves to -1 and
* its pidfd carries no monitor.
*/
int pfd = pidfd_create(g, ret, proc_resolve_guest_pid(ret));
if (pfd >= 0) {
int32_t pfd32 = (int32_t) pfd;
if (guest_write_small(g, ca.pidfd, &pfd32, sizeof(pfd32)) < 0) {
Expand Down
38 changes: 28 additions & 10 deletions src/syscall/proc-pidfd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ static void *pidfd_monitor_thread(void *arg)
return NULL;
}

int pidfd_create(guest_t *g, int64_t target_pid)
int pidfd_create(guest_t *g, int64_t target_pid, pid_t host_pid)
{
(void) g;
int pfd[2];
Expand Down Expand Up @@ -141,9 +141,16 @@ int pidfd_create(guest_t *g, int64_t target_pid)
entry->write_end = pfd[1];
pthread_mutex_unlock(&pidfd_lock);

pid_t host_pid = proc_guest_to_host_pid(target_pid);
if (host_pid > 0) {
bool monitor_ok = false;
/* host_pid <= 0 means the target lives inside this host process -- the
* caller itself, or a CLONE_VM child, which holds a guest tid but no host
* pid of its own. Neither can be watched from here, and neither has exited,
* so the fd stays unreadable rather than being completed.
*/
if (host_pid <= 0)
return gfd;

bool monitor_ok = false;
{
int64_t *ctx = malloc(2 * sizeof(int64_t));
if (ctx) {
ctx[0] = target_pid;
Expand All @@ -164,10 +171,16 @@ int pidfd_create(guest_t *g, int64_t target_pid)
free(ctx);
}
}
if (!monitor_ok)
proc_pidfd_notify_exit(target_pid);
}

/* Nothing will ever mark this fd readable without a monitor behind it, so
* complete it rather than leave the guest polling forever. A target that
* has already exited needs no special case: the monitor thread finds it
* gone and completes the fd the same way.
*/
if (!monitor_ok)

@jserv jserv Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@xalestar xalestar Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

proc_pidfd_notify_exit(target_pid);

@jserv jserv Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@xalestar xalestar Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gfd;
}

Expand Down Expand Up @@ -204,11 +217,16 @@ int64_t sys_pidfd_open(guest_t *g, int64_t pid, unsigned int flags)
if (flags != 0)
return -LINUX_EINVAL;

/* The kernel rejects a non-positive pid before it looks anything up. */
if (pid <= 0)
return -LINUX_EINVAL;

if (pid == proc_get_pid())
return pidfd_create(g, pid);
return pidfd_create(g, pid, 0);

if (proc_guest_to_host_pid(pid) > 0)
return pidfd_create(g, pid);
pid_t host_pid = proc_resolve_guest_pid(pid);
if (host_pid > 0)
return pidfd_create(g, pid, host_pid);

return -LINUX_ESRCH;
}
Expand Down Expand Up @@ -238,7 +256,7 @@ int64_t sys_pidfd_send_signal(guest_t *g,
return 0;
}

pid_t host_pid = proc_guest_to_host_pid(pid);
pid_t host_pid = proc_resolve_guest_pid(pid);
if (host_pid > 0) {
if (sig == 0) {
if (kill(host_pid, 0) < 0)
Expand Down
7 changes: 6 additions & 1 deletion src/syscall/proc-pidfd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,12 @@
#include "core/guest.h"

void pidfd_init(void);
int pidfd_create(guest_t *g, int64_t target_pid);

/* Create a pidfd on @target_pid. @host_pid is the host process to watch for its
* exit, or <= 0 when the target lives in this host process and so cannot be
* watched (the caller itself, or a CLONE_VM child).
*/
int pidfd_create(guest_t *g, int64_t target_pid, pid_t host_pid);
void proc_pidfd_notify_exit(int64_t exited_pid);
int64_t proc_pidfd_lookup_pid(int guest_fd);
int64_t sys_pidfd_open(guest_t *g, int64_t pid, unsigned int flags);
Expand Down
6 changes: 6 additions & 0 deletions src/syscall/proc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

@cubic-dev-ai cubic-dev-ai Bot Sep 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

@xalestar xalestar Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
pid_t host_pid = proc_guest_to_host_pid(guest_pid);
return host_pid > 0 ? host_pid : proc_namespace_host_pid(guest_pid);
}

int64_t proc_host_to_guest_pid(pid_t host_pid)
{
pthread_mutex_lock(&pid_lock);
Expand Down
11 changes: 11 additions & 0 deletions src/syscall/proc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,17 @@ int proc_get_namespace_targets(proc_signal_target_t *out,
*/
pid_t proc_namespace_host_pid(int64_t guest_pid);

/* Resolve one guest pid the way pid-directed signalling does: the child table
* answers for descendants, the fork-family registry for every other relative.
* Every caller that turns a guest pid the guest named into a host pid -- kill,
* pidfd_open, pidfd_send_signal -- goes through here, so they all reach the
* same set of processes.
*
* Returns the host pid, or -1 when no live fork-family member carries that
* guest pid.
*/
pid_t proc_resolve_guest_pid(int64_t guest_pid);

/* Publish the caller's current guest pid/pgid to the fork-family registry. */
void proc_registry_publish_self(void);

Expand Down
15 changes: 2 additions & 13 deletions src/syscall/syscall.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1034,17 +1034,6 @@ static int kill_deliver_targets(const proc_signal_target_t *targets,
return delivered;
}

/* Resolve a guest pid for kill(2). The child table answers for descendants.
* Every other member of the fork family, the caller's own parent above all,
* exists only in the namespace registry, the same source the group and
* broadcast forms already read.
*/
static pid_t kill_resolve_host_pid(int64_t gpid)
{
pid_t hpid = proc_guest_to_host_pid(gpid);
return hpid > 0 ? hpid : proc_namespace_host_pid(gpid);
}

static int64_t sc_kill(guest_t *g,
uint64_t x0,
uint64_t x1,
Expand Down Expand Up @@ -1094,7 +1083,7 @@ static int64_t sc_kill(guest_t *g,
}
int64_t r = (pid == (int) our_pid) ? 0 : -LINUX_ESRCH;
if (r == -LINUX_ESRCH) {
pid_t hpid = kill_resolve_host_pid((int64_t) pid);
pid_t hpid = proc_resolve_guest_pid((int64_t) pid);
if (hpid > 0)
r = (kill(hpid, 0) == 0) ? 0 : -LINUX_ESRCH;
}
Expand Down Expand Up @@ -1159,7 +1148,7 @@ static int64_t sc_kill(guest_t *g,
signal_queue(sig);
return 0;
}
pid_t hpid = kill_resolve_host_pid((int64_t) pid);
pid_t hpid = proc_resolve_guest_pid((int64_t) pid);
if (hpid > 0)
return (proc_send_guest_signal(hpid, (int64_t) pid, sig) == 0)
? 0
Expand Down
2 changes: 2 additions & 0 deletions tests/test-matrix.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,8 @@ run_unit_tests()
"$bindir/test-kill-pgroup"
test_check "$runner" "test-kill-parent" "0 failed" \
"$bindir/test-kill-parent"
test_check "$runner" "test-pidfd-targets" "0 failed" \
"$bindir/test-pidfd-targets"
test_rc "$runner" "test-sigio" 0 "$bindir/test-sigio"
test_rc "$runner" "test-fault-signal-mt" 0 "$bindir/test-fault-signal-mt"
test_rc "$runner" "test-exit-group-worker" 0 "$bindir/test-exit-group-worker"
Expand Down
Loading
Loading