Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repart: Copy fs-verity status for CopyFiles= #35401

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

allisonkarlitskaya
Copy link
Contributor

When populating a filesytem with CopyFiles=, we first copy the files to a temporary directory. Make sure we use the (new) COPY_FS_VERITY flag when doing that copy so that the mkfs that we invoke can see the files with fs-verity enabled.

Closes #35352

@github-actions github-actions bot added build-system util-lib meson repart please-review PR is ready for (re-)review by a maintainer labels Nov 28, 2024
@allisonkarlitskaya
Copy link
Contributor Author

Note: this obviously requires fs-verity support on the host filesystem, but it additionally requires support for FS_IOC_READ_VERITY_METADATA which presently only works on ext4 and f2fs. There's a patch in the for-next branch of the btrfs tree to enable it for btrfs: btrfs/linux@0af58a6.

@allisonkarlitskaya
Copy link
Contributor Author

Note also: this patch can result in fsync() being called on a O_RDONLY file descriptor. I've done it this way to make sure that the fs-verity metadata is also synced. fsync(2) ("HISTORY" section) mentions that this is forbidden by AT&T UNIX System V Release 4, AIX, and HP-UX, but that POSIX mandates that it work.


return RET_NERRNO(ioctl(*fdt, FS_IOC_ENABLE_VERITY, &enable_arg));
#else
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

-EOPNOTSUPP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If fs-verity is not supported then I think it's more appropriate to report the copy of all known fs-verity attributes as trivially succeeding. I can add a comment for that, if you like.

Otherwise all copies that request verity will fail. And as of this PR systemd-repart requests it unconditionally.

Copy link
Member

Choose a reason for hiding this comment

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

we usually return EOPNOTSUPP in these cases, and handle it in the caller

@allisonkarlitskaya
Copy link
Contributor Author

Thanks for the review. I've taken most review comments, but I think we might have a minor philosophical disagreement about the return value in the case that the file doesn't have fs-verity enabled. Compare to copy_xattr() for example: this doesn't return EOPNOTSUPP if there are no xattrs on the file...

So at least for the ENODATA case I'm fairly certain that 0 is correct. We could have a discussion about ENOTTY (although in that case there's clearly no fs-verity enabled, as well). The #else block is the one where my argument is least strong, but I think it holds there too.

Also, just pragmatically: it's a lot easier to not have to special-case the EOPNOTSUPP error code once we get back to the calling function... and the actual result is the same (and this function isn't API -- not even internally).

@allisonkarlitskaya allisonkarlitskaya force-pushed the repart-verity branch 2 times, most recently from d976b14 to a938024 Compare November 29, 2024 02:20
int old_fd = *fd;
*fd = new_fd;

return close_nointr(old_fd);
Copy link
Member

Choose a reason for hiding this comment

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

close_and_replace(*fd, new_fd);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I avoided this because the original code in the copy function is very pedantic about checking the return status of close_nointr() and unlinking the new file if it fails. close_and_replace() ignores errno returns from close().

I'm happy to do either way.

Copy link
Member

Choose a reason for hiding this comment

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

close_and_replace() uses safe_close() which uses close_nointr(), hence it's the same pretty much. Please use close_and_replace() hence.

