Skip to content

bootc-dev/bootc#2571: operator request #703

Open

Description

@cgwalters-bot

The operator commented on bootc-dev/bootc#2571, a pull request cgwalters-bot did not open. Runs for it take this issue as their item and answer here; cgwalters-bot's comment there links to it.

cgwalters wrote (bootc-dev/bootc#2571 (comment)):

In bootc upstream we shouldn't have anything that is hardcoded about RPM or a specific distro/OS - while this is just a comment, I would make that clear.

That said! For this specific case again there's a HUGE detail in that in OCI images it's not required to include the root, and many don't!

More on that in coreos/fedora-coreos-tracker#2238 (comment) - please include direct links for that here.

So we should handle this - if the input OCI image has permissions for the root we should use them. If it doesn't, then let's match what composefs-rs is doing and take the permissions from the /usr directory.

(or of course...in theory we could look at the tmpfiles.d config now...but I am not sure I want to have parsing those be in the truly load bearing path)

I am not a fan of propagating hardcoding 0o555 in more places.

(Note contrast w/your rpm-ostree PR - there I think we can assume that the RPM content set includes metadata for the root)

I think here though...it is complicated and subtle in the true general case, but I'd just be happier if we hardcoded propagaing the perms of /usr (after unpacking all layers) instead to match composefs-rs.

cgwalters wrote (bootc-dev/bootc#2571 (comment)):

The thing that's way easier on the composefs side is it always operates on an in-memory metadata tree so we can easily express "Option" to know whether or not we found metadata for the root (or any arbitrary directory) whereas for an actual kernel filesystem there's no concept of "unset permissions/mtime".

cgwalters wrote (bootc-dev/bootc#2571 (comment)):

Let's do exactly the same as composefs-rs, all metadata: ownership, mtime, xattrs etc.

cgwalters wrote (bootc-dev/bootc#2571 (review)):

LGTM that said I think a readonly tmt test that asserts / matches the rpm database would make sense in addition to a unit test.

Can be a followup

cgwalters wrote (bootc-dev/bootc#2571 (comment)):

Holy cow. This is...such a giant mess.

Look at https://src.fedoraproject.org/rpms/filesystem/blob/rawhide/f/filesystem.spec#_403 - why are the perms for /usr 0o755 but the perms for /usr/bin are 0o555? It seems just nonsensical.

OK @cgwalters-bot p0 task sol+luna worker - dig through the OCI container images of fedora, opensuse, debian, ubuntu, arch: unpack their OCI archives via skopeo copy to oci: dir to access teh raw tarballs see if these container images have a . directory at all, report its perms if present or not. But then, try to find the source code (filesystem rpm equivalent) for each OS and report what should be the perms of / from the perspective of the package system. Finally also "ground truth" crosscheck find a stock "cloud image" e.g. a thing with cloud-init, use virt-install's cloud init support to boot it and ssh in, and report the perms of that. Use bootc-dev/cgwalters-devspace-sandbox for CI runners.

More of theirs, not quoted here: bootc-dev/bootc#2571 (comment), bootc-dev/bootc#2571 (comment), bootc-dev/bootc#2571 (comment), bootc-dev/bootc#2571 (comment)

Generated-by: https://github.com/cgwalters/#llms

Activity

  1. cgwalters-bot commented on Oct 17, 2026

    @cgwalters-bot
    CollaboratorAuthor

    Conclusion: valid and reproducible; the fix is approved and one rebase away from mergeable.

    What the bug is. write_merge_commit_impl() in crates/ostree-ext/src/container/store.rs inherits the merge commit's root directory metadata from the temp checkout directory, so / lands at 0o755 instead of 0o555. systemd v261's tmpfiles.d z / 555 line then fails with fchmod() of / failed: Read-only file system on read-only roots (FCOS tracker #2238). The PR author reproduced it on FCOS/cosa builds (stat shows 755 vs 555).

    What the fix is. PR #2571 (head 7b970f0a) adds normalize_root_dirmeta(), which copies /usr's metadata checksum to the root mtree after all layers are unpacked — mirroring composefs-rs's copy_root_metadata_from_usr (all metadata: mode, uid/gid, mtime, xattrs). This is the operator-approved approach: no hardcoded 0o555, and it works for OCI images that omit the root entry entirely. Unit tests (assert_root_dirmeta_matches_usr) cover two import paths.

    Why /usr-propagation is the right call (research). The measurement run found: Fedora/Ubuntu/Arch container layers have no . root entry at all; openSUSE and Debian do (0755). Fedora's filesystem spec says / 0555 but /usr 0755 (the 2009 capabilities hardening, commit cd01d2d); the booted Fedora Cloud image actually shows / 0755. openSUSE's filesystem doesn't own /; dpkg stores no directory metadata. So no single hardcoded mode is right across distros — propagating /usr's actual metadata is the only distro-neutral rule, and it matches composefs-rs exactly.

    What fixing it takes:

    1. Rebase #2571 onto main. All current CI failures on head 7b970f0a are the known infra breakage #2572 (grub2-efi-x64-cc missing from quay.io/fedora/eln), fixed by the now-merged #2575 — I confirmed the failing job log shows No package "grub2-efi-x64-cc" available.
    2. Merge as approved (cgwalters approved twice, most recently 2026-10-16). The PR is "correct in combination with" Fedora filesystem PR 44 (changes /usr to 0555, still open), which makes / come out 0555 on Fedora once it lands.
    3. Optional fast-track/followup per the operator: chmod 0555 /usr in bootc's Dockerfile, plus a readonly tmt test asserting /'s mode (comment). Rolv has local tests comparing against /usr and against the filesystem RPM; placement (here vs followup) is the one open question.

    What is unclear: only timing and test placement — whether the tmt test and Dockerfile chmod land in this PR or a followup, and when Fedora's filesystem PR 44 merges. Until it does, /usr-propagation yields 0755 on Fedora, which is what the stock booted Fedora Cloud image shows anyway, so nothing regresses in the meantime.

    Generated by dispatch · ◷

  2. locked as resolved and limited conversation to collaborators on Oct 18, 2026
  3. cgwalters-bot commented on Oct 18, 2026

    @cgwalters-bot
    CollaboratorAuthor

    ✅ dispatch ended well. Result: #703 (comment)

    (Run status from 2026-10-17T11:46:20Z, re-posted by the bot without the agent's own words. The original job-token comment was removed so this issue counts as collaborator-only input, tracker#738.)

    Generated-by: https://github.com/cgwalters/#llms

  4. cgwalters-bot commented on Oct 19, 2026

    @cgwalters-bot
    CollaboratorAuthor

    More from bootc-dev/bootc#2571:

    cgwalters wrote (bootc-dev/bootc#2571 (comment)):

    It'd be helpful if you posted your exact patch. But sorry no I don't know offhand @cgwalters-bot take a look at p0 use devspace build from this PR try reproducing the problem analyze relevant code paths

    Generated-by: https://github.com/cgwalters/#llms

  5. cgwalters-bot commented on Oct 19, 2026

    @cgwalters-bot
    CollaboratorAuthor

    ✅ dispatch ended well. Result: #703 (comment)

  6. cgwalters-bot commented on Oct 20, 2026

    @cgwalters-bot
    CollaboratorAuthor

    Re-dispatched on the devspace runners (KVM), Claude pool: answers land on bootc-dev/cgwalters-devspace-sandbox#22.

    Generated-by: https://github.com/cgwalters/#llms

  7. cgwalters-bot commented on Oct 20, 2026

    @cgwalters-bot
    CollaboratorAuthor

    Answer is on bootc-dev/cgwalters-devspace-sandbox#22 (comment) (partial: the run hit its timeout before compiling its patch). It reproduced the problem on bootc#2571 in a KVM VM. There are two bugs in the merge commit: the union checkout drops a later layer's directory mode, so chmod 0555 /usr in the Dockerfile is lost, and the PR's normalize_root_dirmeta does nothing, because mt.walk(&["usr"], 0) returns the root itself. /usr/bin at 0755 comes from the base image, not bootc. A proposed (untested) patch is in the answer.

    Generated-by: https://github.com/cgwalters/#llms

  8. cgwalters-bot commented on Oct 20, 2026

    @cgwalters-bot
    CollaboratorAuthor

    Follow-up: bootc-dev/cgwalters-devspace-sandbox#22 (comment) built and tested the proposed patch for bootc#2571. As posted, it broke 15 of the 30 ostree-ext integration tests (fchmod: Bad file descriptor). After a one-line scope fix, all of them pass, and in the VM repro the merge commit now has / and /usr at 0555. Unpatched bootc gives 0755 for both.

    Generated-by: https://github.com/cgwalters/#llms

  9. cgwalters-bot commented on Oct 20, 2026

    @cgwalters-bot
    CollaboratorAuthor

    Board scheduler: running this operator request again, once, on the claude pool, as its triage run https://github.com/cgwalters-forge/tracker/actions/runs/39328927648 ended failure on the mistral pool. Retry: https://github.com/cgwalters-forge/tracker/actions/runs/39503382361.

  10. cgwalters-bot commented on Oct 20, 2026

    @cgwalters-bot
    CollaboratorAuthor

    Conclusion: valid and reproduced, and this request is already answered. The failed mistral run relayed cgwalters's request to reproduce why chmod 0555 /usr in the Dockerfile doesn't reach the deployed VM. Two later devspace runs answered it in a KVM VM: repro and root cause, then a built and tested patch. This retry adds no new repro. It re-checks the key claims and lists what is still open.

    Root cause (re-checked against the code):

    1. The PR's fix is a no-op. normalize_root_dirmeta calls mt.walk(&["usr"], 0) (store.rs#L633-L644 @ 7b970f0a). ostree_mutable_tree_walk returns self when start == len - 1 (ostree-mutable-tree.c#L552-L556), so the root's dirmeta gets copied onto the root. The PR's test can't catch this because / and /usr have the same dirmeta in the fixtures. Copying the whole dirmeta checksum would also give / the SELinux label usr_t.
    2. Already broken before the PR: the UNION_FILES layer checkout keeps the first layer's directory metadata, so a later chmod 0555 /usr layer is dropped. OCI and composefs-rs keep the last layer's.
    3. /usr/bin at 0755 is already in the base image, not caused by the import.

    What fixing it takes: the tested diff in the second link. It reapplies directory mode and owner from each layer in order, then copies /usr's mode and owner to / before write_dfd_to_mtree. With it, all ostree-ext it tests pass and the VM merge commit has / and /usr at 0555. The PR's test also needs to compare mode/uid/gid rather than the dirmeta checksum, plus a fixture with a derived layer that chmods /usr.

    Unclear / risks:

    • Layer commits are imported with --tar-autocreate-parents (tar/write.rs#L440). With "last layer wins", a later layer that lacks an explicit usr/ entry would put a made-up 0755 back over an earlier 0555. I inferred this from the code; it hasn't been tested.
    • The patch carries mode, uid and gid but not xattrs or mtime, which is short of "exactly the same as composefs-rs, all metadata" (review). It also walks every directory of every layer, and nobody has measured that cost on large images.
    • It only went through a bare pull of the merge commit. Nobody ran bootc install, booted the result, or ran just test-tmt readonly.
    • The patch hasn't been posted to bootc#2571. Rolv-Apneseth (the PR author) or the operator still has to pick it up.

    Generated by dispatch · ◷

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions