nspawn: Stop overmounting /sys and /proc when a user namespace is used

[daandemeyer]

When the container runs in a user namespace, we don't need to protect /proc and /sys by overmounting things. In fact this is actively harmful as it prevents nested systemd-nspawn from working as to mount procfs and sysfs in a container it cannot be overmounted or the kernel will refuse the mount.

To make nesting possible, let's stop overmounting parts of /proc and /sys when user namespaces are in use.

[daandemeyer]

@claude review this pull request

[github-actions]

Claude review: 1 must-fix, 5 suggestion(s)

[github-actions]

Claude: must-fix: setup_boot_id() skips generating a unique boot ID when in a user namespace, meaning the container will inherit the host's boot ID. This is a functional regression — containers sharing the host boot ID can confuse systemd-journald and other services that use boot ID to distinguish boots. If bind-mounting over /proc/sys/kernel/random/boot_id is not possible in a user namespace, this should at minimum be documented with a comment explaining why skipping is acceptable, or an alternative mechanism should be used.

[github-actions]

Claude: suggestion: In both setup_boot_id() (here) and setup_kmsg() (line 2565), the pattern if (r != 0) return r; returns true (1) on the success-skip path from functions that conventionally return 0 on success or negative on error. While the callers only check r < 0, propagating a positive value from a setup function is non-idiomatic. Consider separating the error and skip cases:

r = container_in_userns();
if (r < 0)
        return r;
if (r > 0)
        return 0;

[github-actions]

Claude: suggestion: When namespace_is_init() returns -EBADR or -ENOSYS, the error is silently swallowed and the function falls through to return r == 0, which evaluates to false ("not in a user namespace"). The -ENOSYS case (procfs not mounted / not available) could indicate a constrained environment where assuming "not in a user namespace" may be incorrect. Consider whether these error cases should default to "assume we're in a user namespace" for safety, or at minimum log a debug/warning message so the fallback behavior is observable.

[github-actions]

Claude: suggestion: Clearing MOUNT_APPLY_APIVFS_RO disables not just read-only sysfs/procfs mounts but also all PROC_INACCESSIBLE_REG and PROC_READ_ONLY overmounts in mount_all() — entries like /proc/kallsyms, /proc/kcore, /proc/keys, /proc/sysrq-trigger, /proc/timer_list, and directories like /proc/acpi, /proc/bus, /proc/scsi are left exposed. While the kernel does restrict access to most of these in user namespaces, some entries like /proc/kallsyms (depending on kptr_restrict sysctl) may still leak kernel address information. The commit message and comments should explicitly acknowledge that these masking overmounts are also being removed, not just the read-only flags.

[github-actions]

Claude: suggestion: container_in_userns() calls namespace_is_init() (a stat() syscall on /proc/ns/user) every time it is invoked, and it is called in 5 different places (verify_arguments, setup_boot_id, setup_kmsg, run_container, and indirectly via mount_sysfs). Since the result cannot change during the process lifetime, consider computing this once in verify_arguments() and encoding the result into a flag (e.g., in arg_mount_settings), rather than re-checking the namespace status at each call site.

[github-actions]

Claude: suggestion: When is_mount_point is true AND the existing mount is already sysfs (checked in the block above), the function returns 0 at line 465 before reaching the new !FLAGS_SET(mount_settings, MOUNT_APPLY_APIVFS_RO) check. This means if /sys is already mounted as sysfs and we're in the user namespace path, we return without ensuring it's read-only. The comment at line 468 says "we still have to mount it read-only" but this early-return path doesn't enforce that. Is this intentional?

[daandemeyer]

@claude review this pr

[github-actions]

Claude review: 3 must-fix, 1 suggestion(s), 1 nit(s)

[github-actions]

Claude: must-fix: setup_boot_id() uses if (r != 0) return r; which returns true (1) when container_in_userns() indicates a user namespace. Functions like setup_boot_id() follow the 0-on-success / negative-on-error convention. The current callers only check r < 0 so this works by accident, but returning a positive value is non-idiomatic and fragile — any future caller checking r > 0 or r != 0 would misinterpret the result. Please split the check:

r = container_in_userns();
if (r < 0)
        return r;
if (r > 0)
        return 0;

[github-actions]

Claude: must-fix: Same issue as setup_boot_id(): setup_kmsg() returns 1 (true) via if (r != 0) return r; on the user-namespace skip path. Should be:

r = container_in_userns();
if (r < 0)
        return r;
if (r > 0)
        return 0;

[github-actions]

Claude: must-fix: When /sys is already a mount point with sysfs (the is_mount_point && is_sysfs path), the function returns 0 here before reaching the new !FLAGS_SET(mount_settings, MOUNT_APPLY_APIVFS_RO) check at line 469. In the user namespace case MOUNT_APPLY_APIVFS_RO is cleared, so the intent is to mount sysfs read-only directly — but this early return bypasses that entirely. The container ends up with whatever mount flags sysfs already had, contradicting the comment at line 468 that says "we still have to mount it read-only". The early return should either be moved below the new userns check, or this path should explicitly handle the read-only remount.

[github-actions]

Claude: suggestion: container_in_userns() calls namespace_is_init() (a procfs stat) on every invocation, and it is called from 4 separate sites (verify_arguments, setup_boot_id, setup_kmsg, run_container). Since the user namespace status cannot change during the process lifetime, consider computing this once in verify_arguments() and storing the result in a flag (e.g., extending arg_mount_settings or a dedicated static variable) to avoid redundant syscalls and the repeated error-handling boilerplate at each call site.

[github-actions]

Claude: nit: bool is_mount_point is declared at the top but not initialized until several lines later. It could be declared at the point of initialization for clarity:

bool is_mount_point = r > 0;

[github-actions]

Claude review of PR #8 (5070e78)

Must fix

• setup_boot_id returns positive value — src/nspawn/nspawn.c:2152 — if (r != 0) return r; returns 1 (true) to callers expecting 0-on-success convention; should split into separate error/skip checks
• setup_kmsg returns positive value — src/nspawn/nspawn.c:2566 — Same if (r != 0) return r; issue as setup_boot_id
• mount_sysfs early return bypasses userns read-only path — src/nspawn/nspawn-mount.c:465 — When sysfs is already mounted, the function returns before reaching the new !FLAGS_SET(MOUNT_APPLY_APIVFS_RO) check, so the read-only mount intended for user namespaces is never applied

Suggestions

• Silent error swallowing in container_in_userns — src/nspawn/nspawn.c:1653 — -EBADR/-ENOSYS errors are silently ignored, defaulting to "not in userns" which may be incorrect; consider at least a debug log
• Cache container_in_userns result — src/nspawn/nspawn.c:1645 — Called from 4 sites but result is invariant; compute once and store

Nits

• Declare is_mount_point at initialization — src/nspawn/nspawn-mount.c:453 — Could be bool is_mount_point = r > 0; at point of use

[daandemeyer]

@claude review this pr

[daandemeyer]

@claude review this pr