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

[Robbie Harwood <rharwood@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4185 bytes --]

Raymund Will <rw@suse.de> writes:

> Robbie Harwood wrote on 2022-08-23T17:15:42 -0400:
>> From: Raymund Will <rw@suse.com>
> [...]
>> By default the systemctl kexec option is used so systemd can shutdown
>> all of the running services before doing a reboot using kexec. But if
>> this is not present, it fallbacks to executing the kexec user-space
>> tool directly.
>
> The last sentence should probably read more like:
>
>   The provision to force a kexec-reboot, in case systemctl kexec
>   fails, must only be used in controlled environments to avoid
>   possible file-system corruption and data-loss.

I can append something to this effect, sure.

> [...]
>> --- /dev/null
>> +++ b/grub-core/loader/emu/linux.c
>> @@ -0,0 +1,180 @@
> [...]
>> +static grub_err_t
>> +grub_linux_boot (void)
>> +{
>> +  grub_err_t rc = GRUB_ERR_NONE;
>> +  char *initrd_param;
>> +  const char *kexec[] = { "kexec", "-la", kernel_path, boot_cmdline, NULL, NULL };
>
> You might have noticed the change in the first parameter to kexec,
> which makes a perfect argument for Daniel's request to have that
> configurable!  But implementation would be quite expensive, maybe
> unless it's strictly restricted to non-whitespace- bearing parameters.
> Would that be sufficient and viable?

Well, just because configuration changed doesn't mean it should be
configurable... my understanding is that -a causes KEXEC_FILE_LOAD to be
tried first, and that without -a it isn't tried at all, so I don't see
the use case for not having -a.

I mentioned in my other email that restricting to parameters that don't
contain whiespace breaks things like --append and --comand-line.  Maybe
that's okay?  I'm just not immediately seeing the use case for it being
configurable, but I could be convinced if someone has one.

>> +  const char *systemctl[] = { "systemctl", "kexec", NULL };
>> +  int kexecute = grub_util_get_kexecute ();
>> +
>> +  if (initrd_path)
>> +    {
>> +      initrd_param = grub_xasprintf ("--initrd=%s", initrd_path);
>> +      kexec[3] = initrd_param;
>> +      kexec[4] = boot_cmdline;
>> +    }
>> +  else
>> +    {
>> +      initrd_param = grub_xasprintf ("%s", "");
>> +    }
>> +
>> +  grub_dprintf ("linux", "%serforming 'kexec -la %s %s %s'\n",
>> +                (kexecute) ? "P" : "Not p",
>> +                kernel_path, initrd_param, boot_cmdline);
>
> Well, the original grub_printf() in this case was very helpful to
> "create" a kexec-load command for cut-n-paste.  Is it really necessary
> to bury it in a ton of debug messages?

I'll defer to Daniel on this, but feedback we've received in the past
has requested that all printing go through the debug infrastructure.

>> +  if (kexecute)
>> +    rc = grub_util_exec (kexec);
>> +
>> +  grub_free(initrd_param);
>> +
>> +  if (rc != GRUB_ERR_NONE)
>> +    {
>> +      grub_error (rc, N_("Error trying to perform kexec load operation."));
>> +      grub_sleep (3);
>> +      return rc;
>> +    }
>> +
>> +  if (kexecute < 1)
>> +    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");
>> +  rc = grub_util_exec (systemctl);
>> +
>> +  if (kexecute == 1)
>> +    grub_error (rc, N_("Error trying to perform 'systemctl kexec'"));
>
> This grub_error() needs to be a grub_fatal() to achieve the intended
> behavior, right?
>
> If kexecute is 1 it should bail out here.  Only if it's bigger the
> forced kexec should be tried!  (Note, that "< 1" is already covered
> above!)

Correct, will fix.  (Peeking behind the curtain, I'm manually merging
your patch with ours, so this kind of checking is appreciated.)

>> +  /* need to check read-only root before resetting hard!? */
>
> This comment probably needs to be replaced with a strict one
> (reflected in GRUB's docs) explaining, that the user takes full
> responsiblity in "force" being exclusively used in read-only
> environments, as grub-emu itself simply can't guarantee this.  (Any
> check here would hardly scratch the surface.)

Okay.  For what it's worth, the openSUSE patch also has the same
comment.

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]