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

[Daniel Kiper]

On Tue, Jul 19, 2022 at 04:39:34PM -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 fallbacks to executing the kexec user-space tool directly.
>
> 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 | 182 +++++++++++++++++++++++++++++++++++
>  include/grub/emu/exec.h      |   4 +-
>  include/grub/emu/hostfile.h  |   3 +-
>  include/grub/emu/misc.h      |   3 +
>  8 files changed, 213 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..b4da9d21cf 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."), 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..020cf56cd1
> --- /dev/null
> +++ b/grub-core/loader/emu/linux.c
> @@ -0,0 +1,182 @@
> +/*
> + *  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/>.

Please add a blurb, after an empty line, saying what this code does.
You can find good example in grub-core/kern/efi/sb.c.

> + */
> +
> +#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", "-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.

> +  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 curly brackets.

> +  grub_dprintf ("linux", "%serforming 'kexec -l %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);

Missing space before "(".

> +  if (rc != GRUB_ERR_NONE)
> +    {
> +      grub_error (rc, N_("Error trying to perform kexec load operation."));
> +      grub_sleep (3);
> +      return rc;
> +    }
> +
> +  if (kexecute < 1)

s/kexecute < 1/kexecute/

> +    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.

> +  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.

> +  kexec[1] = "-e";
> +  kexec[2] = "-x";
> +  kexec[3] = NULL;
> +  rc = grub_util_exec (kexec);
> +  if ( rc != GRUB_ERR_NONE )
> +    grub_fatal (N_("Error trying to directly perform 'kexec -e'."));
> +
> +  return rc;
> +}
> +
> +static grub_err_t
> +grub_linux_unload (void)
> +{
> +  grub_dl_unref (my_mod);
> +  if ( boot_cmdline != NULL )

You can drop this if.

> +    grub_free (boot_cmdline);

What about kernel_path and initrd_path?

> +  boot_cmdline = NULL;

Module will be destroyed. So, I do not think this is needed.

> +  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]) )

Too many spaces...

> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find kernel file 
> %s"), argv[0]);

s/Cannot/cannot/

> +  if ( kernel_path != NULL )

You can drop this if. Of course everything should be initialized to NULL
above.

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

Again, too many spaces... But this if seems redundant too.

> +    {
> +      grub_free(boot_cmdline);

Missing space before "("... :-)

> +      boot_cmdline = NULL;
> +    }
> +
> +  if ( argc > 1 )

Ditto and below please...

> +    {
> +      boot_cmdline = grub_xasprintf("--command-line=%s", argv[1]);
> +      for ( i = 2; i < argc; i++ )
> +        {
> +          tempstr = grub_xasprintf("%s %s", boot_cmdline, argv[i]);
> +          grub_free(boot_cmdline);
> +          boot_cmdline = tempstr;
> +        }
> +    }
> +
> +  grub_loader_set (grub_linux_boot, grub_linux_unload, 0);
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), int argc, char 
> *argv[])
> +{
> +  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 initrd file 
> %s"), argv[0]);

s/Cannot/cannot/ All error messages should start with lowercase.

> +  if ( initrd_path != NULL )
> +    grub_free (initrd_path);

Again, redundant if...

> +
> +  initrd_path = grub_xasprintf("%s", argv[0]);
> +
> +  grub_dl_unref (my_mod);
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_command_t cmd_linux, cmd_initrd;
> +
> +GRUB_MOD_INIT (linux)
> +{
> +  cmd_linux = grub_register_command ("linux", grub_cmd_linux, 0, N_("Load 
> Linux."));
> +  cmd_initrd = grub_register_command ("initrd", grub_cmd_initrd, 0, N_("Load 
> initrd."));
> +  my_mod = mod;
> +  kernel_path = NULL;
> +  initrd_path = NULL;
> +  boot_cmdline = NULL;

This can be done in variable definitions above.

Daniel