(We generally don't want that any code in our codebase calls plain close() actually, they all should call safe_close() typically, or one of the wrappers of it)

@poettering
Copy link
Member

btw, please click on "resolve conversation" on all review items you have addressed.

@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Nov 29, 2024
@allisonkarlitskaya
Copy link
Contributor Author

btw, please click on "resolve conversation" on all review items you have addressed.

We do this the other way in Cockpit: the person who wrote the comment resolves it when they agree that it's addressed :)

@poettering
Copy link
Member

We do this the other way in Cockpit: the person who wrote the comment resolves it when they agree that it's addressed :)

we are a bit more trusting there. and reviewers are more overloaded than patch writers, hence let's minimize work for them.

@github-actions github-actions bot added documentation please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Dec 2, 2024
@allisonkarlitskaya
Copy link
Contributor Author

(Quick upload just to see what CI thinks. There are still outstanding cleanups.)

@allisonkarlitskaya
Copy link
Contributor Author

Open issues:

  • how strict we want to be about error checking close() in fd_reopen_replace()
  • the return value on copy_fs_verity() in case the file/filesystem doesn't have fs-verity enabled
  • anything CI notices

@@ -18,6 +18,7 @@ int make_filesystem(
const char *root,
sd_id128_t uuid,
bool discard,
bool fsverity,
Copy link
Member

Choose a reason for hiding this comment

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

So we typically have the rule that if a function has >= bool args, it should be turned into a proper flags field. This is qualified for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... I was thinking the same, but was hoping to sneak under the radar ... 😏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is getting a bit ugly in terms of turning boolean values present at the callsite into the correct flags bits.

...how do you feel about multiplication of bool with enum values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fwiw, I just checked. This outputs 6:

    bool b = 5;
    printf("%d\n", b * 6);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it's the difference between something like

   (want_discard ? MKFS_DISCARD : 0) | (want_verity ? MKFS_FS_VERITY : 0)

and

   want_discard * MKFS_DISCARD | want_verity * MKFS_FS_VERITY

I find the multiplication thing to be very deeply weird, and I'm kinda surprised to find myself proposing such a hack, but the fact that it is well-defined (even if the value assigned into the bool was not 1/0) is pretty reassuring. After I understand it, I find the second form much easier to read.

Note the operator precedence works with us in the case of * but against us with ?:, since * > | > ?:.

This is definitely a matter of taste: I let you guys decide.

Copy link
Contributor

Choose a reason for hiding this comment

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

We always do the first form, let's stick to that here as well

This is the same as fd_reopen() except that it takes the original file
descriptor by reference and closes it if the reopen succeeded, replacing
it with the reopened file descriptor.

This is quite useful for the "I need to make this writable file
descriptor read-only before sealing something" case.

Signed-off-by: Allison Karlitskaya <[email protected]>
Add a new member to CopyFlags to request copying of fs-verity status.
We already call FS_IOC_GETFLAGS in the copy code, and it tells us if we
have verity support.  If not, there is no change.

If we do notice a file with fs-verity enabled, and if CopyFlags
requested that we copy this, we request the descriptor from the source
file and use it to setup fs-verity on the destination, using the same
parameters.

Signatures don't seem to be a particularly well-loved (or used) feature
of fs-verity and we don't bother to query them here.  Support for that
could be added later, if desired.

This change means that, with the correct combination of flags, we might
end up calling `fsync()` on a read-only file descriptor.  This is
permitted by POSIX and supported on Linux.

Nothing uses this yet.

Signed-off-by: Allison Karlitskaya <[email protected]>
Add a new `MkfsFlags` enum and use it to replace the existing `quiet`
and `discard` booleans on `make_filesystem()`.

This introduces a strange approach to converting booleans into flags
values: multiplication.  This is done because:

  bool a, b;
  ..., FLAG_A * a | FLAG_B * b, ...

is a lot less visually noisy than

  ..., (a ? FLAG_A : 0) | (b ? FLAG_B : 0), ...

(Note that the extra brackets are required only in the ternary case
because `|` has lower precedence than `*` but higher than `?:`.)

It also works even if the original value assigned to the `bool` was
something other than 1 or 0.  I was also unable to get either `gcc` or
`clang` to produce different behaviour or complain about it across a
wide range of `-std` and `-W` options.

Signed-off-by: Allison Karlitskaya <[email protected]>
Add an fsverity flag to MkfsFlags and use it to pass the `-O verity`
option when creating an ext4 or f2fs filesystem: they share the same
argument for this.

The only other filesystem that currently supports fs-verity is btrfs and
it doesn't require a flag to be enabled when creating the filesystem.

Nothing uses this yet.

Signed-off-by: Allison Karlitskaya <[email protected]>
We currently convert the source:target pairs of the `CopyFiles=` lines
in `repart.d` files into a pairwise strv.  This works great if the only
thing that can be specified is a source and a target, but we're about to
add a flags field.

Let's start by making this a bit more explicit: we now turn each
`CopyFiles=` line into a `CopyFilesLine` struct.  We keep an array of
those in the `Partition` now, instead of the strv.

So far this is a whole lot of added complexity for nothing, but it's
necessary for the next step.

Signed-off-by: Allison Karlitskaya <[email protected]>
We currently pass the CopyFlags that we use to populate the temporary
directory in the form of a constant at each of the copy_tree_at() call
sites.  De-duplicate that and move it into the `CopyFilesLine` struct,
initializing it from the parser.

Add our first non-constant flag: `fsverity=`.  This can be set to `off`
(the default) or `copy`, in which case we copy the fs-verity state from
the source files.

This arrangement is amenable to the introduction of more flags to
`CopyFiles=` lines, if we want to add them in the future.

Update the `repart.d(5)` manpage.

Closes systemd#35352

Signed-off-by: Allison Karlitskaya <[email protected]>
@allisonkarlitskaya
Copy link
Contributor Author

nudge Any other feedback here?


if (r < (int) sizeof desc)
/* This is really quite unexpected... */
return -EINVAL;
Copy link
Member

Choose a reason for hiding this comment

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

we typically raise EINVAL if we determine data to be invalid that some code passed to us. But if we get back something from the kernel we should probably raise something else, or probably better assert() on it, because apparently we are not using the kernel apis right, and that'd be our bug.

if (r < 0)
return r;

r = fd_reopen_replace(fdt, O_RDONLY | O_CLOEXEC | O_NOCTTY);
Copy link
Member

Choose a reason for hiding this comment

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

why not use:

_cleanup_close int reopened_fd = -EBADF;
r = fd_reopen_condition(fdt, O_RDONLY|O_CLOEXEC|O_NOCTTY, O_ACCESS|O_PATH, &reopened_fd);
if (r < 0)
        return r;
if (reopened_fd >= 0)
        close_and_replace(fdt, reopened_fd);

This is a bit nicer since it means the whole function will become generic, i.e. can deal with O_PATH and O_RDWR fds, and will do the right thing of only reopening it in that case but not otherwise.

struct fsverity_enable_arg enable_arg = {
.version = desc.version,
.hash_algorithm = desc.hash_algorithm,
.block_size = 1u << desc.log_blocksize,
Copy link
Member

Choose a reason for hiding this comment

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

UINT32_C(1)

#include "strv.h"
typedef enum MkfsFlags {
MKFS_QUIET = 1 << 0, /* Suppress mkfs command output */
MKFS_DISCARD = 1 << 1, /* Enable 'discard' mode on the filesystem */
Copy link
Member

Choose a reason for hiding this comment

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

not that it really matters, but that's a lot of inner space.

alignment around = is great, but maybe a bit less space, but of course, doesn't really matter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was copy-paste from an existing flags field (maybe the copy one?)

Happy to shrink it down, but I guess it will grow again at some point...


override_copy_files = ptr;
ptr = mempcpy(ptr, p->copy_files, p->n_copy_files);
ptr = mempcpy(ptr, p->suppressing->copy_files, p->suppressing->n_copy_files);
Copy link
Member

Choose a reason for hiding this comment

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

hmm, you are copying by byte size here, not by elements? what am i missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. I meant to be using the mempcpy_typesafe macro which does the multiplication by the element size. Thanks for the catch!

if (r < 0)
return r;
n_copy_files = p->n_copy_files + p->suppressing->n_copy_files;
CopyFilesLine *ptr = new(CopyFilesLine, n_copy_files);
Copy link
Member

Choose a reason for hiding this comment

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

just use greed_realloc_append()

free(f);
}


Copy link
Member

Choose a reason for hiding this comment

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

double empty line

if (!STR_IN_SET(val, "off", "copy"))
return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "fsverity= expects either 'off' or 'copy'");

if (strcmp(val, "copy") == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

streq()

@poettering
Copy link
Member

sorry for the late review, done now.

@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

repart: support creating CopyFile= filesystems with fs-verity files
5 participants