Add efi variable to augment /proc/cmdline by keszybz · Pull Request #13246 · systemd/systemd

keszybz · 2019-08-01T16:11:58Z

No description provided.

keszybz · 2019-08-02T07:13:54Z

It seems that the first patch for cg_unified_flush()/cg_unified_update() is wrong.

keszybz · 2019-08-02T13:17:20Z

I reworked the first patch. It passes on its own, so the rest should now pass too.

evverx · 2019-08-02T20:05:50Z

Regarding test-journal-flush, it also failed in #13250, which most likely means it has nothing to do with this PR but as far as I can tell, the test isn't exactly a unit test in the sense that it relies on the journal not being corrupted and it's probably going to fail until the image used on Ubuntu CI gets updated.

poettering · 2019-08-06T08:00:23Z

src/basic/efivars.c

+                return 0;
+        }
+
+        r = efi_get_variable_string(EFI_VENDOR_LOADER, "SystemdOptions", line);


hmm, shouldn't this maybe something the loader appends, not userspace?

i mean, in particular as you put this in the loader UEFI UUID namespace?

The capitalization of "SystemdOptions" is a bit weird...

Shouldn't this be KernelCommandLineAppend or so, and then simply be appended by the boot loader or the EFI stub? I mean, otherwise people would try to add "audit=0 selinux=0" and then be very annoyed and surprised if it doesn't work.

Also, I am pretty sure this should also come in a oneshot flavour, so that you can append "fsck.mode=force" for the next boot only.

hmm, shouldn't this maybe something the loader appends, not userspace?

I wanted this to work independently of the loader. In particular, I'm thinking about providing an alternative way to say 'systemd.uniified-cgroup-hierarchy=no' in F31 after we switch the default. For the loader, we already have a well-established way to set options. I guess it'd make sense to add something like this too sd-boot too, but for me that's an independent issue.

as you put this in the loader UEFI UUID namespace?

Good point. I'll move it to a different namespace.

The capitalization of "SystemdOptions" is a bit weird...

Suggestions?

KernelCommandLineAppend

As you say, this would then confuse people because it's only about systemd in this patch. I think KernelCommandLineAppend (or Prepend?) could be used for the sd-boot option.

onshot flavour

OK, will add.

Actually, what does EFI_VENDOR_LOADER and EFI_VENDOR_GLOBAL mean? What namespace would be reasonable for "systemd options"?

introduce a new namespace for that. It's just a uuid you can make up.

I am not sure doing a new efi var that feels like a kernel cmdline string but also totally isnt is the best way forward.

I see two options:

This could be an option read by the EFI loader stub, and not the EFI boot menu, and is added implicitly to the kernel cmdline.

Instead of adding a string formatted option, instead add explicitly settings, i.e. EFI_VENDOR_SYSTEMD:DefaultUnit= for systemd.unit= EFI_VENDOR_SYSTEMD:FileSystemCheck= for systemd.fsck= and so on. i.e. make them high-level variables, and only expose the ones that have the strong usecase. In this case they feel distinct enough not to be confused with a kernel cmdline option.

ntroduce a new namespace for that. It's just a uuid you can make up.

That's what I did.

This could be an option read by the EFI loader stub, and not the EFI boot menu, and is added implicitly to the kernel cmdline.

This doesn't really solve the goal: I want to have to have a way to amend systemd options independently of the boot loader used. And I want this to work "now", i.e. when we are pushing cgroups v2 in Fedora. Some future solution that requires discussion and changes in other projects doesn't really help.

Instead of adding a string formatted option, instead add explicitly settings, i.e. EFI_VENDOR_SYSTEMD:DefaultUnit= for systemd.unit= EFI_VENDOR_SYSTEMD:FileSystemCheck= for systemd.fsck=

We already expose only specific settings for overriding on the kernel command line, so I think the list that is overridable through EFI should be the same. Otherwise things would be confusing and harder to maintain for no good reason.

OK, I'll try to amend the patch like this.

keszybz · 2019-08-06T16:40:50Z

Updated:

renamed efi_options_variable → efi_systemd_options_variable
new EFI_VENDOR_SYSTEMD namespace for the variable

Fix commits from #13246

keszybz · 2019-09-16T16:04:15Z

Rebased and updated (slightly):

the docs are now very verbose that the EFI variable is not a replacement for the /proc/cmdline and only applies to systemd

@poettering wrote:

Instead of adding a string formatted option, instead add explicitly settings, i.e. EFI_VENDOR_SYSTEMD:DefaultUnit= for systemd.unit= EFI_VENDOR_SYSTEMD:FileSystemCheck= for systemd.fsck= and so on. i.e. make them high-level variables, and only expose the ones that have the strong usecase.

I tried that, but this much more annoying (to implement and to use). The biggest issue is that I think we should simply allow all variables that are parsed on the kernel command line to be parsed in this manner. If we start with some that "have a strong use case", then inevitably people will ask for additional ones. We'll be playing whack-a-mole, and there will no clear way to figure out if a given systemd version supports a given variable or not. I think that if we decided to implement command-line parsing for a given variable, than that's a strong-enough use case to also support overriding it through the EFI variable. But once we accept that we allow multiple variables, making a separate EFI variable for each and every one is ugly. In particular, there are variables which modify the same setting, e.g. 3 and systemd.unit= and single. If we allowed them to be set through separate variables, some way to resolve the order needs to be designed. If we ever stop looking at some variable, or there is a variable which is supported in a newer systemd version set by the user, there is no way to "clean up", and there is no way to tell the user that a variable is unsupported. Also, a separate brand new naming scheme is necessary to use for the variable names (systemd.fsck↔FileSystemCheck, …). All in all, I think that the current approach is much more user friendly without bringing unnecessary complexity. As I understand, the motivation to split into separate variables is to avoid confusion between this and the kernel command line. I think that the additions I added to the docs now are enough to serve this purpose.

