Re: [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries

[Daniel Kiper]

On Mon, Aug 08, 2022 at 02:58:06PM -0400, Robbie Harwood wrote:
> Daniel Kiper <dkiper@net-space.pl> writes:
>
> > On Tue, Jul 19, 2022 at 04:39:34PM -0400, Robbie Harwood wrote:
> >
> >> +static grub_err_t
> >> +grub_linux_boot (void)
> >> +{
> >> +  grub_err_t rc = GRUB_ERR_NONE;
> >> +  char *initrd_param;
> >> +  const char *kexec[] = { "kexec", "-l", kernel_path, boot_cmdline, NULL, 
> >> NULL };
> >> +  const char *systemctl[] = { "systemctl", "kexec", NULL };
> >
> > I would prefer if we do not hardcode these commands. E.g. kexec
> > command has many options which can be useful for debugging. If we
> > hardcode the command here we cannot use these options.
>
> Can you clarify what you would like to see instead?  I'm not sure what
> the alternative would be.

E.g. grub.cfg could contain:

set kexec_load_cmd=/sbin/kexec
set kexec_load_opts='-l'

Then GRUB should replace "linux" command with combined kexec_load_cmd,
kexec_load_opts, boot_cmdline, etc. Of course we can assume some reasonable
defaults if kexec_load_cmd and/or kexec_load_opts variables are not present.

> >> +  rc = grub_util_exec (systemctl);
> >> +
> >> +  if (rc == GRUB_ERR_NONE)
> >> +    return rc;
> >> +
> >> +  grub_error (rc, N_("Error trying to perform 'systemctl kexec'"));
> >> +
> >> +  /* need to check read-only root before resetting hard!? */
> >> +  grub_dprintf ("linux", "Performing 'kexec -e -x'");
> >
> > I would really do not fall back to 'kexec -e' by default. It is too
> > dangerous. And again I would not hardcode this command too.
>
> Same question as above regarding the alternative... also, can you

set kexec_exec_cmd=/bin/systemctl
set kexec_exec_opts=kexec

set kexec_exec_force_cmd=/sbin/kexec
set kexec_exec_force_opts='-e -x'

Etc...

Maybe names are not perfect but I hope you know what I mean...

> elaborate on the danger you see here?

The "kexec -e" executes the new kernel immediately without unmounting/remounting
filesystems, etc. So, ... I think this feature should be disabled by default.
If somebody knows what is doing then they should be able to enable it, e.g.,
by setting a GRUB env variable.

> >> +    grub_fatal (N_("Use '"PACKAGE"-emu --kexec' to force a system 
> >> restart."));
> >> +
> >> +  grub_dprintf ("linux", "Performing 'systemctl kexec' (%s) ",
> >> +          (kexecute==1) ? "do-or-die" : "just-in-case");
> >
> > s/kexecute==1/kexecute/
> >
> > Please be more consistent how you check kexecute.
>
> None of this is my code yet - I just rebased the existing patch - but
> I will make these and other requested changes :)  Thanks for the review;
> I'll cut another version once we resolve the conversations above.

Cool! Thank you!

Please do not forget about docs... :-)

Daniel