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

BLS: only write /etc/kernel/cmdline if writable #106

Merged
merged 2 commits into from
Aug 17, 2022

Conversation

jlebon
Copy link
Contributor

@jlebon jlebon commented Aug 17, 2022

On OSTree systems, grub2-mkconfig is run with /etc mounted read-only
because as part of the promise of transactional updates, we want to make
sure that we're not modifying the current deployment's state (/etc or
/var).

This conflicts with 0837dcd ("BLS: create /etc/kernel/cmdline during
mkconfig") which wants to write to /etc/kernel/cmdline. I'm not
exactly sure on the background there, but based on the comment I think
the intent is to fulfill grubby's expectation that the file exists.

However, in systems like Silverblue, kernel arguments are managed by the
rpm-ostree stack and grubby is not shipped at all.

Adjust the script slightly so that we only write /etc/kernel/cmdline
if the parent directory is writable.

In the future, we're hoping to simplify things further on rpm-ostree
systems by not running grub2-mkconfig at all since libostree already
directly writes BLS entries. Doing that would also have avoided this,
but ratcheting it into existing systems needs more careful thought.

Signed-off-by: Jonathan Lebon [email protected]

Fixes: fedora-silverblue/issue-tracker#322

Copy link

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

I didn't dig into the history here, but it seems really strange to me that the grub config generation process writes to /etc. I'm sure there's a way we can architect things here to avoid that.

Anyways, LGTM as is for now.

Signed-off-by: Robbie Harwood <[email protected]>
@frozencemetery
Copy link
Member

Not our choice - this is where kernel-install reads the config from. Thanks for the PR.

On OSTree systems, `grub2-mkconfig` is run with `/etc` mounted read-only
because as part of the promise of transactional updates, we want to make
sure that we're not modifying the current deployment's state (`/etc` or
`/var`).

This conflicts with 0837dcd ("BLS: create /etc/kernel/cmdline during
mkconfig") which wants to write to `/etc/kernel/cmdline`. I'm not
exactly sure on the background there, but based on the comment I think
the intent is to fulfill grubby's expectation that the file exists.

However, in systems like Silverblue, kernel arguments are managed by the
rpm-ostree stack and grubby is not shipped at all.

Adjust the script slightly so that we only write `/etc/kernel/cmdline`
if the parent directory is writable.

In the future, we're hoping to simplify things further on rpm-ostree
systems by not running `grub2-mkconfig` at all since libostree already
directly writes BLS entries. Doing that would also have avoided this,
but ratcheting it into existing systems needs more careful thought.

Signed-off-by: Jonathan Lebon <[email protected]>

Fixes: fedora-silverblue/issue-tracker#322
@frozencemetery frozencemetery merged commit 3c3d1a3 into rhboot:fedora-38 Aug 17, 2022
@cgwalters
Copy link

Not our choice - this is where kernel-install reads the config from.

(Is there a better place to discuss this design? Maybe an issue on this repo?)

For ostree at least, the default source-of-truth for the kernel command line is the current bootloader entry. So in the case where the kernel arguments don't change, we just propagate the current options into the new entry.

In the case where they do change (e.g. rpm-ostree kargs --append=nosmt) we end up serializing the new desired state into a file in /run, which is applied when the new bootloader entry is created (during shutdown time as part of ostree-finalize-staged).

It seems to me that if /etc/kernel/default doesn't exist, the logic here could use /proc/cmdline for example?

@frozencemetery
Copy link
Member

I suggest taking it up with kernel-install if you want to debate the design. But I think there's misunderstanding of what this change is for.

The current bootloader entry is the source of truth for only the current bootloader entry. kernel-install is responsible for creating a new entry. To do this, it needs to start from some set of default args for that new entry - and that's what /etc/kernel/cmdline and /usr/lib/kernel/cmdline are for. If neither of those are set, it falls back to /proc/cmdline - but that's just taking the current args as the default for new args, which isn't ideal.

From kernel-install(8):

   /usr/lib/kernel/cmdline /etc/kernel/cmdline /proc/cmdline
       Read by 90-loaderentry.install. The content of the file
       /etc/kernel/cmdline specifies the kernel command line to use. If
       that file does not exist, /usr/lib/kernel/cmdline is used. If that
       also does not exist, /proc/cmdline is used.

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.

Cannot update silverblue (grub2-mkconfig error)
3 participants