…rarchy This avoid the use of the global variable. Also rename cgroup_unified_update() to cgroup_unified_cached() and cgroup_unified_flush() to cgroup_unified() to better reflect their new roles.

…-setup This way less stuff needs to be in basic. Initially, I wanted to move all the parts of cgroup-utils.[ch] that depend on efivars.[ch] to shared, because efivars.[ch] is in shared/. Later on, I decide to split efivars.[ch], so the move done in this patch is not necessary anymore. Nevertheless, it is still valid on its own. If at some point we want to expose libbasic, it is better to to not have stuff that belong in libshared there.

It if of course related to /proc/cmdline parsing, but is higher-level functionality built on top of it. It should be in shared/ because it is something to be used by pid1 and related utilities, not something for level-level libraries.

It's just a small function, but it is higher-level functionality. I don't see a good place for it, reboot-util.[ch] seems least bad

I want to use efivars.[ch] in proc-cmdline.c, but most of the efivars stuff is not needed in basic/. Move the file from shared/ to basic/, but then move back most of the higher-level functions to the new shared/efi-loader.c file.

In various circumstances, overriding the kernel commandline can be inconvenient. People have different bootloaders, and e.g. the grub config can be pretty scary. grubby helps, but it isn't always available. This option adds an alternative mechanism that can quite convenient on EFI systems. cmdline settings have higher priority, because they can be (usually) changed on the bootloader prompt. $SYSTEMD_EFI_OPTIONS can be used to override, same as $SYSTEMD_PROC_CMDLINE.

keszybz · 2019-10-01T15:36:37Z

This was reviewed a few times, and although there were questions about the user interface, and I think they have been answered. I'm happy with how this works. If nobody objects, I'll merge this.

keszybz added pid1 util-lib sd-boot/sd-stub/bootctl ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR and removed ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels Aug 1, 2019

keszybz force-pushed the add-SystemdOptions-efi-variable branch from 6c4f40a to 1ab24a5 Compare August 1, 2019 19:20

keszybz mentioned this pull request Aug 2, 2019

Rework unified cache #13249

Closed

keszybz added ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR and removed ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels Aug 2, 2019

keszybz force-pushed the add-SystemdOptions-efi-variable branch from 1ab24a5 to 8a0d887 Compare August 2, 2019 13:16

keszybz added the ci-failure-appears-unrelated label Aug 3, 2019

evverx removed the ci-failure-appears-unrelated label Aug 4, 2019

poettering reviewed Aug 6, 2019

View reviewed changes

keszybz force-pushed the add-SystemdOptions-efi-variable branch from 8a0d887 to 1bacc72 Compare August 6, 2019 16:40

keszybz added a commit that referenced this pull request Aug 22, 2019

Merge pull request #13365 from keszybz/fix-commits-from-pr-13246

d0696f3

Fix commits from #13246

keszybz force-pushed the add-SystemdOptions-efi-variable branch from 1bacc72 to 3f7b219 Compare September 16, 2019 15:50

keszybz force-pushed the add-SystemdOptions-efi-variable branch from 3f7b219 to 71108a9 Compare September 16, 2019 16:04

keszybz added 8 commits September 16, 2019 18:06

basic/cgroup-util: let cgroup_unified_flush() return the detected hie…

d4d99bc

…rarchy This avoid the use of the global variable. Also rename cgroup_unified_update() to cgroup_unified_cached() and cgroup_unified_flush() to cgroup_unified() to better reflect their new roles.

util-lib: move yes_no() and friends to string-util.h

55fced5

util-lib: move shall_restore_state() to shared/reboot-util

2bfa846

It's just a small function, but it is higher-level functionality. I don't see a good place for it, reboot-util.[ch] seems least bad

test-proc-cmdline: use test_setup_logging()

e825ea2

keszybz added 2 commits September 16, 2019 18:08

man: document SystemdOptions variable

28f0aef

bootctl: add "system-options" verb

4e5aa79

keszybz force-pushed the add-SystemdOptions-efi-variable branch from 71108a9 to 4e5aa79 Compare September 16, 2019 16:09

keszybz merged commit 86e94d9 into systemd:master Oct 3, 2019

keszybz deleted the add-SystemdOptions-efi-variable branch October 3, 2019 13:24

evverx mentioned this pull request Nov 4, 2019

journal: refresh cached credentials of stdout streams #13771

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add efi variable to augment /proc/cmdline#13246

Add efi variable to augment /proc/cmdline#13246
keszybz merged 10 commits intosystemd:masterfrom
keszybz:add-SystemdOptions-efi-variable

keszybz commented Aug 1, 2019

Uh oh!

keszybz commented Aug 2, 2019

Uh oh!

keszybz commented Aug 2, 2019

Uh oh!

evverx commented Aug 2, 2019

Uh oh!

poettering Aug 6, 2019

Uh oh!

keszybz Aug 6, 2019

Uh oh!

keszybz Aug 6, 2019

Uh oh!

poettering Aug 19, 2019

Uh oh!

keszybz Aug 20, 2019

Uh oh!

keszybz commented Aug 6, 2019

Uh oh!

keszybz commented Sep 16, 2019

Uh oh!

keszybz commented Oct 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

keszybz commented Aug 1, 2019

Uh oh!

keszybz commented Aug 2, 2019

Uh oh!

keszybz commented Aug 2, 2019

Uh oh!

evverx commented Aug 2, 2019

Uh oh!

poettering Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

keszybz Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

keszybz Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

poettering Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

keszybz Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

keszybz commented Aug 6, 2019

Uh oh!

keszybz commented Sep 16, 2019

Uh oh!

keszybz commented Oct 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants