Skip to content

install: Initialize separately mounted /var filesystems from the image - #2

Draft
jmarrero-bot wants to merge 8 commits into
bot/defer-var-markerfrom
bot/defer-var
Draft

jmarrero-bot wants to merge 8 commits into
bot/defer-var-markerfrom
bot/defer-var

Conversation

@jmarrero-bot

@jmarrero-bot jmarrero-bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Fork note, to remove before posting: this is the next revision of bootc-dev#2521, to be force-pushed there. The title and the text below are written as bootc-dev#2521's new description. jmarrero's two commits come first and stay his (author and trailers kept). Rebased onto main and onto the --defer-var PR (#5), they changed as follows. The Features: list code in cli.rs and its test now come from #5, so his commit only adds its FEATURES entry. His commit message changed: it now says the option is listed as initialize-var-mounts in the Features: list of bootc --version, instead of that --version "now prints" the list, and the container inspect rationale moved to #5's commit. In the tmt test, his commit adapts #5's --defer-var case to his nested volume: it reformats that volume too, and checks that the image's files are missing instead of checking that the /var volume is empty. It also drops #5's mkfs.vfat -i UUID preservation, which his boot-partition UUID fix makes unnecessary. In this revision his commits are unchanged apart from that restack. The rest is context.

A common setup puts /var, or parts of it such as /var/log, on separate filesystems, for example LVM logical volumes. Hardening profiles such as DISA STIG and CIS ask for this. With bootc install to-filesystem, these filesystems currently end up empty. The deployment backend seeds the image's /var into its own state directory, and at boot the separately mounted filesystems hide that content. This was reported in osbuild/bootc-image-builder#1222. This is the second part of bootc-dev#2599. It is stacked on the --defer-var PR (#5 until that is opened upstream), which also has the composefs setup-root change that keeps a deferred /var volume in fstab from being hidden.

This series:

  1. tmt: Use the boot partition UUID in the separate /var test. An independent fix.
  2. install: Add --initialize-var-mounts for separate /var filesystems. On a fresh to-filesystem install, bootc discovers the filesystems mounted at the target's /var or below and fills the empty ones from the image, then SELinux-labels and syncs them. The option is opt-in, and it is listed in the Features: list of bootc --version.
  3. lsm: Leave paths the policy labels <<none>> as they are. A prep fix. Labeling failed on paths the policy has no label for, such as /var/tmp/.*, so image content there failed the install. Like setfiles and restorecon, such a path now keeps its label. A file with no label at all gets its directory's, as a file created there would. So on ostree such a file keeps the label of the deployment checkout's file (system_conf_t for /var/tmp content), and on composefs it usually gets its directory's (tmp_t). With no policy loaded, labeling is skipped with a debug message, and policies are loaded one at a time, since loading sets libselinux's process-wide policy root.
  4. install: Initialize /var mounts from the image, per filesystem. This copies from the image's /var (the deployment checkout on ostree, the mounted image on composefs) instead of from the seeded copy. Each empty filesystem gets its own cp -a, run in a private mount namespace where the mountpoints of the filesystems nested in it are hidden, so each file lands on the filesystem that holds it at boot. A filesystem that already has data is left alone, but its empty parent is still filled. Hardlinks are kept within a filesystem on composefs. On ostree they become separate files, because a commit doesn't record them. A failed copy fails the command with an error that names the filesystem and says it may be partly filled and won't be filled again on its own. Filesystems completed before the failure are still labeled and synced, each on its own, and their errors are reported after the copy's. A lost+found that something is mounted on is left to that filesystem. Later runs say which filesystems they skip. Fills, skips and failures are logged to the journal.
  5. install: Initialize a deferred /var with --initialize-var-mounts. With --defer-var, the state directory gets only what isn't on a mounted filesystem, so nothing is stored twice. Errors for it name it as /var, the deployment's state directory, and a failed copy there says to install again onto a fresh root.
  6. cli: Add bootc state initialize-var. This runs the same routine on a booted system, for example for a volume added later. It only fills block-device filesystems mounted from their root. Other mounts, like a container's tmpfs, still count as boundaries, so an empty volume with a tmpfs below it is filled around it.
  7. composefs: Leave /var to fstab after --initialize-var-mounts filled it. When a filesystem is mounted on the target's /var, bootc writes a second marker, .bootc-var-mounts, into the stateroot var, and setup-root accepts it like install: Add --defer-var to leave the stateroot /var unseeded #5's .bootc-defer-var. Without it, on composefs --initialize-var-mounts alone fills the /var volume, and setup-root then bind-mounts the seeded state directory over it. Reusing .bootc-defer-var would also stop composefs from seeding the state directory. An initramfs older than this ignores the marker after a switch or rollback, like .bootc-defer-var.
  8. tmt: Cover --defer-var --initialize-var-mounts in the separate /var test.

Only empty filesystems are filled, and nothing is merged into existing data. A volume left empty because the image has nothing for it still counts as not initialized, so a later run copies the image's empty directory onto it again and relabels its root. The man page says so. Skipping it would need a record of the first run on the volume or in the state directory, and putting state on a caller's volume seemed worse than an idempotent re-copy. Subsequent image updates don't update /var. Callers remain responsible for mounting these filesystems at boot.

Tested at 2139e25 on a fresh 16-core RHEL 10 devspace (jmarrero-devspace-40915942997). The images report package bootc-202610291722.g2139e251c3-1.el10. Since then only tests changed: static checks, unit tests and mutations ran again at ce2a1e1 (jmarrero-devspace-40932828423). Logs: https://gist.github.com/jmarrero-bot/cb0ed128e38cab8cc220d7b21e63de0b (rounds 9 and 10).

  • cargo fmt --check, clippy (Makefile flags), cargo doc with -D warnings, cargo check --no-default-features (lib), cargo xtask update-generated direct --check, and update-generated from-code with no diff. cargo test -p bootc-lib -p bootc-initramfs-setup: 358 + 16 passed, and the default bootc-units container run passed. cargo check --tests passes at each commit.
  • The labeling tests (relabel_none, finish_labels_and_syncs) build a policy from a file_contexts file, so they need no host policy. They skip where this process can't both set and remove labels, judged from the raw extended attribute only, which includes the default just unit-tests container on SELinux hosts. Otherwise they assert that bootc's own classification sees the result as unlabeled, so a regression there fails rather than skips them. So upstream CI can't be counted on to run them, and there only tmt covers <<none>>. In the container with SYS_ADMIN and spc_t, they now skip, where 5875cfc failed.
  • Reverting each fix in turn makes a test fail: finish() doing nothing, stopping at the first error, dropping later errors, syncing only after a successful label, not labeling the stateroot var, the real finisher not labeling, not syncing, or labeling the stateroot var by its own path, the stateroot var's failure text, labeling a mounted-on lost+found, day two filtering the boundaries, the scope message, hide_target, writing or reading the second marker, and relabel() failing on a <<none>> path, overwriting its label, or not inheriting one, and the unlabeled type never being looked up. Not copying an unlabeled parent's label is only observable where labels can be removed (not on an SELinux host).
  • tmt plan-32-install-to-filesystem-var-mount covers --initialize-var-mounts with a nested volume, the same install without it, --defer-var --initialize-var-mounts without a /var volume, and --defer-var alone. It checks that .bootc-var-mounts is written exactly when a volume is mounted on /var, and the test image has a file under /var/tmp, which the policy labels <<none>>. Its directory must get /var/tmp's label on both backends. It PASSED on ostree/grub and composefs grub ext4 BLS. It fails on composefs without the label inheritance, and in round 8 it failed without the install-time marker call (composefs) and without the <<none>> fix (both backends, with and without a /var volume). composefs systemd-boot xfs UKI (unsealed) last ran at 0438277 and passed.
  • VM cases at 55fd0e5 (bcvk install onto LVM, qemu boot, SELinux enforcing; composefs on UEFI with OVMF). Since then only the labeling of <<none>> paths and of a mounted-on lost+found changed, which tmt and the unit tests cover:
    • ostree and composefs --defer-var --initialize-var-mounts with nested volumes and no /var volume: content, owners, modes, labels, and no duplicate in the stateroot var. Then bootc state initialize-var on a new volume. A forced failure (immutable target) gives the error, and a journal entry with MESSAGE_ID=a3dbd7d2…. A re-run names each filesystem it skips. bootc switch to v2 and rollback keep the volumes' data on both backends.
    • The STIG shape, a /var volume with nested volumes and --defer-var --initialize-var-mounts, on ostree and composefs: all checks pass through switch and rollback. On composefs /var is the volume at boot.
    • Day two on ostree with two new volumes, the second immutable: the first is filled and the command fails on the second. After syncing only the root filesystem and a hard power-off, the first volume keeps its content and labels.
    • composefs --initialize-var-mounts alone, with an overlay /etc whose image fstab mounts /var: the stateroot var has .bootc-var-mounts, setup-root leaves /var to fstab, and /var is the volume at first boot. It stays the volume after a metadata-only copy-up of fstab (overlay.metacopy=Y). An upper fstab without the /var line, and a whiteout, both give the state directory's bind mount. This case ran with enforcing=0, because an overlay /etc comes up unlabeled_t under enforcing SELinux, also on main (draft upstream issue to follow).

Open questions:

Closes bootc-dev#997, closes bootc-dev#1615. Related: bootc-dev#336, bootc-dev#1727, osbuild/image-builder#2733

Generated-by: https://github.com/jmarrero-bot#llms


Review draft in jmarrero-forge, not upstream yet. This section is removed when the PR is opened upstream.

  • Upstream: bootc-dev/bootc, base main
  • Board item: PVTI_lADOFBIZ7c4BlZPrzhAZed0
  • Fork CI: off; the devspace testing described above is this PR's CI, and upstream CI runs once it is opened there

To review:

  • Approve, or comment /promote on a line of its own, to open it upstream, ready for review. Either covers only the commits pushed so far.
  • If upstream requires DCO, approving also signs off: promote adds Signed-off-by: Joseph Marrero Corchado <jmarrero@redhat.com> to the commits lacking it (the bot's and yours; anyone else's only if you ask), with you as committer.
  • Add a /draft line (in the same comment or before) to open it upstream as a draft (/ready undoes that).
  • Close to drop it.
  • Edit the title and description freely: they become the upstream PR's. Review comments are squashed into the commits they concern, with a reply here.

Bot run: session e7bb5c51, agents a819c411 +2 · claude-opus-5-5 102.5M in (97% cached) / 249k out · ~$41.99 inference + ~$4.99 compute (est., list prices) · 2h 5m wall · 31.7 core-h, Actions run 40714907801

Bot run: session e7bb5c51, agents a819c411 +2 · claude-opus-5-5 223.6M in (95% cached) / 378k out · ~$108.13 inference + ~$11.52 compute (est., list prices) · 17h 57m wall · 73.1 core-h, Actions runs 40832841988, 40714907801

Bot run: session e7bb5c51, agents a6341f71 +1 · claude-opus-5-5 30.0M in (97% cached) / 137k out · ~$12.66 inference + ~$2.58 compute (est., list prices) · 15h 54m wall · 16.4 core-h, Actions runs 40848700131, 40739864348

@jmarrero-bot jmarrero-bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Review guide for head 83fe4960e0: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.

Prototype stacked on bootc-dev#2521 (the first two commits are that PR and are not reviewed here). It replaces bootc-dev#2521's copy-from-the-seeded-stateroot-var with one cp -a per empty filesystem from the image's /var, run in a private mount namespace that covers other filesystems' mountpoints with tmpfs; adds --defer-var (ostree: write libostree's .ostree-selabeled stamp so prepare_deployment_var() skips seeding; composefs: skip the initial copy); adds bootc state initialize-var for booted systems; and makes composefs setup-root skip its /var bind mount when fstab has /var. The mount-namespace code is sound (allocation-free pre_exec, rprivate before mounting, fails closed). The main risks: the day-two command scans every mount under /var, including empty tmpfs mounts owned by podman or kubelet, and relabels their roots; using libostree's stamp also turns off libostree's relabel of the stateroot var root, which --defer-var alone never redoes; and the setup-root fstab change switches /var to the fstab volume on existing composefs systems that have run on the state directory's var until now. Unit tests, clippy, a VM matrix and tmt were run on a devspace. tmt covers install only; day two and boot are covered only by the gist's VM matrix.

Hotspots

  1. risky · security — crates/lib/src/install/var_mounts.rs:481-487 (cd8c3ae): On a booted host, discover("/") also returns pseudo and runtime mounts under /var, such as podman's per-container shm tmpfs (userdata/shm) or kubelet emptyDir and secret tmpfs. An empty one passes is_empty_fs, the image has no content for it, and label() then relabels its root or fails on a context= mount. Restrict this to block-backed filesystems, or require PATH to be a mountpoint.
  2. look-closely · logic — crates/lib/src/install/var_mounts.rs:394-397 (b0d2f30): A mount the image has no content for is still added to prepared.mounts, so it gets relabeled and synced. That is fine at install time, but on day two it is how foreign empty mounts get relabeled (see above).
  3. look-closely · security — crates/lib/src/install/var_mounts.rs:290-322 (b0d2f30): pre_exec hook: unshare(NEWNS), then rprivate /, then tmpfs mounts. It makes no allocations and fails closed before mounting anything. A failure surfaces as a bare errno with no hint of which path failed to mount.
  4. note · logic — crates/lib/src/install/var_mounts.rs:260-277 (2ba316c): Hidden mountpoint directories in the stateroot var are copied from the tmpfs root, so they carry the tmpfs's SELinux xattr until label() relabels the stateroot var. That relabel is unconditional, so the result is correct, but only because of it.
  5. look-closely · logic — crates/lib/src/install/var_mounts.rs:448-467 (2ba316c): libostree's stamp also suppresses selinux_relabel_var_if_needed(), which normally labels the stateroot var root as /var. With --defer-var alone, label() never runs, so the root keeps the physical-path label from the final walk (usr_t on ostree, default_t on composefs) until tmpfiles fixes it at boot.
  6. note · logic — crates/lib/src/install.rs:2233-2240 (2ba316c): initialize_var holds only with --initialize-var-mounts, so --defer-var alone skips populate() and label() entirely. That is where the stateroot var root label gap above comes from.
  7. look-closely · logic — crates/lib/src/install/var_mounts.rs:104-147 (b0d2f30): The emptiness rules decide what gets overwritten. If a cp fails partway, the filesystem is left non-empty, and the next run prints Preserving and skips it, leaving a half-initialized volume. That matters for the day-two command.
  8. risky · api — crates/initramfs/src/lib.rs:680-694 (3a61f24): This changes behavior for existing composefs systems: one whose fstab has /var has been running on state/os/default/var, since the bind mount won. After an update to this initramfs it boots on the fstab volume instead, so its /var data appears to vanish. Call this out, or gate it.
  9. note · logic — crates/initramfs/src/lib.rs:522-534 (3a61f24): openat follows symlinks, so an absolute symlink at the state's etc/fstab would resolve inside the initramfs rather than the deployment. This is an unlikely edge case.
  10. note · test-gap — tmt/tests/booted/test-install-to-filesystem-var-mount.sh:293-333 (83fe496): The tmt test covers install only. The day-two command, the setup-root fstab change and booting a deferred /var are covered only by the gist's VM matrix, and the --defer-var-alone case never checks labels.

Safe to skim

  • docs/src/man/bootc-install-to-filesystem.8.md: Prose matching the code; the options section is generated
  • docs/src/man/bootc-setup-root-conf.5.md: One doc paragraph
  • crates/lib/src/bootc_composefs/boot.rs: Passes the new flag through
  • crates/lib/src/bootc_composefs/update.rs: Passes false for the new flag
  • crates/lib/src/install/baseline.rs: New struct field default
  • tmt/tests/tests.fmf: Duration bump, regenerated
  • crates/lib/src/install.rs:3426-3480: Table test rewrite for check_var_options

@jmarrero-bot jmarrero-bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Needs rework at 83fe496 (reviewed only 5f31d97..83fe496).

  • P2 — wrong fstab view: crates/initramfs/src/lib.rs:685-695 always prefers state etc/fstab, including with [etc] mount = "none" or "transient", where that file is not the booted /etc. A stale state /var entry can suppress the bind mount while systemd sees no /var entry, leaving /var read-only; the reverse hides the intended volume. Mount/configure /etc first, then inspect its effective fstab through visible_root. Do not fall back to the image fstab when the effective bind-mounted /etc has deleted it. Add tests with conflicting state/image fstabs and these /etc modes.
  • P2 — lost mountpoint metadata: crates/lib/src/install/var_mounts.rs:262-277,301-308 reconstructs hidden source directories with only mode/uid/gid. The deferred stateroot placeholders consequently lose image xattrs/default ACLs (and timestamps); I reproduced user.image and system.posix_acl_default disappearing on the devspace. Preserve the original directory metadata on the placeholders, or prepare fully attributed empty surrogate directories before spawning cp; add an xattr/default-ACL assertion to the namespace test.

The per-filesystem/hardlink deviations and empty-only policy are reasonable prototype choices; the stamp matches libostree’s current prepare_deployment_var() and relabel checks. The pre_exec hook uses preallocated C strings and isolates propagation. Reran fmt, diff/commit hygiene and shell syntax locally; on the existing devspace, clippy (Makefile flags), 303 bootc-lib tests, 11 initramfs tests and the ignored namespace test passed. Reviewed the linked VM/tmt evidence; existing tests miss the two cases above.

@jmarrero-bot

Copy link
Copy Markdown
Author

Review fixes pushed as 859eb94, squashed into the commits they belong to:

  • bootc state initialize-var only considers block-device filesystems; container tmpfs, rpc_pipefs and NFS mounts under /var are skipped (cli commit).
  • --defer-var: the stateroot var's root is labeled as /var even when nothing is copied into it (install --defer-var commit; checked by the od VM case and the tmt test). Mountpoint placeholders in a deferred stateroot var now get the image directory's owner, mode, xattrs (label, ACLs) and timestamps (same commit; checked by the namespace test).
  • setup-root reads the fstab that /etc will show: the state's for bind, the state's then the image's for overlay, the image's for none or transient (composefs commit). That commit and the body now say that existing composefs systems with a /var fstab entry change behavior.
  • Copy errors name the hidden paths (first commit).

Left as open questions in the body: an interrupted copy, the .ostree-selabeled stamp versus a bootc marker or libostree option, and read_optional following symlinks (not changed). Unit tests, clippy, the namespace test, the four affected VM cases and the tmt plan on both backends pass at 859eb94; details in the body and the gist.

@jmarrero-bot jmarrero-bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Review guide for head 859eb94349: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.

Prototype stacked on bootc-dev#2521 (the first two commits are that PR and are not reviewed here). It replaces bootc-dev#2521's copy-from-the-seeded-stateroot-var with one cp -a per empty filesystem from the image's /var, run in a private mount namespace that covers other filesystems' mountpoints with tmpfs; adds --defer-var (ostree: write libostree's .ostree-selabeled stamp so prepare_deployment_var() skips seeding; composefs: skip the initial copy); adds bootc state initialize-var for booted systems, limited to block-device filesystems; and makes composefs setup-root skip its /var bind mount when the fstab /etc will show has /var. The namespace code is sound. This head fixes the earlier review's findings: the day-two scan skips tmpfs and network mounts, a deferred stateroot var root is labeled as /var, deferred mountpoint placeholders get the image directory's metadata, and the commit message documents the change for existing composefs systems. What remains is smaller. Bind mounts of block filesystems still count as block-backed, and the day-two command still labels empty mounts the image has no content for. copy_dir_metadata is stricter about xattrs than cp -a. setup-root's fstab read errors on an overlay whiteout. Unit tests, clippy, the VM matrix and tmt (ostree and composefs BLS) were run at this head, per the PR body and gist.

Hotspots

  1. look-closely · security — crates/lib/src/install/var_mounts.rs:99-101 (21f9da7): Bind mounts of block filesystems (source /dev/X[/path]) still count. An empty one, such as a kubelet subPath bind under /var/lib/kubelet/pods, would be treated as a fresh filesystem, and since the image has no content for it, its root would be relabeled. Consider requiring the filesystem root (no [..] in the source) for everything but /var itself.
  2. note · logic — crates/lib/src/install/var_mounts.rs:454-457 (5b52260): A mount the image has no content for is still labeled. That is right at install time, but on day two it is the only reason a foreign empty mount gets touched. Skipping these in day-two mode would close the bind-mount case above as well.
  3. look-closely · error-handling — crates/lib/src/install/var_mounts.rs:339-377 (351dc87): The ordering is fine (chown, then chmod, then xattrs, then times last). But any lsetxattr failure aborts the install, where cp -a silently skips xattrs it can't set, for example a security.selinux value the host policy rejects, or security.ima/evm. label() relabels the stateroot var anyway, so skipping security.selinux or tolerating ENOTSUP/EPERM would match cp.
  4. note · logic — crates/lib/src/install/var_mounts.rs:433-441 (351dc87): Placeholder metadata is copied from the image's real directory, outside cp's namespace, after cp has created each placeholder from the hiding tmpfs. Only hidden paths that check_source_path accepted are included, so there are no symlinks to follow.
  5. note · logic — crates/lib/src/install/var_mounts.rs:479-505 (351dc87): The stateroot var root is now relabeled as /var whenever --defer-var or --initialize-var-mounts is given, including when the backend seeded it. That case is harmless, since the root is already var_t.
  6. note · security — crates/lib/src/install/var_mounts.rs:290-334 (5b52260): The pre_exec namespace hook makes no allocations, makes the mount tree private before mounting, and fails before mounting anything if a step errors. Errors now name the hidden paths.
  7. look-closely · error-handling — crates/initramfs/src/lib.rs:690-706 (2a47cc0): With an overlay /etc, a deleted fstab is a whiteout (a 0/0 char device) in the state's upper directory. read_optional then fails to open it (ENXIO), and setup-root aborts the boot. Treat non-regular files as absent, and open with O_NOFOLLOW.
  8. note · logic — crates/initramfs/src/lib.rs:528-540 (2a47cc0): This follows symlinks, so an absolute etc/fstab symlink in the state would resolve inside the initramfs. It is left as an open question in the PR.
  9. note · test-gap — tmt/tests/booted/test-install-to-filesystem-var-mount.sh:321-334 (859eb94): The --defer-var-alone case now checks the stateroot var root's label. The day-two command and the fstab detection in setup-root are still covered only by the gist's VM matrix.

Safe to skim

  • docs/src/man/bootc-install-to-filesystem.8.md: Prose matching the code; the options section is generated
  • docs/src/man/bootc-setup-root-conf.5.md: One doc paragraph
  • crates/lib/src/bootc_composefs/boot.rs: Passes the new flag through
  • crates/lib/src/bootc_composefs/update.rs: Passes false for the new flag
  • crates/lib/src/install/baseline.rs: New struct field default
  • tmt/tests/tests.fmf: Duration bump, regenerated
  • crates/lib/src/install.rs:3424-3478: Table test rewrite for check_var_options
  • crates/initramfs/src/lib.rs:123-139: MountConfig::mount_type, extracted from mount_subdir unchanged

@jmarrero-bot jmarrero-bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Review guide for head afc76580e9: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.

Prototype stacked on bootc-dev#2521 (the first two commits are that PR and are not reviewed here). It replaces bootc-dev#2521's copy-from-the-seeded-stateroot-var with one cp -a per empty filesystem from the image's /var, run in a private mount namespace that covers other filesystems' mountpoints with tmpfs; adds --defer-var (ostree: write libostree's .ostree-selabeled stamp so prepare_deployment_var() skips seeding; composefs: skip the initial copy); adds bootc state initialize-var for booted systems, limited to block filesystems mounted from their root (plus /var itself); and makes composefs setup-root skip its /var bind mount when the fstab /etc will show has /var. All findings from both review rounds are fixed or left as open questions in the PR body. What remains is design: the stamp versus a bootc marker or a libostree option, re-run semantics, the behavior change for existing composefs systems with a /var fstab entry, and day-two support for ZFS and btrfs subvolume mounts. Most of the risk is in var_mounts.rs.

Hotspots

  1. note · logic — crates/lib/src/install/var_mounts.rs:101-103 (8e42d4f): Excluding sources with [..] also excludes btrfs subvolume mounts (/dev/sda2[/@x]) and ZFS datasets, so those volumes can't be initialized on day two. That is conservative and acceptable, but worth documenting.
  2. note · security — crates/lib/src/install/var_mounts.rs:296-336 (5b52260): The pre_exec namespace hook makes no allocations, makes the mount tree private before mounting, and fails before mounting anything if a step errors. Errors name the hidden paths.
  3. look-closely · error-handling — crates/lib/src/install/var_mounts.rs:341-389 (338c2d2): Placeholder metadata is copied after cp. It skips security.selinux (the stateroot var is relabeled afterwards anyway) and tolerates ENOTSUP and EPERM like cp -a, while chown, chmod and utimensat stay strict.
  4. note · logic — crates/lib/src/install/var_mounts.rs:491-517 (338c2d2): The stateroot var root is relabeled as /var whenever --defer-var or --initialize-var-mounts is given, since the stamp keeps libostree from doing it.
  5. look-closely · api — crates/initramfs/src/lib.rs:700-716 (823fc4a): This changes behavior for existing composefs systems with a /var fstab entry: they move from the state directory's var to the fstab filesystem. The commit and the PR body say so; whether that is acceptable is a design question.
  6. note · error-handling — crates/initramfs/src/lib.rs:529-552 (823fc4a): A symlinked etc/fstab now reads as absent, so such a system keeps the bind mount (the old behavior) rather than failing to boot. Whiteouts and FIFOs are also treated as absent.

Safe to skim

  • docs/src/man/bootc-install-to-filesystem.8.md: Prose matching the code; the options section is generated
  • docs/src/man/bootc-setup-root-conf.5.md: One doc paragraph
  • crates/lib/src/bootc_composefs/boot.rs: Passes the new flag through
  • crates/lib/src/bootc_composefs/update.rs: Passes false for the new flag
  • crates/lib/src/install/baseline.rs: New struct field default
  • tmt/tests/tests.fmf: Duration bump, regenerated
  • crates/initramfs/src/lib.rs:123-139: MountConfig::mount_type, extracted from mount_subdir unchanged

@jmarrero-bot jmarrero-bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Needs rework at afc7658: mountpoint metadata is fixed; the fstab-view finding remains for persistent /etc overlays.

  • P2 — wrong overlay fstab view: crates/initramfs/src/lib.rs:704-708 reads state/etc/fstab, but build_overlay_fd() uses state/etc/upper as the upperdir (lines 295-302). An upper-layer fstab adding /var is missed, and a whiteout deleting the image fstab still falls back to that image file, suppressing the /var bind mount despite no effective /var entry. Mount /etc first, then read its effective fstab through visible_root; add conflicting upper/lower and whiteout cases.

The bind/none/transient selection and directory metadata restoration look correct by inspection, as do the additional bind-mount filter, xattr error handling and deferred-root labeling. Reviewed the range-diff; diff and commit hygiene checks pass. Read-only follow-up: no tests rerun or devspace started; the PR records testing at 859eb94.

@jmarrero-bot jmarrero-bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Review guide for head dd929f0330: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.

Prototype stacked on bootc-dev#2521 (the first two commits are that PR and are not reviewed here). It replaces bootc-dev#2521's copy-from-the-seeded-stateroot-var with one cp -a per empty filesystem from the image's /var, run in a private mount namespace that covers other filesystems' mountpoints with tmpfs; adds --defer-var (ostree: write libostree's .ostree-selabeled stamp so prepare_deployment_var() skips seeding; composefs: skip the initial copy); adds bootc state initialize-var for booted systems, limited to block filesystems mounted from their root (plus /var itself); and makes composefs setup-root skip its /var bind mount when the fstab /etc will show has /var. All review findings are fixed or left as open questions in the PR body. What remains is design: the stamp versus a bootc marker or a libostree option, re-run semantics, the behavior change for existing composefs systems with a /var fstab entry, and day-two support for ZFS and btrfs subvolume mounts. Most of the risk is in var_mounts.rs.

Hotspots

  1. note · logic — crates/lib/src/install/var_mounts.rs:101-103 (4e0db6d): Excluding sources with [..] also excludes btrfs subvolume mounts (/dev/sda2[/@x]) and ZFS datasets, so those volumes can't be initialized on day two. That is conservative and acceptable, but worth documenting.
  2. note · security — crates/lib/src/install/var_mounts.rs:296-336 (5b52260): The pre_exec namespace hook makes no allocations, makes the mount tree private before mounting, and fails before mounting anything if a step errors. Errors name the hidden paths.
  3. look-closely · error-handling — crates/lib/src/install/var_mounts.rs:341-389 (338c2d2): Placeholder metadata is copied after cp. It skips security.selinux (the stateroot var is relabeled afterwards anyway) and tolerates ENOTSUP and EPERM like cp -a, while chown, chmod and utimensat stay strict.
  4. note · logic — crates/lib/src/install/var_mounts.rs:491-517 (338c2d2): The stateroot var root is relabeled as /var whenever --defer-var or --initialize-var-mounts is given, since the stamp keeps libostree from doing it.
  5. look-closely · api — crates/initramfs/src/lib.rs:702-727 (a35622b): This changes behavior for existing composefs systems with a /var fstab entry; the commit and the body say so. With an overlay /etc it reads state/etc/upper/fstab (a whiteout reads as absent) and otherwise falls back to the image's.
  6. note · error-handling — crates/initramfs/src/lib.rs:531-551 (a35622b): A symlinked etc/fstab now reads as absent, so such a system keeps the bind mount (the old behavior) rather than failing to boot. Whiteouts and FIFOs are also treated as absent.

