-
Notifications
You must be signed in to change notification settings - Fork 149
store: Preserve /sysroot readonly for read-only operations #1753
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
Conversation
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.
Code Review
This pull request effectively addresses a regression where /sysroot was left writable after read-only operations. The introduction of the Environment enum is a solid architectural improvement that clarifies the execution context and helps manage environment-specific logic, such as when to prepare for write operations. The changes correctly prevent ostree from remounting /sysroot as writable during read-only commands by ensuring a private mount namespace is used.
I've included a couple of suggestions to further refine this logic, mainly to avoid an unnecessary performance impact on composefs systems for read-only commands, while ensuring write operations are still correctly handled for all booted environments. Additionally, the new test case is a great addition for verifying the fix.
crates/lib/src/cli.rs
Outdated
| if env.needs_mount_namespace() { | ||
| prepare_for_write()?; | ||
| } |
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.
Following the suggestion to narrow needs_mount_namespace in store/mod.rs, this check in get_storage should be broader. Since get_storage prepares for write operations, it should call prepare_for_write() for all booted environments (OstreeBooted and ComposefsBooted).
Using a matches! macro here directly makes the intent clear for this write-path, distinguishing it from the read-only path in status::get_host.
if matches!(env, crate::store::Environment::OstreeBooted | crate::store::Environment::ComposefsBooted(_)) {
prepare_for_write()?;
}| pub(crate) fn needs_mount_namespace(&self) -> bool { | ||
| matches!(self, Self::OstreeBooted | Self::ComposefsBooted(_)) | ||
| } |
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.
The needs_mount_namespace method seems overly broad. The issue of /sysroot being remounted as writable is specific to ostree::Sysroot::load(), which is only used in the OstreeBooted environment. The ComposefsBooted environment does not seem to perform any operation that would require a private mount namespace for read-only commands like bootc status.
Including ComposefsBooted here causes an unnecessary re-exec for bootc status on composefs-booted systems.
I suggest narrowing this method to only cover the OstreeBooted case. For write operations, a separate check should be used, which I've suggested in crates/lib/src/cli.rs.
| pub(crate) fn needs_mount_namespace(&self) -> bool { | |
| matches!(self, Self::OstreeBooted | Self::ComposefsBooted(_)) | |
| } | |
| pub(crate) fn needs_mount_namespace(&self) -> bool { | |
| matches!(self, Self::OstreeBooted) | |
| } |
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.
The ComposefsBooted environment does not seem to perform any operation that would require a private mount namespace for read-only commands like bootc status.
Very insightful, but that's actually a different bug.
6c9f64f to
c6f8ee0
Compare
PR bootc-dev#1718 introduced a regression where /sysroot was left writable after running `bootc status`. This occurred because BootedStorage::new() unconditionally calls set_mount_namespace_in_use(), which tells ostree it can safely remount /sysroot as writable. When sysroot.load() is called without actually being in a mount namespace, it leaves the global /sysroot writable. Fix by introducing an Environment enum that detects the runtime environment (ostree, composefs, container, or other) early in the execution flow. Callers now detect the environment and call prepare_for_write() if needed before constructing BootedStorage. This ensures a single call to prepare_for_write() per execution path and eliminates the previous layering violation where storage code called into CLI code. The Environment abstraction also makes it clearer when mount namespace setup is required and provides a foundation for future environment-specific behavior. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>
c6f8ee0 to
f4ad3d0
Compare
|
Yep this added another |
PR #1718 introduced a regression where /sysroot was left writable after running
bootc status. This occurred because BootedStorage::new() unconditionally calls set_mount_namespace_in_use(), which tells ostree it can safely remount /sysroot as writable. When sysroot.load() is called without actually being in a mount namespace, it leaves the global /sysroot writable.Fix by introducing an Environment enum that detects the runtime environment (ostree, composefs, container, or other) early in the execution flow. Callers now detect the environment and call prepare_for_write() if needed before constructing BootedStorage. This ensures a single call to prepare_for_write() per execution path and eliminates the previous layering violation where storage code called into CLI code.
The Environment abstraction also makes it clearer when mount namespace setup is required and provides a foundation for future environment-specific behavior.
Assisted-by: Claude Code (Sonnet 4.5)