Skip to content

tree: Replace ExternalNoVerity with ExternalPath, keep ostree's redirect - #409

Open
cgwalters-bot wants to merge 1 commit into
composefs:mainfrom
cgwalters-forge:bot/capi-externalpath
Open

cgwalters-bot wants to merge 1 commit into
composefs:mainfrom
cgwalters-forge:bot/capi-externalpath

Conversation

@cgwalters-bot

Copy link
Copy Markdown
Contributor

ostree gives each file a libcomposefs payload of xx/<checksum>.file and sets the fsverity digest separately. composefs-capi turned the payload into an ObjectID (dropping .file), or ignored it when a digest was set, so the overlay redirect pointed at a file that doesn't exist and an ostree deployment on the Rust libcomposefs couldn't boot ("overlayfs: lazy lowerdata lookup failed", "No /sbin/init").

This replaces RegularFile::ExternalNoVerity with ExternalPath { redirect: Option<_>, verity: Option<_>, size }, which is how libcomposefs models a file: an optional redirect as given, plus an independent optional digest for the metacopy xattr. RegularFile::external() builds one from that pair and normalizes it, so the capi, the EROFS reader and the dumpfile parser all agree: a redirect that is the digest's object path gives External (the usual composefs layout), neither gives Sparse, and anything else is ExternalPath. The capi maps payload and digest exactly like C (empty files get no metacopy or redirect, an empty payload writes no redirect). RegularFile::backing_path() gives the redirect target of either external variant, which composefs-info ls/objects/missing-objects and cfsctl's backing-path dump now list, like C's composefs-info does with the payload. RegularFile::repo_object_id() gives callers that open the backing object from a repository the object for either variant.

This is a public API change, and so is dumpfile_parse::Item::Regular's path becoming an Option (see below). bootc needs a small change for both (bot/composefs-externalpath on cgwalters-bot/bootc, on top of bootc-dev/bootc#2490): four ExternalNoVerity matches and one Item::Regular in its GC. That only matters when bootc bumps to a composefs-rs release with this; the revdep workflow is disabled until then anyway.

Tested on a 16-core RHEL 10 devspace (kernel 6.12):

  • OSTREE_IMAGE_C_DIGEST comes from the C libcomposefs (composefs/composefs ec2573a): tests/ostree-image.c built against it in a Fedora container, then fsverity digest of its output. Besides ostree's forms (.file with and without a digest, an empty file, a symlink), the image has a digest-only file and one whose payload is the digest's object path. The capi test test_ostree_image_matches_c builds the same image through our capi, checks what the reader makes of each file, and gets the same digest.
  • just test-capi passes, including the step that cmps the image from ostree-image.c written by each library, and the C test-checksums.sh/test-units.sh suites.
  • cargo test --workspace --exclude composefs-integration-tests -- --skip fsverity:: in a Fedora container like the fedora CI job: all pass (composefs 219, capi 22). After the last round of fixes, cargo test -p composefs-capi -p composefs-ctl, cargo clippy --workspace -- -D warnings, fmt and just test-capi were rerun and pass. The fsverity tests were skipped since the container's /var/tmp has no fs-verity. just clippy, check-feature-combos, fmt-check and check-fuzz pass.
  • just test-integration in the same container: 109 passed, 3 failed, and the same 3 fail on main there (no skopeo, no security.selinux on the container's /var/tmp, and a varlink connection reset).
  • bootc: Test ostree against the Rust libcomposefs cgwalters-forge/composefs-rs#4 restacked on this: just bootc/test-ostree (bootc from the branch above) builds the composefs RPMs, confirms libostree and the initramfs load our libcomposefs, and bootc's readonly and image-upgrade-reboot plans on the ostree backend pass, so the deployment boots and upgrades through the Rust libcomposefs.

Design notes:

  • Digest without payload. C writes a metacopy xattr with the digest and no redirect, so overlayfs looks the file up by its own path in the data layer. That is ExternalPath { redirect: None, verity: Some(_) } everywhere now: the capi no longer makes up a redirect from the digest, the reader no longer turns such a file into External (which would add a redirect when the image is rewritten), and the dumpfile parser accepts payload - with a digest, as C does and as our own writer emits. For that, Item::Regular's path is now an Option. Such a file has no backing path, so composefs-info objects doesn't list it, like C.
  • Interior NUL. A redirect with an interior NUL can't be a C payload; converting it for the capi now fails (EINVAL from lcfs_load_node_from_image*, with the error logged at debug) instead of silently dropping the redirect. The test for it has a subdirectory and a hardlinked pair converted before the bad file, so freeing the partial tree is exercised; it passes under ASan (nightly -Zsanitizer=address, with LeakSanitizer) on the devspace.
  • Absolute payloads. composefs-info missing-objects trims leading slashes before joining a payload to --basedir, like C's abs_to_rel_path(), so a /-prefixed payload can't escape it.
  • V2 null chunk index. V2 writes external files as chunk-based with a single chunk (format 31), so the inode needs one null chunk index, which External gets. ExternalNoVerity got none there. That was harmless in practice, since only the capi produced it and the capi writes V1. ExternalPath gets the same index as External. V1 output is unchanged, and no pinned V2 digest moved. Sparse in V2 still gets no chunk index; that looks like the same issue but is left alone here.
  • Stacking. bootc: Test ostree against the Rust libcomposefs cgwalters-forge/composefs-rs#4 (the ostree revdep test) is stacked on this and should land after it.

Related: #323

The Signed-off-by: Colin Walters <walters@verbum.org> on these commits was added on cgwalters's approval of the review draft: cgwalters-forge#7 (review)

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

ostree gives each file a libcomposefs payload of `xx/<checksum>.file` and
sets the fsverity digest separately. The capi turned the payload into an
ObjectID (dropping `.file`) or ignored it when a digest was set, so the
redirect pointed at a file that doesn't exist and an ostree deployment on
the Rust libcomposefs couldn't boot.

ExternalPath carries an optional redirect as given plus an optional
verity digest, which is how libcomposefs models it. RegularFile::external()
builds it from the pair and keeps the usual composefs layout (redirect is
the digest's object path) as External, so the capi, the EROFS reader and
the dumpfile parser all normalize the same way. A digest without a payload
now stays that way everywhere, as in C: no redirect is made up for it, and
the dumpfile parser accepts it. composefs-info lists the redirect as the
object path, like C's, so ostree images don't lose their objects there.
In V2 images ExternalPath gets the same single null chunk index as
External.

Note Item::Regular's path in dumpfile_parse is now an Option, and a
redirect with an interior NUL fails the capi conversion instead of being
dropped.

The new test builds an image like ostree does via the C API and pins its
digest; `just test-capi` compares the same image against the C library.

Generated-by: AI
Signed-off-by: Colin Walters <walters@verbum.org>
Self::External(id, _) => Ok(id.clone()),
Self::ExternalPath {
verity: Some(id), ..
} => Ok(id.clone()),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Couldn't this function return a borrowed value?

Comment on lines +280 to +281
let start = bytes.iter().position(|&b| b != b'/').unwrap_or(bytes.len());
Path::new(OsStr::from_bytes(&bytes[start..]))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Use split_once

use zerocopy::IntoBytes;

let name = CString::new("ostree-image").unwrap();
let fd = unsafe { libc::memfd_create(name.as_ptr(), 0) };

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There's rustix APIs for this ensure need a review checklist item to prefer rustix over libc

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants