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

[Daniel Kiper]

On Tue, Oct 04, 2022 at 03:16:48PM -0400, Robbie Harwood wrote:
> From: Raymund Will <rw@suse.com>
>
> The GRUB emulator is used as a debugging utility but it could also be
> used as a user-space bootloader if there is support to boot an operating
> system.
>
> The Linux kernel is already able to (re)boot another kernel via the
> kexec boot mechanism.  So the grub-emu tool could rely on this feature
> and have linux and initrd commands that are used to pass a kernel,
> initramfs image and command line parameters to kexec for booting a
> selected menu entry.
>
> 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 can fall back to executing the kexec user-space
> tool directly.  The ability to force a kexec-reboot when systemctl kexec
> fails must only be used in controlled environments to avoid possible
> filesystem corruption and data loss.
>
> Signed-off-by: Raymund Will <rw@suse.com>
> Signed-off-by: John Jolly <jjolly@suse.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>
> ---
>  grub-core/Makefile.am        |   1 +
>  grub-core/Makefile.core.def  |   2 +-
>  grub-core/kern/emu/main.c    |   4 +
>  grub-core/kern/emu/misc.c    |  18 +++-
>  grub-core/loader/emu/linux.c | 181 +++++++++++++++++++++++++++++++++++
>  include/grub/emu/exec.h      |   4 +-
>  include/grub/emu/hostfile.h  |   3 +-
>  include/grub/emu/misc.h      |   3 +

Please document this functionality in the user documentation.

>  8 files changed, 212 insertions(+), 4 deletions(-)
>  create mode 100644 grub-core/loader/emu/linux.c
>
> diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
> index ee88e44e97..80e7a83edf 100644
> --- a/grub-core/Makefile.am
> +++ b/grub-core/Makefile.am
> @@ -307,6 +307,7 @@ KERNEL_HEADER_FILES += 
> $(top_srcdir)/include/grub/emu/net.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostdisk.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostfile.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/extcmd.h
> +KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/exec.h
>  if COND_GRUB_EMU_SDL
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/sdl.h
>  endif
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 7159948721..5350408601 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1816,9 +1816,9 @@ module = {
>    arm64 = loader/arm64/linux.c;
>    riscv32 = loader/riscv/linux.c;
>    riscv64 = loader/riscv/linux.c;
> +  emu = loader/emu/linux.c;
>    common = loader/linux.c;
>    common = lib/cmdline.c;
> -  enable = noemu;
>  };
>
>  module = {
> diff --git a/grub-core/kern/emu/main.c b/grub-core/kern/emu/main.c
> index 44e087e988..855b11c3de 100644
> --- a/grub-core/kern/emu/main.c
> +++ b/grub-core/kern/emu/main.c
> @@ -107,6 +107,7 @@ static struct argp_option options[] = {
>     N_("use GRUB files in the directory DIR [default=%s]"), 0},
>    {"verbose",     'v', 0,      0, N_("print verbose messages."), 0},
>    {"hold",     'H', N_("SECS"),      OPTION_ARG_OPTIONAL, N_("wait until a 
> debugger will attach"), 0},
> +  {"kexec",       'X', 0,      0, N_("use kexec to boot Linux kernels via 
> systemctl (pass twice to enable dangerous fallback to non-systemctl)."), 0},
>    { 0, 0, 0, 0, 0, 0 }
>  };
>
> @@ -164,6 +165,9 @@ argp_parser (int key, char *arg, struct argp_state *state)
>      case 'v':
>        verbosity++;
>        break;
> +    case 'X':
> +      grub_util_set_kexecute ();
> +      break;
>
>      case ARGP_KEY_ARG:
>        {
> diff --git a/grub-core/kern/emu/misc.c b/grub-core/kern/emu/misc.c
> index d0e7a107e7..521220b49d 100644
> --- a/grub-core/kern/emu/misc.c
> +++ b/grub-core/kern/emu/misc.c
> @@ -39,6 +39,7 @@
>  #include <grub/emu/misc.h>
>
>  int verbosity;
> +int kexecute;
>
>  void
>  grub_util_warn (const char *fmt, ...)
> @@ -82,7 +83,7 @@ grub_util_error (const char *fmt, ...)
>    vfprintf (stderr, fmt, ap);
>    va_end (ap);
>    fprintf (stderr, ".\n");
> -  exit (1);
> +  grub_exit ();
>  }
>
>  void *
> @@ -153,6 +154,9 @@ xasprintf (const char *fmt, ...)
>  void
>  grub_exit (void)
>  {
> +#if defined (GRUB_KERNEL)
> +  grub_reboot ();
> +#endif
>    exit (1);
>  }
>  #endif
> @@ -214,3 +218,15 @@ grub_util_load_image (const char *path, char *buf)
>
>    fclose (fp);
>  }
> +
> +void
> +grub_util_set_kexecute (void)
> +{
> +  kexecute++;
> +}
> +
> +int
> +grub_util_get_kexecute (void)
> +{
> +  return kexecute;
> +}
> diff --git a/grub-core/loader/emu/linux.c b/grub-core/loader/emu/linux.c
> new file mode 100644
> index 0000000000..bdcdbb0ff4
> --- /dev/null
> +++ b/grub-core/loader/emu/linux.c
> @@ -0,0 +1,181 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2006,2007,2008,2009,2010  Free Software Foundation, Inc.

s/2006,2007,2008,2009,2010/2022/

> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/loader.h>
> +#include <grub/dl.h>
> +#include <grub/command.h>
> +#include <grub/time.h>
> +
> +#include <grub/emu/exec.h>
> +#include <grub/emu/hostfile.h>
> +#include <grub/emu/misc.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static grub_dl_t my_mod;
> +
> +static char *kernel_path;
> +static char *initrd_path;
> +static char *boot_cmdline;
> +
> +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 };
> +  const char *systemctl[] = { "systemctl", "kexec", NULL };

Please drop spaces after "{" and before "}".

> +  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", "");
> +    }

Please drop these redundant curly braces.

> +  grub_dprintf ("linux", "%serforming 'kexec -la %s %s %s'\n",
> +                (kexecute) ? "P" : "Not p",
> +                kernel_path, initrd_param, boot_cmdline);
> +
> +  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."));

All error messages should start with lowercase and do not contain full
stop at the end. Fix similar issues below too...

> +      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_fatal (N_("Error trying to perform 'systemctl kexec'"));

I would add '": %d", rc' to the end of fatal message.

> +  /* WARNING: forcible reset should only be used in read-only environments.
> +   * grub-emu cannot check for these - users beware. */

Please format comment properly [1].

> +  grub_dprintf ("linux", "Performing 'kexec -e -x'");
> +  kexec[1] = "-e";
> +  kexec[2] = "-x";
> +  kexec[3] = NULL;
> +  rc = grub_util_exec (kexec);
> +  if ( rc != GRUB_ERR_NONE )

Please drop space after "(" and before ")". Fix similar issues below too...

> +    grub_fatal (N_("Error trying to directly perform 'kexec -e'."));

Please display rc value as above.

> +  return rc;
> +}
> +
> +static grub_err_t
> +grub_linux_unload (void)
> +{
> +  grub_dl_unref (my_mod);

I understand that this marks this module as not in use. Right? If yes
please add a comment before grub_dl_unref() call what exactly happens
here and why. It took me a while to understand why it is needed. Please
do the same below for all grub_dl_ref() and grub_dl_unref() calls.

> +  if ( boot_cmdline != NULL )

This if is redundant.

> +    grub_free (boot_cmdline);
> +  boot_cmdline = NULL;
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)), int argc, char 
> *argv[])
> +{
> +  int i;
> +  char *tempstr;
> +
> +  grub_dl_ref (my_mod);
> +
> +  if (argc == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +
> +  if ( !grub_util_is_regular (argv[0]) )
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find kernel file 
> %s"), argv[0]);
> +
> +  if ( kernel_path != NULL )

This if is redundant too.

> +    grub_free (kernel_path);
> +
> +  kernel_path = grub_xasprintf ("%s", argv[0]);
> +
> +  if ( boot_cmdline != NULL )

Ditto. Please fix similar issues below too...

> +    {
> +      grub_free(boot_cmdline);

Missing space before "(". Same things should be fixed below too...

Daniel

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments