Stabilize single-copy tfork clone path

[yiying-zhang]

Summary

This PR stabilizes the single-copy tfork/live-clone path and aligns it with the n-copy restore machinery. It fixes nested PID-namespace restoration, preserves application-visible thread IDs, hardens pstree construction and unwind behavior, improves restore synchronization and diagnostics, and restores pycriu compatibility with both PSTREE image formats.

The final diff touches 18 files across Podman, crun, CRIU, and pycriu.

Single-copy clone routing

• Route single-copy live clones through the direct crun/tfork invocation instead of the fragile legacy conmon bootstrap.
• Use the n-copy restore helper for one copy by default so single- and multi-copy clones share PID-file, stdio, readiness, and status handling.
• Keep two explicit debugging escape hatches:
  • PODMAN_TFORK_SINGLE_COPY_CONMON=1 selects the legacy conmon bootstrap.
  • PODMAN_TFORK_SINGLE_COPY_DIRECT=1 bypasses the n-copy restore helper.
• Treat --tfork-copies=1 as a real n-copy-helper request throughout Podman, crun, and CRIU.
• Register and return only clones that reached visible libpod state.
• Make clone readiness/crun completion timeout configurable with PODMAN_TFORK_CLONE_READY_TIMEOUT_SECS.

PID namespaces and process restoration

• Preserve nested PID hierarchy metadata through dump and restore.
• Finalize leaf PID-namespace IDs before writing PSTREE so nsid stays aligned with the innermost localpid.
• Prevent nested PID namespace init tasks from colliding with the container init at PID 1.
• Ensure n-copy helpers create only the per-copy mount namespace; CRIU recreates PID namespaces from the image.
• Repair missing CLONE_NEWPID state for nested PID namespace init tasks.
• Rebase/truncate stale source-side outer PID chains for tfork restore copies.
• Allocate fresh ancestor-namespace PIDs where concurrent copy helpers could collide while preserving IDs visible inside the restored container.
• Preserve dumped non-leader TIDs in the innermost PID namespace via clone3 set_tid[0]; outer namespace TIDs remain kernel-assigned. The ns_level > 0 guard keeps the zero-level case from reading an uninitialized TID slot.
• Propagate tfork state into the restorer so tfork-only restore behavior remains scoped.

PSTREE correctness and compatibility

• Load namespace IDs before inserting pstree PIDs, preventing incomplete PID namespace keys from entering the rbtrees.
• Make pstree parse failures unwind only nodes actually inserted by the current item.
• Correct the non-leader thread unwind bounds and avoid treating the leader mirror as an independently indexed thread.
• Remove linked PID nodes, parent/root links, and allocated thread storage consistently on failure.
• Reset failed items to TASK_UNDEF for diagnostic-safe reuse checks.
• Keep arena-owned pstree items in the shared restore arena instead of freeing an interior allocation.
• Add a dual-format pycriu PSTREE handler for:
  • legacy images containing one pstree_entry per framed record;
  • current images containing one file-level pstree_file_entry.
• Stream framed payloads, validate truncation/negative sizes, preserve round-trip dumping, and document the proto2-required-field format discriminator.
• Add test_pstree_compat.py and run it before starting the pycriu service tests, including under parallel make.

Restore synchronization and failure handling

• Bound tfork restore-stage waits and emit participant/task/thread/helper counts on timeout.
• Add stage-level leader/thread diagnostics around restore barriers, helper waits, zombie waits, signals, and inotify cleanup.
• Avoid deadlock when a dumped zombie is not reparented to the restored tfork task.
• Make freezer collection tolerate transient tfork tasks and EINTR without changing the normal restore path.
• Preserve hard failure for unexpected thread-TID mismatches now that the innermost TID is restored exactly.
• Improve clone3/PID-chain diagnostics and propagate useful CRIU error state.

Operational behavior and diagnostics

• Print relevant CRIU failure lines plus the last 80 log lines.
• Include the main restore log and per-copy restore logs when tfork fails.
• Add PID namespace, pstree, clone3, and restore-stage context to failure logs.
• Make CRIU kernel-module reloads fail closed: abort if a loaded module cannot be removed or the rebuilt module cannot be inserted, rather than continuing with stale code.
• Propagate external Unix socket support through crun only when explicitly opted in with CRUN_TFORK_EXT_UNIX_SK.

Inotify behavior

The temporary tfork-specific inotify bypass was reverted. This is independent of the thread-TID fix: clones now use CRIU's normal watch-mark replay path. If mark replay cannot be completed, restore fails instead of silently producing a clone with dead watchers. Because the final tree matches the normal CRIU implementation, fsnotify.c does not appear in the net PR diff.

Validation

Completed:

• Gensee-driven live Codex/tclone fork validation on the wuklab environment during development.
• Legacy and current PSTREE decode, entry count, and round-trip compatibility test: pass.
• Python compilation for the pycriu handler and compatibility test: pass.
• Parallel-make ordering dry run for the pycriu test target: pass.
• git diff --check: pass.
• Multiple review rounds covering PID namespace identity, pstree unwind ownership/bounds, argv sizing, EINTR scoping, module reload safety, and behavioral compatibility.

Still required before merge on the Ubuntu x86_64 tfork host:

• ZDTM thread and PID-namespace coverage.
• A live fork while a non-leader thread owns a robust or errorcheck mutex.
• Confirmation that non-leader gettid() inside the clone matches its pre-fork value.
• A live fork with an active inotify watcher to verify event delivery after clone.

These checks require root, btrfs-backed Podman storage, and the tfork kernel modules and cannot be run on the macOS/ARM development machine.

[yiying-zhang]

Review: stabilize single-copy tfork clone path

Read the full diff (1,326 lines across vendored CRIU, crun, and podman).

The PID-namespace work looks like the real fix here — nested pidns leaf-id assignment at dump time, hierarchy truncation at restore, and the --keep-pid-hierarchy plumbing all hang together sensibly. The podman-side path selection with env escape hatches is a reasonable way to land this.

My main concern is a pattern running through the rest of the diff: under tfork.active, a number of hard restore errors have been converted into warnings, skips, or bounded timeouts. Individually each is defensible; together they mean a fork that used to fail loudly now succeeds and is quietly wrong. Details below, roughly in priority order.

---

🔴 Threads are restored with fresh TIDs

Two changes combine. Thread creation drops set_tid entirely under tfork:

if (args->tfork_active) {
    pr_info("tfork: restore thread pid=%d with fresh tid, set_tid_size %d -> 0 ...");
    c_args.set_tid = 0;
    c_args.set_tid_size = 0;
}

and __export_restore_thread now accepts the resulting mismatch instead of failing:

