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

rofiles-fuse: Check fsverity flag for copyup #3175

Merged
merged 3 commits into from
Feb 15, 2024

Conversation

cgwalters
Copy link
Member

rofiles-fuse: Port to statx

This allows us to query fsverity efficiently.


rofiles-fuse: Check fsverity flag for copyup

We need to do a copyup if fsverity is enabled.
Sadly to do this we can't just use ostree_break_hardlink
as is.


This allows us to query fsverity efficiently.
@alexlarsson
Copy link
Member

This looks good to me, and it works on fedora and CS9, and I think using statx (supported since glibc 2.28) is fine. However STATX_ATTR_VERITY was added in Linux 5.5 and glibc 2.32, which was in 2020. Maybe we should add an ifdef for that to at least build on older systems?

@alexlarsson
Copy link
Member

Btw, this test depends on the "fsverity" tool from fsverity-utils, which is not currently in CS9 (it is in epel though). Maybe the test should check for it?

We need to do a copyup if fsverity is enabled.
Sadly to do this we can't just use ostree_break_hardlink
as is.
The logic simplified, so we don't need it anymore.
@cgwalters
Copy link
Member Author

However STATX_ATTR_VERITY was added in Linux 5.5 and glibc 2.32, which was in 2020. Maybe we should add an ifdef for that to at least build on older systems?

Done.

Btw, this test depends on the "fsverity" tool from fsverity-utils, which is not currently in CS9 (it is in epel though). Maybe the test should check for it?

Done.


BTW...I have this lingering vague concern about the changes here to do_open(). Previously we had a complex dance of potentially opening the file twice to deal with O_TRUNC etc. I maybe what I was trying to do before is avoid race conditions, but in practice we don't need to worry about concurrency on the mount, and the lower shouldn't be concurrently mutated. Does it seem sane to you?

Bigger picture this is a nontrivial change so I'd like to have this soak a bit in git main and we can try it out w/rpm-ostree etc.

@cgwalters
Copy link
Member Author

Also reworked this with some more cleanups; I started using ostree_break_hardlink for the symlink case to notably lessen code duplication.

@alexlarsson
Copy link
Member

BTW...I have this lingering vague concern about the changes here to do_open(). Previously we had a complex dance of potentially opening the file twice to deal with O_TRUNC etc. I maybe what I was trying to do before is avoid race conditions, but in practice we don't need to worry about concurrency on the mount, and the lower shouldn't be concurrently mutated. Does it seem sane to you?

The only two reasons I can see for the original code was to either avoid races, or as an optimization to avoid resolving the path twice. However, given that we don't really care about concurrent mutators of the lower, I don't see anything wrong with the new approach. What would the worry be? The stat seeing a non-hardlinked file, and then something replaces the filename with a hard-link to the repo that we open writable. In some kind of generic usecase it may be a problem, but as used in ostree where the lower dir is not meant to change it doesn't seem like a real risk

And anyway, I don't see how the original code would help much against concurrent mutators anyway in a more "generic" usecase. Sure, you decide whether to copy-up or not based on the single resolve done during open(), but someone could create a second link to the file between the open and the writes to it.

@alexlarsson
Copy link
Member

So, lgtm

@cgwalters cgwalters merged commit f46cc0c into ostreedev:main Feb 15, 2024
24 checks passed
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.

None yet

2 participants