Safe to skim

  • docs/src/man/bootc-install-to-filesystem.8.md: Prose matching the code; the options section is generated
  • docs/src/man/bootc-setup-root-conf.5.md: One doc paragraph
  • crates/lib/src/bootc_composefs/boot.rs: Passes the new flag through
  • crates/lib/src/bootc_composefs/update.rs: Passes false for the new flag
  • crates/lib/src/install/baseline.rs: New struct field default
  • tmt/tests/tests.fmf: Duration bump, regenerated
  • crates/initramfs/src/lib.rs:123-139: MountConfig::mount_type, extracted from mount_subdir unchanged

@jmarrero-bot

Copy link
Copy Markdown
Author

Second round of review fixes, now at dd929f0 (squashed into the cli, defer and composefs commits):

  • bootc state initialize-var also skips bind mounts of host directories (/dev/X[/path] sources other than /var itself), e.g. kubelet subPath mounts. The man page says bind mounts, btrfs subvolumes and ZFS datasets are left alone.
  • The placeholder metadata copy skips security.selinux (the relabel sets it) and xattrs it can't set, like cp -a.
  • setup-root reads an overlay /etc's fstab from its upper layer (state/etc/upper), else the image's; a whiteout, symlink or other non-regular file counts as no fstab, so a deleted fstab can't fail the boot.

At dd929f0: fmt, clippy, doc, update-generated direct --check, unit tests (304 + 12) and the namespace test pass; the ostree deferred case and the composefs /var LV case passed again at afc7658, which differs only in the overlay branch. Details in the body and gist.

@jmarrero-bot jmarrero-bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

opencode review of dd929f0: changes needed.

P2 — raw overlay upper is not the effective fstab (crates/initramfs/src/lib.rs:710-719). With OverlayFS metacopy enabled (a kernel/module default this mount does not override), chmod/chown of a lower /etc/fstab can leave an empty metadata-only upper file. The merged fstab still contains the lower /var entry, but this code reads the empty raw upper file and binds the state var over the intended volume. Mount/configure /etc first, then read etc/fstab through visible_root; this also delegates opaque-directory and whiteout semantics to OverlayFS. Add effective-view regression cases for conflicting upper/lower files, deletion and metadata-only copy-up. The recorded VM cases explicitly use no overlay /etc, and the added unit tests only exercise parsing and mount-type selection.

The host-directory bind-mount filter and prior placeholder-metadata/deferred-root-label fixes look correct. Reviewed only 5f31d979..dd929f03, the range-diff from afc7658, prior reviews/fix summaries and linked test evidence. Locally reran cargo fmt --check, shell syntax, git diff --check and bot-git check (all passed). Static review; no build or devspace run.

Review guide for head dd929f0330: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.

Independent static recheck of the five prototype commits above bootc-dev#2521. The day-two host-directory bind-mount filter and earlier metadata and labeling fixes look correct. The remaining correctness issue is setup-root reading the raw persistent /etc overlay upper layer rather than the merged fstab: a metadata-only copy-up can make these contents differ. The recorded VM matrix explicitly does not use overlay /etc; the unit tests cover parsing and mount-type selection, not effective file lookup. The stamp, interrupted-copy semantics and behavior change for existing composefs systems remain documented prototype design questions.

Hotspots

  1. risky · logic — crates/initramfs/src/lib.rs:710-719 (a35622b): With OverlayFS metacopy enabled, chmod of a lower fstab can create an empty upper metadata-only file while the merged fstab still reads lower data. Reading the raw upper file then misses /var and binds the state directory over the intended volume. Mount /etc first and inspect its effective fstab through visible_root.
  2. look-closely · test-gap — crates/initramfs/src/lib.rs:756-792 (a35622b): The tests verify mount-type selection and parsing but never exercise conflicting upper/lower fstabs, whiteouts or metadata-only copy-up. The linked VM matrix uses no overlay /etc, so it cannot detect the remaining lookup bug.
  3. note · logic — crates/lib/src/install/var_mounts.rs:96-103 (4e0db6d): The filter rejects host-directory bind sources while retaining the intended state-directory bind at /var. The fixture covers kubelet binds, and btrfs subvolume and ZFS exclusions are documented prototype limitations.
  4. look-closely · error-handling — crates/lib/src/install/var_mounts.rs:341-389 (338c2d2): Placeholder metadata is restored after cp leaves its private namespace. SELinux labels are left to the final logical-path relabel; forbidden and unsupported xattr writes are tolerated. The ordering matters to the earlier ACL and metadata-loss fix.

Safe to skim

  • crates/lib/src/bootc_composefs/boot.rs: Passes the defer flag through.
  • crates/lib/src/bootc_composefs/update.rs: Passes false for the initial-install flag.
  • crates/lib/src/install/baseline.rs: New field default.
  • tmt/tests/tests.fmf: Generated duration change.

@jmarrero-bot

Copy link
Copy Markdown
Author

Reviewers disagree at dd929f0: the Claude reviewer said ship; opencode (review) says changes needed: with an overlay /etc, setup-root reads the raw upper-layer fstab, which can be wrong under metadata-only copy-up (an empty upper file whose content is in the lower layer) or an opaque /etc, so /var may be bound or not on the wrong fstab; the tests don't cover an overlay /etc. Its suggested fix: mount /etc first and read the effective fstab, plus overlay-view tests. The stricter verdict stands until resolved.

@jmarrero-bot jmarrero-bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Review guide for head 0438277436: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.

Next revision of bootc-dev#2521, stacked on #5. --initialize-var-mounts discovers the filesystems mounted at or below the target's /var and fills each empty one from the image's /var (the ostree deployment checkout, or the mounted composefs image), one cp -a per filesystem in a private mount namespace where nested mountpoints are covered by tmpfs on both sides. With --defer-var, the stateroot var gets only what no mounted filesystem holds. bootc state initialize-var runs the same code on a booted system for block-device filesystems. A setup-root commit skips the composefs /var bind mount when the effective fstab (read through the mounted /etc) has a /var entry. The copy logic is careful and well covered by unit tests, tmt and VM runs. The riskiest piece is the setup-root change: it runs on every composefs boot, not only for users of the new flags, and any read error there (including a non-UTF-8 fstab) fails the boot. Also look at the pre_exec mount-namespace hook, the label pass, and the commit message in jmarrero's commit, which still says --version 'now prints' the Features list.

Hotspots

  1. risky · error-handling — crates/initramfs/src/lib.rs:526-548 (6ab7f0a): This runs in the initramfs on every composefs boot. read_to_string fails on a non-UTF-8 fstab, and other open or read errors propagate, so setup-root fails. Reproduced: one Latin-1 byte in a comment sends a UEFI composefs VM to emergency mode ('stream did not contain valid UTF-8'). Read the bytes and decode lossily, and on errors warn and keep the bind mount.
  2. look-closely · logic — crates/initramfs/src/lib.rs:699-716 (6ab7f0a): This changes boot for existing composefs systems that already have a /var fstab entry: they move from the state directory to the fstab filesystem (the commit message says so). The read goes through visible_root after /etc is mounted, which is right. Only the pre-6.15 path is exercised by the default build; the floating-tree path is untested.
  3. look-closely · security — crates/lib/src/install/var_mounts.rs:252-318 (f671436): pre_exec runs unshare(NEWNS) and makes / rprivate before the tmpfs mounts, so nothing leaks to the host namespace. It only allocates beforehand, which is correct after fork in a multithreaded process. The default view skips the namespace so unit tests can run unprivileged.
  4. look-closely · logic — crates/lib/src/install/var_mounts.rs:114-150 (34fe90e): The emptiness check decides between filling and leaving data alone. It allows only an empty root lost+found and directories that lead to nested mountpoints; anything else, including a non-UTF-8 name or a symlink, counts as data. It is conservative in the right direction.
  5. look-closely · logic — crates/lib/src/install/var_mounts.rs:411-430 (e9742e9): Deferred stateroot var fill. The source mountpoints are hidden by tmpfs, so cp gives the stateroot var's mountpoint directories tmpfs metadata, which copy_dir_metadata then restores from the unhidden source (SELinux label excluded; label() relabels the whole stateroot var as /var later).
  6. note · error-handling — crates/lib/src/install/var_mounts.rs:499-527 (f671436): Failure path: the error names the mountpoint and source and says the filesystem may be partly filled. cp's stderr is attached by run_capture_stderr. Journal events are a no-op inside the install container (no systemd), as the man page says.
  7. note · logic — crates/lib/src/install/var_mounts.rs:91-93 (159ee0f): The day-two filter: /dev/ sources without a [subpath], plus /var itself. This excludes btrfs subvolumes, ZFS datasets, NFS, tmpfs and bind mounts, as bootc-dev#2599 promises.
  8. look-closely · logic — crates/lib/src/install.rs:2432-2473 (f671436): Ordering: populate runs after the deployment and before the physical-root relabel. Copied files keep the source labels, so the walk skips them, and label() then labels the mounts and the deferred stateroot var by their /var paths. The ImageVar (and the composefs mount) is dropped at the end of the if-let, before labeling.
  9. note · test-gap — tmt/tests/booted/test-install-to-filesystem-var-mount.sh:303-330 (0438277): --defer-var --initialize-var-mounts is tested only without a /var volume. The STIG-shaped case (a /var volume with nested volumes, and --defer-var) was booted neither here nor in the gist's VM cases.

Safe to skim

  • crates/lib/src/install/var_mounts.rs:627-1128: unit tests
  • crates/initramfs/src/lib.rs:734-792: unit tests
  • docs/src/man/bootc-setup-root-conf.5.md: doc paragraph

@jmarrero-bot jmarrero-bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Reviewer verdict at 0438277: fix first. The copy logic, the mount-namespace handling, labels, hardlinks, nested volumes and the failure and journal path are sound. Two things need fixing before promote.

  1. crates/initramfs/src/lib.rs:526-548 (risky). setup-root now reads /etc/fstab on every composefs boot. read_to_string fails on non-UTF-8 content, and any other open or read error propagates, so the boot fails. Reproduced on a UEFI composefs VM: one Latin-1 byte in an fstab comment goes to emergency mode (Reading etc/fstab: stream did not contain valid UTF-8). Fix: read the bytes and decode them lossily (/var is ASCII). On any other error, warn and keep the bind mount (the old behavior). Add a case with a 0xff byte.
  2. 34fe90e's message (jmarrero's) still says the Features: list "bootc --version now prints", which #5 adds now. Reword it to "It is listed as initialize-var-mounts in the Features: list of bootc --version", and drop or move the container inspect rationale to #5. Keep his authorship and say in the reply that the message changed.

Minor:

  • The tmt hunk in 34fe90e also drops #5's mkfs.vfat -i UUID preservation. That's harmless after b99826c, but the fork note doesn't mention it. The other conflict resolutions match range-diff against 5f31d97/c3949f7f.
  • Test gap: --defer-var --initialize-var-mounts with a /var volume plus nested ones (the STIG layout) wasn't booted in tmt or in the gist. On composefs it depends on 6ab7f0a. Add it to the e2e matrix.
  • 6ab7f0a's fstab read through the mounted /etc is right for pre-6.15. The floating-tree path (non-default) is untested.
  • bot-git check flags b99826c and 34fe90e for Generated-by under jmarrero's authorship. He already carried that trailer in bootc-dev#2521, so this is fine.
  • bootc-dev#2599 says install-time failures reach the journal. Inside the install container journal_send is a no-op. The man page's "when it is available" is accurate.

Pre-existing on main, not caused by this PR: the overlay /etc comes up unlabeled_t. build_overlay_fd creates etc/upper and work in the initramfs before the policy loads (crates/initramfs/src/lib.rs:284-285), and the overlay root takes the upper dir's missing label. I confirmed this by reading the code, not by booting main.

Re-run on a 16-core devspace: fmt, clippy, doc, --no-default-features, update-generated (no diff), 350 + 12 unit tests, and mounted_var_filesystems under unshare -Urnm, all passing. VM: a composefs layout-D run with --defer-var --initialize-var-mounts on UEFI (OVMF) passed every check through boot, bootc status, switch to v2 and rollback, enforcing. That covers the composefs switch and rollback the body lists as untested. The body's other claims match the round-4 logs.

e2e (tracker#55): nothing here blocks it, since osbuild writes an ASCII fstab, but fix item 1 before promote. Run composefs e2e on UEFI, because BIOS hits main's SystemNotUEFI bug.

Generated-by: https://github.com/jmarrero-bot#llms

@jmarrero-bot

Copy link
Copy Markdown
Author

Review fixes pushed: 0438277 is now d72d38a, on top of #5 at 3452994.

  • A later copy failure no longer skips finishing earlier filesystems. populate() records what it completed as it goes, and finish() labels and syncs that before returning the error, both at install and in bootc state initialize-var. There's a two-volume unit test where the second copy fails (squashed into the per-filesystem commit).
  • jmarrero's 3c8272f: its message now says the option is listed in the Features: list, instead of that --version "now prints" it, and the container inspect rationale moved to install: Add --defer-var to leave the stateroot /var unseeded #5. The fork note says so, and that the commit drops install: Add --defer-var to leave the stateroot /var unseeded #5's mkfs.vfat -i.
  • The composefs setup-root commit, with the non-UTF-8 fstab fix, moved to install: Add --defer-var to leave the stateroot /var unseeded #5.
  • New VM cases: the STIG shape (a /var volume plus nested ones, --defer-var --initialize-var-mounts) on both backends. All composefs cases ran on UEFI, so composefs switch and rollback are now covered.

Results are in the body, logs in the gist (round 5).

@jmarrero-bot

Copy link
Copy Markdown
Author

Restacked unchanged onto #5 at 9506ed3: d72d38a is now 4c65307. Static checks, unit tests (351 + 15), the namespace test and cargo check --tests at each commit pass. One effect to note: #5 now gates setup-root's fstab check on the --defer-var marker. So on composefs, --initialize-var-mounts alone keeps the bind mount over a /var volume until this PR widens the gate (the body's open question). I haven't made PR 2 changes yet; its review findings come separately.

The test passed the EFI system partition's UUID as the boot mount spec,
while /boot is a separate ext4 partition in its layout. Use the UUID of
the boot partition, which is what the installed system needs to mount.
This went unnoticed because the test does not boot the result.

Generated-by: AI
A common setup places /var, or parts of it such as /var/log, on
separate filesystems (for example LVM logical volumes) so that runaway
writes cannot fill the root filesystem. With `install to-filesystem`
these filesystems end up empty: the deployment backend seeds the
image's /var into its own state directory (`ostree/deploy/<stateroot>/var`,
or the composefs shared var), and at boot the separately mounted
filesystems hide that content.

With the new --initialize-var-mounts option, a fresh to-filesystem
installation discovers filesystems mounted at the target's /var or
below. Each empty top-level mount tree, including its nested mounts, is
initialized from the seeded /var with `cp --archive`, then SELinux
labeled and synced. A tree that already contains data is preserved as a
whole, without merging. The option is rejected for alongside and
existing-deployment installs, before anything on the target is changed.
Callers remain responsible for mounting these filesystems at boot.

Without the option nothing changes, so callers that mount an empty /var
filesystem and fill it themselves after installation are not affected.
Doing this by default was considered, but would have changed behavior
for them. Image builders can tell whether to pass the option from its
`initialize-var-mounts` entry in the `Features:` list of
`bootc --version`.

The seeded copy in the state directory is kept; on OSTree, emptying it
would make the next deployment reseed it. Hardlinks cannot span
filesystems, so trees with nested mounts are copied without preserving
them. The roots of these filesystems are labeled unconditionally:
without selinuxfs (e.g. in an osbuild buildroot), an inode with no label
still reports the kernel's unlabeled context, which the regular relabel
walk takes as already labeled.

The separate /var TMT test now passes the option, checks contents,
ownership, symlinks and SELinux labels on the actual volumes, including
a nested one, and checks that without the option they stay empty.

Related: bootc-dev#1615
Related: bootc-dev#997
Generated-by: AI
@jmarrero-bot

Copy link
Copy Markdown
Author

Re-review fixes pushed: 4c65307 is now 55fd0e5, restacked on #5 at ff41dcc. jmarrero's two commits are unchanged.

  • New commit "composefs: Leave /var to fstab after --initialize-var-mounts filled it": a second marker, .bootc-var-mounts, is written when a filesystem is mounted on the target's /var, and setup-root accepts either marker. The name is defined once, in the initramfs crate. The man pages and the decision-table tests are updated.
  • Install loads the policy before copying and calls finish() right after populate(). Each filesystem is finished on its own, and every error is collected, with the copy error first.
  • Day two keeps every mount as a boundary and only filters which filesystems are filled. The error now says what is mounted there when nothing can be filled.
  • A failed copy into the deferred stateroot var names it as /var, the deployment's state directory, and says to install again onto a fresh root.
  • Docs: "bind mounts of a subdirectory", and that a whole-filesystem bind mount counts as the filesystem.
  • Re-runs: documented, not skipped. Skipping would mean writing state onto the caller's volume.
  • Tests: a two-volume failure that also fails as root, finishing where the first filesystem fails, a day-two tmpfs under an empty volume, and the namespace test now runs through unshare -Urnm and fails without hide_target. All 8 mutations fail a test.

Tested at the pushed commit (package g55fd0e5973): static checks, unit tests, tmt plan-32 on ostree and composefs BLS, and VM cases co, oai, cai, odi, cdi and cf. co (--initialize-var-mounts alone, overlay /etc) now has /var on the volume at first boot. Details are in the body. Logs: https://gist.github.com/jmarrero-bot/cb0ed128e38cab8cc220d7b21e63de0b (round 7).

@jmarrero-bot

Copy link
Copy Markdown
Author

Second-review items and the e2e <<none>> bug: 55fd0e5 is now 5875cfc. jmarrero's two commits are unchanged.

  • New prep commit b21a427, "lsm: Leave paths the policy labels <> as they are". relabel() now uses the optional label: a path such as /var/tmp/.* keeps its label, as with setfiles and restorecon. A file with no label at all gets its directory's. Without that, it would stay unlabeled_t on composefs, where the copy arrives unlabeled. The fix covers the volumes (label()), the deferred state directory (relabel_recurse) and bootc state initialize-var, on both backends.
  • label() skips a lost+found that is a mountpoint.
  • Finishing goes through a Finisher trait, and the tests drive the real label-then-sync code with a recorder. There are also tests for the state directory's failure text and the marker in plan-32.
  • The namespace test fails when CI is set instead of skipping, and just unit-tests now passes CI into its container: before this, the test could never see it.
  • Nits: the rewraps, and the older-initramfs note for .bootc-var-mounts.

Tested at the pushed commit: static checks, unit tests, CI=true just unit-tests, and tmt plan-32 on ostree and composefs BLS. tmt also ran against reverted fixes, and failed each time: without the marker call, and without the <<none>> fix on both backends, both in the /var-volume case and in the case with no /var volume. Logs: https://gist.github.com/jmarrero-bot/cb0ed128e38cab8cc220d7b21e63de0b (round 8).

@jmarrero-bot

Copy link
Copy Markdown
Author

Third-review fixes pushed: 5875cfc is now 2139e25. jmarrero's two commits are unchanged, and each fix is squashed into the commit it belongs to (lsm prep, per-filesystem, deferred /var, tmt).

  1. relabel_none unlabels by removing the xattr where this process doesn't see SELinux. It skips when labels can't be both set and removed. In bootc-units with --cap-add SYS_ADMIN --security-opt label=type:spc_t it now skips, where 5875cfc failed. It also skips in the default container, so the body says that upstream CI doesn't run it there and only tmt covers <<none>>.
  2. --env CI is dropped: jmarrero-forge/bootc runs no CI for these branches (no runs at all), so it can't be checked on ubuntu-26.04. The namespace test skips with its message again.
  3. Labeling now logs <<none>> and no-policy skips at debug level. The unlabeled type is looked up once per relabel or walk, and the xattr read is shared. Also new: new_sepolicy_at() loads one policy at a time. A mutation run showed another test failing with the test policy loading in parallel, since loading sets libselinux's process-wide policy root.
  4. A new finish_labels_and_syncs test covers the real Finish: it fails if labeling is a no-op, if syncing is a no-op, or if the stateroot var is labeled by its own path. plan-32 asserts that the /var/tmp directory gets matchpathcon -n /var/tmp on both backends; it fails on composefs without the inheritance. The gist's round 8 mutation log is corrected.
  5. The body and man page say that a <<none>> file keeps system_conf_t from the checkout on ostree and gets tmp_t on composefs.

One mutation still survives: not checking that the parent has a label before copying it. On an SELinux host an unlabeled parent is unlabeled_t, so copying it changes nothing. Where labels can be removed, the test catches it.

Logs: https://gist.github.com/jmarrero-bot/cb0ed128e38cab8cc220d7b21e63de0b (round 9).

relabel() failed on any path the policy has no label for. In the
targeted policy those are the <<none>> entries, such as /var/tmp/.*,
which mean "don't relabel": setfiles and restorecon skip them. Relabeling
a /var that holds image content there, as --initialize-var-mounts does,
then failed the installation with "No label found in policy".

Skip them too, keeping whatever label they have. A file without any
label, for example one copied onto a fresh filesystem before its root
was labeled, gets its directory's label instead, as it would if it had
been created there; left unlabeled_t, confined services such as
systemd-tmpfiles couldn't touch it. This applies to every caller of
relabel(), all of which would otherwise have failed outright. With no
policy loaded at all, every lookup comes back empty the same way: that
now skips labeling too, saying so at debug level, like the <<none>>
paths.

The test writes a policy of its own from a file_contexts, so it doesn't
depend on the host's; it skips where this process can't set and remove
labels. Loading a policy sets libselinux's process-wide policy root, so
new_sepolicy_at() now loads one at a time: with the test's policy and
another test's loading in parallel threads, either could get the other's
file contexts. The few direct SePolicy constructors left (the SELinux
re-exec, the overlay relabel service, the internal relabel command) run
alone in single-threaded command paths, and taking them through
new_sepolicy_at() would change how they handle a missing policy, so
they stay as they are.

Generated-by: AI
--initialize-var-mounts copied the deployment backend's seeded /var
into each empty mount tree, and skipped a whole tree when any of its
filesystems held data. That made the stateroot copy the source of
truth for the volumes, and a single volume restored from backup under
an empty /var volume meant the system booted with none of the image's
/var, and with /var unlabeled.

Copy from the image's /var instead: the deployment's checkout on
ostree, the mounted composefs image on composefs. Each empty filesystem
gets its own `cp --archive`, run in a private mount namespace where the
mountpoints of the filesystems nested in it are covered by a tmpfs, on
both the source and the target side. So each file lands on the
filesystem that holds it at boot, a filesystem with data is left alone
without stopping its empty parent or children from being filled, and
hardlinks are kept within a filesystem. This is also what lets a later
change fill the stateroot var itself without duplicating what the
volumes hold.

An ostree commit doesn't record hardlinks: identical files share one
object, so a deployment checkout links files that are separate in the
image. Copying it with links preserved would make writes to one show up
in the other, so on ostree every file is copied on its own, like
libostree's own seeding does. composefs images record them, so there
they are kept.

The namespace is only entered when there is something to cover, so
single-filesystem copies still work unprivileged in unit tests.

A failed copy leaves its filesystem partly filled, which later runs
take as holding data, so it is never repaired behind the user's back.
The error says that, naming the filesystem's mountpoint and source,
and later runs say which filesystems they skip because they hold data.
The filesystems completed before a failure are still labeled and synced
before the error is returned, since a retry leaves them alone too; each
is tried on its own, so one failing to label or sync doesn't stop the
others, and their errors are reported after the copy's. At install this
happens right after the copies, before the physical root's relabel can
fail. A volume that is still empty because the image has nothing for it
counts as not initialized, so a later run copies and labels it again;
the man page says so. Marking such volumes would put state on them.

Labeling a filesystem's root and lost+found unconditionally skips a
lost+found that something is mounted on, as the walk below them skips
mountpoints: that belongs to another filesystem.

The namespace test, which needs a private user and mount namespace, now
runs with the others through unshare -Urnm where that is available, and
says so when it skips.
Fills, skips and failures are also logged to the journal under one
message ID, like other bootc events.

Generated-by: AI
With --initialize-var-mounts alone the deployment backend still seeds
its state directory with the whole image /var, so everything that ends
up on a mounted filesystem is also stored on the root filesystem,
hidden under the mount at boot.

Combined with --defer-var, which leaves the state directory with only
its .bootc-defer-var marker, bootc fills it itself: it gets only what
isn't on a mounted filesystem, and nothing at all when a filesystem is
mounted on /var itself. The copy is the same per-filesystem one as for
the mounts, with the mountpoints hidden, so it only leaves empty
mountpoint directories (with the image's metadata) where the volumes
go. It is labeled like libostree labels the /var it seeds. Whether to
fill the state directory follows from --defer-var, rather than from
the directory being empty, since on ostree it also holds libostree's
own stamp by then.

Messages and journal entries name it as /var, the deployment's state
directory, and a failed copy there says to install again onto a fresh
root, since it is filled only at installation.

The two options are now checked together.

Generated-by: AI
--initialize-var-mounts only helps at installation. A volume added to
/var later, say a new LV for /var/opt/app, starts empty and hides the
image's content for that path just like an uninitialized one did at
install time.

`bootc state initialize-var [PATH]` runs the same per-filesystem copy on
a booted system: each empty block-device filesystem mounted at or below
PATH (/var by default) gets its part of the booted deployment's /var,
taken from the deployment's checkout on ostree and from its composefs
image on composefs, and is then labeled and synced. Filesystems with
data are left alone, so running it on /var as a whole only touches empty
volumes. Other mounts there, like container tmpfs mounts, network
filesystems or bind mounts of subdirectories, aren't ours to fill or
label, but they stay boundaries: an empty volume with a tmpfs mounted
below it is still filled, around the tmpfs. When only such mounts are
there, the error names them.

Filling a /var whose installation was deferred is not something this
covers on its own: by the time anything can run it on the booted
system, systemd-tmpfiles has already written into /var, so it isn't
empty anymore. That needs either the image builder doing it at install
time, or an early first-boot unit.

It is advertised as "state-initialize-var" in `bootc --version`.

Generated-by: AI
setup-root leaves /var to the fstab only on a system whose stateroot var
has the --defer-var marker. On composefs, --initialize-var-mounts
without --defer-var onto a filesystem mounted on /var then fills that
filesystem, and setup-root bind-mounts the seeded state directory over
it at boot, so the volume bootc just filled goes unused.

So when a filesystem is mounted on the target's /var itself, whether
bootc fills it or leaves it alone because it holds data, write a second
marker, .bootc-var-mounts, into the stateroot var, labeled like /var's
files, and have setup-root accept either marker. It is written after
the first deployment: on ostree, anything in the stateroot var before
it would stop libostree from seeding it, and the marker only matters
to composefs anyway. Reusing .bootc-defer-var isn't possible, since it
would also keep composefs from seeding the state directory that
--initialize-var-mounts alone still seeds. The name is defined once,
next to the other marker, in the initramfs crate.

Existing systems, and installs with neither option, still have no
marker, and keep the bind mount.

Generated-by: AI
The point of combining the two is that a file under a mounted
filesystem is stored only there. Check that on a target without a /var
filesystem, where the state directory is /var at boot: it gets the
image's /var minus the nested volume's content, which lands only on the
volume, and both are labeled. Hardlinks within the volume are kept on
composefs and split on ostree, whose commits don't record them.

The --defer-var alone case now also runs with the nested volume, and
checks that the state directory is labeled as /var.

The installs also check that .bootc-var-mounts is written exactly when a
filesystem is mounted on /var itself.

The image also has content under /var/tmp, which the policy has no
label for: it must not stop the labeling of the filesystems or of the
state directory, and must not end up unlabeled.

Generated-by: AI
@jmarrero-bot

Copy link
Copy Markdown
Author

opencode's fix-first item is fixed: 2139e25 is now ce2a1e1. Only the lsm commit (784be9f) and the per-filesystem commit (598adef) changed, and only in tests and the lsm commit message.

  • The skip probe in cannot_label() now checks only the raw xattr. After unlabeling, the probe file must read back NODATA, or exactly unlabeled_t where this process sees SELinux. relabel_none and finish_labels_and_syncs then assert is_unlabeled() outside the skip path. Making UnlabeledType::query() always return None now fails both tests; at 2139e25 it made them skip.
  • The other direct SePolicy constructors (the re-exec, the overlay relabel service and internals relabel) stay as they are. They run alone in single-threaded command paths, and going through new_sepolicy_at() would change how they handle a missing policy. The commit message says so.

Static checks and unit tests (358 + 16) pass, and cargo check --tests passes at each commit. 19 of 20 mutations fail a test; the parent-label check still survives on an SELinux host, as before. tmt wasn't rerun, since no production code changed. Logs: https://gist.github.com/jmarrero-bot/cb0ed128e38cab8cc220d7b21e63de0b (round 10).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Draft

Development

Successfully merging this pull request may close these issues.

2 participants