if (args->ta && args->ta->tfork_active) {
    pr_info("tfork: accepting fresh thread tid %d instead of dumped tid %d\n", ...);
    args->pid = my_pid;

CRIU restores exact TIDs specifically because userspace caches them. After this, the restored TCB still holds the dumped tid while the kernel tid is fresh. That breaks:

• robust and errorcheck mutexes — the futex word stores the owner tid, so ownership checks and EOWNERDEAD recovery misfire
• pthread_join / pthread_kill against a cached pthread_t
• anything that recorded gettid() in its own state (thread pools, userspace schedulers, tracing)

The clone comes up fine and then misbehaves later, far from the cause. If this is a deliberate tradeoff to get forks working at all, could it get a comment at the set_tid = 0 site stating what is known-broken, and ideally a gate so it only applies when exact-tid restore has actually failed?

🔴 The restore barrier abort is suppressed, then papered over with a timeout

In pie/restorer.c, any child exiting cleanly during restore is now ignored:

if (tfork_active_local && siginfo->si_code == CLD_EXITED && siginfo->si_status == 0) {
    pr_info("tfork: ignoring clean child exit during restore: task %d\n", siginfo->si_pid);
    return;
}

Previously this called futex_abort_and_wake(&task_entries_local->nr_in_progress). A task exiting cleanly but prematurely mid-restore is a genuine failure — the barrier is waiting on it.

And in cr-restore.c, the barrier itself stops being a real barrier under tfork:

for (waited = 0; waited < 100; waited++) {
    if ((int)futex_get(np) <= participants) break;
    usleep(100000);
}
if ((int)futex_get(np) > participants) { ...; return -ETIMEDOUT; }

These two are coupled: suppressing the abort is what makes the barrier hang, and the 10s poll is what stops the hang. That works, but it converts "restore failed for reason X" into "restore timed out for no stated reason."

Concerns with the timeout specifically:

• 10s is hardcoded with no env override, while PODMAN_TFORK_CLONE_READY_TIMEOUT_SECS was added one layer up precisely because 60s wasn't always enough. A large process tree or a loaded host will hit this spuriously — likely a flake source in exactly the workflow this PR is stabilizing.
• It doesn't set_cr_errno, unlike the abort path just below it.
• The interaction with the new logging (next item) is real, not theoretical.

🟠 ~20 new pr_warn calls on the PIE restorer hot path

__export_restore_task and __export_restore_thread now emit pr_warn around every stage transition, per task and per thread — barrier entry/exit, wait_helpers, wait_zombies, cleanup_inotify, each restore_signals, each sigaction.

Two problems:

1. These are normal progress events, so every successful fork now emits a wall of warnings. pr_debug/pr_info is the right level, and ideally behind an opt-in flag.
2. Each one writes to the log fd from inside the restorer. On a large tree this adds real time to the restore — in a path that now has a hardcoded 10s barrier timeout. Verbose logging plausibly pushes a slow-but-healthy restore over the limit.

🟠 inotify watches are silently dropped in clones

if (opts.tfork.active) {
    list_for_each_entry(wd_info, &info->marks, list) skipped++;
    if (skipped)
        pr_warn("tfork: restored inotify fd %#08x without %u watch mark(s)\n", ...);
    ...
    *new_fd = tmp;
    return tmp < 0 ? -1 : 0;
}

The fd is restored with zero watches. For agent/dev workloads this is user-visible: file watchers, dev-server rebuilds, and editors in the clone just stop reacting to changes, with the only signal buried in a CRIU log. Worth surfacing this to the user at the podman/gensee layer, or documenting it as a known limitation of forked containers.

🟠 ext_unix_sk = true is now unconditional

cr_options.ext_unix_sk = true;

The comment explains the motivation, and it's a reasonable one. But this doesn't just unblock the dump — external unix socket connections are not re-established on restore. The change turns "CRIU refuses to dump" into "clone starts with silently dead socket connections." Since the stated motivation is tmux/Codex hooks and the host-control bridge, those are exactly the connections that will be dangling in the clone. Please note this in the PR description at minimum; a flag would be better than a hard default.

🟠 Dead error path in crtools.c

if (opts.tfork.active && opts.tfork.copies >= 1) {
        ...
        if (!opts.tfork.active) {
                pr_err("--tfork-copies requires --tfork-restore ...");
                return 1;
        }

The !opts.tfork.active guard is now inside a block conditioned on opts.tfork.active, so it is unreachable. criu restore --tfork-copies=N without --tfork-restore no longer gets the diagnostic — it silently skips the entire block instead. The check needs to move above the outer if.

🟠 Removing CLONE_NEWPID from the n-copy helper

/* The n-copy helper must not create/occupy PID 1 in a new PID namespace ... */
const int ns_flags = CLONE_NEWNS;

The reasoning is clear and the EEXIST failure it fixes is real. But with N copies, the helper children now share the parent's PID namespace. Two questions:

1. With --tfork-copies=4, where does each copy's PID namespace now come from — the CLONE_NEWPID repair in fork_with_pid for the pidns init? Worth stating explicitly in the comment, since the invariant is no longer local to this function.
2. Is there still isolation between copies, or can copy 2 now see and signal copy 3's processes before their own pidns is established?

Related: opts.keep_pid_hierarchy = 0; is set per-copy here while --keep-pid-hierarchy is force-appended to both the RPC argv and the execv argv. The flag being pushed in one place and zeroed in another reads as contradictory — a comment on why the ncopy child must not keep the hierarchy would help.

---

Correctness / robustness, smaller

• freezer_wait_processes EINTR handling is a no-op. EINTR is allowed past the error check, but the very next branch is if (pid < 0 || waited_ms >= 500) { ...; return 0; } — so an interrupted waitpid immediately abandons collection rather than retrying. Either retry on EINTR or drop it from the allowed set.
• get_or_create_pstree_item / read_one_pstree_item reordering. Insertion moved out of the former into the latter. Previously an insert failure did xfree(item); return NULL;; now __pstree_insert_pid(pi->pid, NULL, NULL) failing just does goto err with the item already linked into the pstree — half-constructed state left behind. Also worth confirming __pstree_insert_pid does its own lookup when passed NULL, NULL (not visible in the diff), and that ret is initialized at declaration given the new earlier goto err sites.
• struct pid tfork_pid = *item->pid; in fork_with_pid copies a struct containing rb-tree nodes onto the stack. Only ns[]/ns_level are read so it's correct today, but a stack copy of a tree-linked node is a footgun — a short comment, or copying just the fields needed, would be safer.
• char tail[200][1024] in show_criu_log is 200 KB of stack. Fine on the main thread, risky if this ever runs on a smaller stack. Also 200 is repeated six times — worth a #define.
• show_criu_tfork_restore_copy_logs always loops 0..15 regardless of actual copy count, so a single-copy failure emits 15 spurious "excerpt from CRIU log" blocks. Pass the real copy count.
• The excerpt filter got much broader (Warn , failed, Unable, Can't, No such) and the last 200 lines are now always dumped — so matched lines print twice and failure output balloons (up to ~3,200 lines with the 16-iteration loop above). Consider dropping the filter now that the tail dump exists.
• needState := !useNcopyRestore && copies == 1 — !useNcopyRestore already implies copies == 1; the second clause is redundant.
• useSingleCopyDirect only participates in the useNcopyRestore expression. It works (it lands on the legacy non-conmon path), but that's non-obvious from the name — a one-line comment on what the three modes actually select would help the next reader.
• Hand-maintained argv sizing (rpc_max = 33 + ..., argv_max = 8 + 3) is correct for this change, but this is the second time these constants have had to move in lockstep with an added arg. A bounds assert before the final NULL write would catch the next miss cheaply.

build.sh module reload

rmmod "${mod}" 2>/dev/null || true
if lsmod | ... ; then echo "still loaded; keeping existing module"; continue; fi

and the same fallback after a failed insmod. This makes the build succeed while a stale .ko stays resident — for a module whose ABI is being changed in this very PR, that produces confusing downstream failures that look like restore bugs. At minimum make that message a loud warning; better would be comparing /sys/module/<mod>/srcversion against the built module and failing on mismatch.

Compatibility

images.py switching 'PSTREE' from pb.pstree_entry to pb.pstree_file_entry changes how crit decodes PSTREE images, unconditionally. That means this crit can no longer read upstream-CRIU images (and vice versa) with no version guard or error message. Intentional given the format change, but worth calling out in the PR description since it affects anyone debugging with crit against a mixed set of images.

Testing

The description cites manual Codex/tclone fork testing on wuklab. For changes to PID-namespace assignment, thread restore, and the restore barrier, that feels thin — a ZDTM run (especially the pidns, threads, and inotify groups) would catch most of the regressions above, and the pidns leaf-id logic in namespaces.c is pure enough to unit test directly.

---

Net: the pidns fixes look right and I'd like to see them land. The fresh-TID thread restore and the abort-suppression/timeout pair are the two I'd want resolved or explicitly documented as known-broken before merge, since both produce clones that look healthy and aren't.

[yiying-zhang]

Re-review: Address tfork review robustness

Went through the delta against the previous head. Nearly every point from the last round got a real fix rather than a comment, and two of the changes go further than what was asked. Notes below on what's resolved, what's new, and what's still open.

---

Resolved

• build.sh stale module — now aborts on rmmod/insmod failure instead of silently keeping the old .ko. Right call.
• pr_warn → pr_debug across all the restorer trace points. The logging-vs-timeout interaction is gone.
• Dead error path in crtools.c — the !opts.tfork.active guard moved above the outer if, so criu restore --tfork-copies=N gets its diagnostic again.
• struct pid stack copy — replaced with explicit field copies plus a comment naming the rb_node hazard.
• Barrier timeout — set_cr_errno(ETIMEDOUT) added, constants named (tfork_restore_wait_timeout_ms / _poll_us), timeout value included in the error message.
• show_criu_log — 200 KB stack array moved to heap calloc with cleanup_free, named constants, tail 200→80, "Warn " dropped from the filter.
• show_criu_tfork_restore_copy_logs — takes the real copy count instead of always looping 16.
• needState redundancy removed; useSingleCopyDirect and keep_pid_hierarchy = 0 both documented.
• EINTR in freezer_wait_processes now actually retries (see one caveat below).

Two that went beyond the review:

• The sigchld_handler clean-exit suppression was reverted entirely. Better than what I suggested. The barrier abort works again, which turns the 10s poll into a genuine safety net rather than cover for a suppressed failure signal.
• ext_unix_sk is now opt-in via CRUN_TFORK_EXT_UNIX_SK, fail-loud by default, with a comment explaining the tradeoff. Cleaner than the flag I asked for.

Also spotted independently: pstree.c now allocates pi->threads before read_pstree_ids(pi). That ordering dependency wasn't in my review and looks like a real latent bug.

---

New issues in this commit

🔴 argc_max is almost certainly a typo for argv_max

argv_new[argc_new++] = "--keep-pid-hierarchy";
if (argc_new >= argc_max) {
        pr_err("tfork restore argv overflow: used=%d max=%d\n", argc_new, argc_max);
        exit(1);
}

The allocation bound in this function is argv_max (argv_max = 8 + 3, then incremented per NUL in the buffer). argc_max appears nowhere else in the diff. Either it's an unrelated in-scope variable — in which case the check guards the wrong bound — or this doesn't compile. Worth confirming the build actually ran on this commit.

🟠 The overflow check runs one write too late

Even with the correct variable, the append happens before the check. If argc_new == argv_max on entry, argv_new[argc_new++] is already out of bounds by the time the guard sees it. Should be if (argc_new + 1 >= argv_max) before the append.

🟠 The EINTR continue isn't gated on tfork

if (pid < 0 && errno == EINTR)
        continue;

This sits above the !opts.tfork.active error branch, so it changes the non-tfork path too — previously EINTR fell through to pr_perror("Unable to wait processes") and returned -1. Retrying on EINTR is correct practice, but this PR otherwise gates behavior changes behind opts.tfork.active, and the blocking-waitpid case now has no iteration bound. Either gate it or call it out as an intentional upstream fix.

🟡 rmmod failing hard blocks rebuilds while clones are live

Correct in principle, but a module with a non-zero refcount — any running tfork container — now aborts the build. Since that's the common cause, the message ("aborting to avoid stale module") would be more actionable with "stop running containers first."

---

Still open from the previous round

• Fresh thread TIDs — documented, not changed. The new comment gives the rationale and asserts "process IDs remain restored exactly," but doesn't say what breaks: robust/errorcheck mutexes storing the owner TID in the futex word, cached gettid() in the TCB, pthread_join against a stale pthread_t. Documenting the cause without the consequence means whoever debugs a mysterious mutex hang later won't connect it back here. One more sentence would do it.
• inotify watches silently dropped in clones — unchanged.
• images.py PSTREE decode change — unchanged; still an unguarded compat break with upstream crit images.
• read_one_pstree_item error paths — the goto err sites still leave the item linked into the pstree, and now leak pi->threads too. Also still worth confirming ret is initialized at declaration, and that __pstree_insert_pid does its own lookup when passed NULL, NULL.

---

Good iteration. Reverting the SIGCHLD suppression in particular moves this from "makes forks succeed" toward "makes forks correct." argc_max is the one I'd resolve first, since it suggests this commit may not have been compiled.

[yiying-zhang]

Re-review: Address tfork single-copy review follow-ups

All four items from the last round are addressed, and the argv_max question is settled. One new off-by-one in the pstree unwind path, described below.

---

🔴 New: thread-cleanup loop bounds don't match the counter

err:
	if (ret < 0) {
		for (i = 1; i <= inserted_threads; i++)
			pstree_remove_pid_if_linked(&pi->threads[i]);

The insertion loop is for (i = 0; i < e->n_threads; i++) { ...; inserted_threads++; }, with every goto err inside it occurring before the increment. So successfully inserted threads occupy indices 0 .. inserted_threads - 1. The cleanup walks 1 .. inserted_threads, which is off by one at both ends:

• Index 0 is never cleaned up. The first thread's struct pid stays linked in the uid / leaf-ns / root-ns rb-trees after the failure — which is the exact leak this commit is fixing.
• Index inserted_threads is touched but was never initialized. pi->threads comes from xmalloc (not zeroed), so pstree_remove_pid_if_linked reads garbage uid, leaf_ns_id, local, and real.

The second one is worse than an uninitialized read, because of this line in the helper:

if (pid_node->leaf_ns_id != ALL_PID_NS_ID) {
        found = __lookup_pid_leaf(&pid_root_rb[pid_node->leaf_ns_id], ...);

A garbage leaf_ns_id indexes pid_root_rb[] out of bounds and hands a wild pointer to __lookup_pid_leaf. The found == pid_node guard prevents a bogus erase, but not the bad read that precedes it.

Should be:

for (i = 0; i < inserted_threads; i++)
        pstree_remove_pid_if_linked(&pi->threads[i]);

If index 0 is deliberately excluded because threads[0] aliases pi->pid, then inserted_threads shouldn't be counting it — worth making that explicit either way, since the two readings disagree about what the counter means.

🟡 Smaller notes on the same hunk

• pi itself still leaks on the error path. linked, pid_inserted, and threads_allocated all unwind, and root_item is nulled, but the pstree_item allocated by get_or_create_pstree_item is never freed. Low impact since CRIU is about to bail, but it's the one piece of the object not unwound.
• get_or_create_pstree_item can return a pre-existing item (the found branch). In that case the unwind would tear down state belonging to an item parsed earlier — pid_inserted would be false so the pid node survives, but threads_allocated would free and NULL a pi->threads the earlier caller may still expect. Worth confirming that path can't be reached with a partially-constructed reused item.
• argc_new + 1 >= argv_max compares int against size_t; the operand promotes, and argc_new is a non-negative counter so it's safe, but it may draw -Wsign-compare depending on the build flags.
• Unbounded EINTR retry in the tfork branch — continue doesn't advance waited_ms, so a signal storm spins without hitting the 500 ms cap. Almost certainly academic; noting it only because the surrounding loop is otherwise carefully bounded.

---

Still open from earlier rounds

Unchanged, and all previously acknowledged — listing only so they don't fall off:

• inotify watches silently dropped in clones
• images.py PSTREE decode change (unguarded compat break with upstream crit images)
• fresh thread TIDs — now well documented, still the behavior

---

The argv_max fix confirms the build wasn't run on the previous commit; the thread-cleanup off-by-one above is the same class of thing and would be caught immediately by a ZDTM run that exercises a pstree parse failure. Worth getting a build + targeted test in the loop before the next push.

[yiying-zhang]

Re-review: Fix tfork pstree unwind bounds

Everything from the last round is addressed correctly. Remaining notes are verification questions about premises I can't see in the diff, not defects I can point at.

---

Resolved

• Thread-cleanup bounds — now for (i = 1; i < next_inserted_thread; i++), with next_inserted_thread initialized to 1 and set to i + 1 only after a successful insert. Walked the cases and the arithmetic is right: fail before the loop → cleans nothing; fail at i == 1 → cleans nothing (index 1 never inserted); fail at i == 2 → cleans index 1 only. No uninitialized slot is touched, which also closes the wild pid_root_rb[] index.
• leaf_ns_id bounds check — valid_leaf_ns gates the leaf-tree lookup. Belt-and-braces given the above, but correct to have.
• Sign-compare — (size_t)argc_new + 1 >= argv_max.
• Reused-item hazard — get_or_create_pstree_item now reports created, and the !created_item && (pi->threads || pi->nr_threads) guard rejects a partially populated reuse before any unwindable state is built. The comment explaining why the unwind can't tear down an already-parsed item is the right thing to have written down.
• pi leak — if (created_item) xfree(pi), correctly ordered after all the unlink steps and after the root_item == pi reset.
• Unbounded EINTR retry — now && waited_ms < 500 with the sleep/accumulate inside the branch, so it shares the same cap as the ECHILD path.

---

Worth confirming before merge

1. Does the thread loop actually skip inserting index 0?

The new comment states the premise the fix rests on:

threads[0] is the leader mirrored by pi->pid. Only non-leader threads are inserted independently.

If that holds, starting cleanup at i = 1 is exactly right. But the loop body visible in the hunk runs insert_status for every i including 0, and pi->threads[0] is a separate struct pid object from pi->pid — "mirrored" isn't the same as "identical storage". If index 0 does get linked into the rb-trees on its own, pstree_remove_pid_if_linked(pi->pid) won't unlink it and it leaks on the error path.

The part of the loop that would settle this isn't in the diff. Worth a quick check, since it's the one assumption the whole unwind depends on.

2. Is xfree(pi) the right free for what get_or_create_pstree_item allocated?

Depends on whether pi->pid is a separate allocation or lives inside the same block as the item. If CRIU's allocator hands back item+pid together, xfree(pi) is fine; if pid is allocated separately, this leaks it, and the matching destructor (free_pstree_item() or equivalent) should be used instead. Hand-rolled xfree against a non-trivial constructor is the usual place this goes wrong.

3. Two small type questions in pstree_remove_pid_if_linked

bool valid_leaf_ns = pid_node->leaf_ns_id >= 0 && pid_node->leaf_ns_id <= max_ns_id;

• If leaf_ns_id is unsigned, >= 0 is always true and may draw -Wtype-limits. (Elsewhere it's printed with %d, which suggests signed — just worth being deliberate.)
• <= max_ns_id assumes pid_root_rb[] has at least max_ns_id + 1 entries. Correct if max_ns_id is the highest allocated id; off by one if it's the array size.

Also if (pid_node->uid > 0) skips the uid-tree unlink for uid == 0. Fine if 0 means "unset", worth confirming it's not a valid uid.

---

Still open from earlier rounds

Unchanged and previously acknowledged, listed only so they don't drop off: inotify watches silently dropped in clones, the images.py PSTREE decode compat break, and the fresh-thread-TID behavior (now well documented).

---

Converging nicely. Nothing here blocks on its own — item 1 is the only one that could still be an actual leak, and it's a two-minute check.

[yiying-zhang]

Re-review: Fix tfork pstree unwind bounds (b1416e6)

Small commit, and it resolves all three verification items from the last round. Nothing new is broken.

One optional tightening

Since pi now stays in the arena after a failed parse, it survives with pi->pid->state == TASK_ALIVE (set before the failure point). The unwind clears pi->threads / pi->nr_threads but not the state. That item should be unreachable — pid_inserted unwinding removes it from pid_root_rb, so get_or_create_pstree_item won't find it again — but if it ever were reached, it would trip BUG_ON(pi->pid->state != TASK_UNDEF) and abort rather than hit the clean Refusing to reuse partially populated pstree item path.

Resetting pi->pid->state = TASK_UNDEF in the unwind would make the item's state consistent with the rest of the teardown and keep that failure mode a diagnostic instead of a BUG_ON. Defensive only — I don't see a reachable path today.

---

Still open from earlier rounds

Unchanged and previously acknowledged: inotify watches silently dropped in clones, the images.py PSTREE decode compat break, and the fresh-thread-TID behavior (documented).

The pstree unwind path looks correct to me now. Remaining items are the three long-standing behavioral ones, none of which are regressions introduced by this PR's later commits.

[yiying-zhang]

Pre-merge checklist — os4agent #2

Consolidating what's still open. Nothing here is a known-broken finding; it's one verification gap plus three cheap cleanups. Ordered by what I'd do first.

---

1. Verify the thread-TID change end to end (blocking)

criu/criu/pie/restorer.c:

if (args->tfork_active) {
        /*
         * Preserve the TID visible in the clone's innermost PID namespace.
         * Outer namespace TIDs are allocated by the kernel so concurrent
         * copy helpers cannot collide with each other on the host.
         */
        c_args.set_tid_size = 1;
}

This reads correctly against the clone3 contract — set_tid[0] is the innermost namespace, tid_in_ns[0] is the innermost in this tree — and it's a real fix rather than a documented breakage. But it changes thread identity in a checkpoint/restore path, and the two compensating hacks that used to absorb a mismatch (the ret != thread_args[i].pid remap and the acceptance branch in __export_restore_thread) were removed in the same commit. If the assumption is wrong, restore now fails hard rather than limping.

Suggested verification:

• ZDTM threads and pidns groups
• one live fork of a process holding a robust or errorcheck mutex — that's the case that exercises whether the TID in the futex owner word still matches the restored thread
• confirm gettid() inside the clone matches the pre-fork value for a non-leader thread

Small hardening while you're in there: the tfork branch forces set_tid_size = 1 unconditionally, while the code it overrides uses ns_level. At ns_level == 0, upstream's 0 means "don't set"; this reads tid_in_ns[0], which may be uninitialized. A container task should always be ns_level >= 1, so this is likely unreachable — but the guard makes the assumption explicit instead of load-bearing:

if (args->tfork_active && thread_args[i].ns_level > 0) {

2. Confirm io is imported in images.py

The commit adds only from google.protobuf.message import DecodeError, but the new pstree_handler.loads() / dumps() both use io.BytesIO. struct is clearly already imported (the existing entry_handler uses it); io is less certain from the diff alone.

This self-answers the moment test_pstree_compat.py runs — it would fail immediately with NameError. Mentioning it only because the compat test is new and may not have been executed yet.

3. Explain the inotify revert in the PR description

criu/criu/fsnotify.c is reverted exactly to its pre-PR blob, so mark replay happens normally again and the file drops out of the PR diff entirely. That's the right outcome — clones get their watchers back.

But "Skip inotify mark replay for tfork restores" was presumably added to get past a real failure. Please state what changed such that replay works now (if it's a consequence of the thread-TID fix, that connection is worth spelling out — it isn't obvious). If the revert was assumed rather than tested, the failure mode has flipped from "clone runs with dead watchers" to "restore fails outright", which is better as a default but is still a behavior change that should be deliberate.

Worth one live fork of a container running something with an active watcher — a dev server or inotifywait — to confirm.

4. Make the PSTREE format discrimination explicit

pstree_handler.load() distinguishes legacy from file-level images by attempting a legacy parse and falling through on DecodeError or not IsInitialized(). That second condition only does real work because pstree_entry is proto2 with required fields; if it ever moved to proto3, IsInitialized() becomes a constant True and a file-level image that happens to parse as a legacy entry would be silently misread.

dump() already discriminates structurally:

file_level = ('tree' in entries[0] or 'ns_max_pids' in entries[0])

Either mirror that on the load path, or add a comment at the IsInitialized() check noting the proto2-required-fields dependency. The asymmetry between a structural check on write and a parse-attempt check on read is what will confuse the next reader.

Also: load() accepts no_payload and ignores it. Fine for PSTREE — the payload is the entry — but worth a one-line comment since other handlers honor it.

5. Nit: Makefile prerequisite ordering

run: pstree-compat start

Both are prerequisites, so under make -j they run concurrently. Harmless (the compat test doesn't touch the service), but if the intent is "check compat before bothering to start the daemon", that needs an explicit ordering.

---

Not open, listed for completeness

These came up in earlier rounds and are resolved — no action needed:

• container→host allowlist bypass, symlink write, authority scoping (that's gensee-crate #23, separate PR)
• argc_max typo and the argv overflow check placement
• pstree unwind bounds, arena xfree, TASK_UNDEF reset
• SIGCHLD suppression (reverted), barrier timeout diagnostics, pr_warn → pr_debug
• EINTR gating, build.sh stale-module abort, ext_unix_sk opt-in