-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
base: main
Are you sure you want to change the base?
Conversation
Note: this obviously requires |
Note also: this patch can result in |
src/shared/copy.c
Outdated
|
||
return RET_NERRNO(ioctl(*fdt, FS_IOC_ENABLE_VERITY, &enable_arg)); | ||
#else | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-EOPNOTSUPP
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
f508e44
to
fb24614
Compare
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 So at least for the Also, just pragmatically: it's a lot easier to not have to special-case the |
d976b14
to
a938024
Compare
int old_fd = *fd; | ||
*fd = new_fd; | ||
|
||
return close_nointr(old_fd); |
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
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 :) |
we are a bit more trusting there. and reviewers are more overloaded than patch writers, hence let's minimize work for them. |
a938024
to
a973100
Compare
(Quick upload just to see what CI thinks. There are still outstanding cleanups.) |
a973100
to
0ace782
Compare
Open issues:
|
src/shared/mkfs-util.h
Outdated
@@ -18,6 +18,7 @@ int make_filesystem( | |||
const char *root, | |||
sd_id128_t uuid, | |||
bool discard, | |||
bool fsverity, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ... 😏
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]>
0ace782
to
6978e50
Compare
nudge Any other feedback here? |
|
||
if (r < (int) sizeof desc) | ||
/* This is really quite unexpected... */ | ||
return -EINVAL; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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 */ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); | ||
} | ||
|
||
|
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
streq()
sorry for the late review, done now. |
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