From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mOMGW-0000Ie-Mi for mharc-grub-devel@gnu.org; Thu, 09 Sep 2021 11:47:04 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:48680) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mOMGV-0000Gd-7x for grub-devel@gnu.org; Thu, 09 Sep 2021 11:47:03 -0400 Received: from dibed.net-space.pl ([84.10.22.86]:34605) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:192) (Exim 4.90_1) (envelope-from ) id 1mOMGS-0007UX-Bt for grub-devel@gnu.org; Thu, 09 Sep 2021 11:47:02 -0400 Received: from router-fw.i.net-space.pl ([192.168.52.1]:50390 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S2111720AbhIIPqx (ORCPT ); Thu, 9 Sep 2021 17:46:53 +0200 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Thu, 9 Sep 2021 17:46:49 +0200 From: Daniel Kiper To: Erwan Velu Cc: grub-devel@gnu.org, alexander.burmashev@oracle.com, phcoder@gmail.com, Erwan Velu Subject: Re: [PATCH v2] kern/efi: Adding efi-watchdog command Message-ID: <20210909154649.me2l7z5wn2x6zf7a@tomti.i.net-space.pl> References: <20210902165035.1986540-1-e.velu@criteo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210902165035.1986540-1-e.velu@criteo.com> User-Agent: NeoMutt/20170113 (1.7.2) Received-SPF: pass client-ip=84.10.22.86; envelope-from=dkiper@net-space.pl; helo=dibed.net-space.pl X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Sep 2021 15:47:03 -0000 On Thu, Sep 02, 2021 at 06:50:35PM +0200, Erwan Velu wrote: > This patch got written by Arthur Mesh from Juniper (now at Apple Sec team). > It was extracted from https://lists.gnu.org/archive/html/grub-devel/2015-09/msg00065.html > > Since this email, the this patch was : > - rebased against the current tree > - added a default timeout value > - reworked to be controlled via mkimage > - reworked to simplify the command line All lines above should go after SOB and "---". Good example what I mean is here [1]. > This commit adds a new command efi-watchdog to manage efi watchdogs. I do not see any reason to name the command "efi-watchdog". It should be simply named as "watchdog". If in the future we will support more watchdog devices we can add a command to switch between them. > On server platforms, this allows GRUB to reset the host automatically Please drop "On server platforms". > if it wasn't able to succeed at booting in a given timeframe. > This usually covers the following issues : > - net boot is too slow and never ends > - the GRUB is unable to find a proper configuration and fails > > Using --efi-watchdog-timeout option at mknetdir time allow users s/efi-watchdog-timeout/watchdog-timeout/ And I am not sure what you mean by "Using ... option at mknetdir time"... > controlling the default value of the timer. > This set the watchdog behavior at the early init of GRUB meaning that > even if grub is not able to load its configuration file, > the watchdog will be triggered automatically. > > Please note that watchdog only covers GRUB itself. The GRUB not always calls ExitBootServices(). So, this is not entirely true. E.g Xen calls ExitBootServices() instead of GRUB on UEFI platforms. > As per the UEFI specification : > The watchdog timer is only used during boot services. On successful completion of > EFI_BOOT_SERVICES.ExitBootServices() the watchdog timer is disabled. > > This implies this watchdog doesn't cover the the OS loading. > If you want to cover this phase of the boot, > an additional hardware watchdog is required (usually set in the BIOS). Ditto. Please update these two paragraphs taking into account my comments above. > On the command line, a single argument is enough to control the watchdog. > This argument defines the timeout of the watchdog (in seconds). > - If 0 > argument < 0xFFFF, the watchdog is armed with the associate timeout. > - If argument is set to 0, the watchdog is disarmed. > > By default GRUB disable the watchdog and so this patch. > Therefore, this commit have no impact on the default GRUB's behavior. > > This patch is used in production for month on a 50K server platform with success. I think you are missing a description why this functionality must be in the GRUB core.img. > Signed-off-by: Erwan Velu > Signed-off-by: Arthur Mesh Arthur Mesh SOB should be before yours. > --- > docs/grub.texi | 13 ++++++ > grub-core/kern/efi/init.c | 85 ++++++++++++++++++++++++++++++++++++- > include/grub/efi/efi.h | 2 + > include/grub/kernel.h | 3 +- > include/grub/util/install.h | 8 +++- > util/grub-install-common.c | 12 +++++- > util/grub-mkimage.c | 11 ++++- > util/mkimage.c | 23 +++++++++- > 8 files changed, 147 insertions(+), 10 deletions(-) > > diff --git a/docs/grub.texi b/docs/grub.texi > index f8b4b3b21a7f..f012e9537306 100644 > --- a/docs/grub.texi > +++ b/docs/grub.texi > @@ -3991,6 +3991,7 @@ you forget a command, you can run the command @command{help} > * distrust:: Remove a pubkey from trusted keys > * drivemap:: Map a drive to another > * echo:: Display a line of text > +* efi-watchdog:: Manipulate EFI watchdog > * eval:: Evaluate agruments as GRUB commands > * export:: Export an environment variable > * false:: Do nothing, unsuccessfully > @@ -4442,6 +4443,18 @@ When interpreting backslash escapes, backslash followed by any other > character will print that character. > @end deffn > > +@node efi-watchdog > +@subsection efi-watchdog s/efi-watchdog/watchdog/ and below please... > +@deffn Command efi-watchdog @var{timeout} > +Control the EFI system's watchdog timer. > +Only available in EFI targeted GRUB. I do not see any reason to put this in two lines. > + > +When the watchdog exceed @var{timeout}, the system is reset. > + > +If @var{timeout} is set to 0, the watchdog is disarmed. > + Please drop this empty line. > +@end deffn > > @node eval > @subsection eval > diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c > index 7facacf09c7b..532e8533426e 100644 > --- a/grub-core/kern/efi/init.c > +++ b/grub-core/kern/efi/init.c > @@ -28,6 +28,8 @@ > #include > #include > #include > +#include I think this include is not needed. > +#include > > #ifdef GRUB_STACK_PROTECTOR > > @@ -82,6 +84,82 @@ stack_protector_init (void) > > grub_addr_t grub_modbase; > > +static grub_command_t cmd_list; > + > + Please drop this empty line. > +static grub_err_t grub_efi_set_watchdog_timer(unsigned long timeout) Please use grub_efi_uintn_t instead of unsigned long. However, please remember that its size depends on architecture. So, it can be 32-bits or 64-bits... > +{ > + /* User defined code, set to BOOT (B007) for GRUB > + UEFI SPEC 2.6 reports : Please refer to the latest UEFI spec. > + The numeric code to log on a watchdog timer timeout event. > + The firmware reserves codes 0x0000 to 0xFFFF. > + Loaders and operating systems may use other timeout codes. This comment says you cannot use 0xb007. I would use 0xdeadb007deadb007 by default. However, I would give an option to change the default by user. > + */ Please format comments according to [2]. > + grub_efi_uint64_t code = 0xB007; > + grub_efi_status_t status = 0; > + > + if (timeout >= 0xFFFF) > + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("efi-watchdog: timeout must be lower than 65535")); Why do you artificially limit the timeout value? Please do not do that. > + if (timeout > 0) > + grub_printf("grub %s: Arming EFI watchdog at %lu seconds\n", PACKAGE_VERSION, timeout); If you want debug messages please use grub_dprintf() instead of grub_printf(). > + else { > + grub_printf("grub %s: Disarming EFI watchdog\n", PACKAGE_VERSION); > + code = 0; > + } > + > + status = efi_call_4 (grub_efi_system_table->boot_services->set_watchdog_timer, timeout, code, sizeof(L"GRUB"), L"GRUB"); > + > + if (status != GRUB_EFI_SUCCESS) > + return grub_error (GRUB_ERR_BUG, N_("Unexpected UEFI SetWatchdogTimer() error")); Please print status value in case of an error, e.g. "UEFI SetWatchdogTimer() error: %..." > + else > + return GRUB_ERR_NONE; > +} > + > +static grub_err_t > +grub_cmd_efi_watchdog (grub_command_t cmd __attribute__ ((unused)), > + int argc, char **args) > +{ > + unsigned long timeout = 0; s/unsigned long/grub_efi_uintn_t/ Please use types according to the UEFI specification and convert properly between them. > + > + Please drop this redundant empty line. > + if (argc < 1) > + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("usage: efi-watchdog ")); As I said earlier. I think "watchdog" command should take two arguments: -t and -c . > + const char *ptr = args[0]; > + timeout = grub_strtoul (ptr, &ptr, 0); Please read strtoul() documentation and handle all errors properly. > + if (grub_errno) if (grub_errno != GRUB_ERR_NONE) > + return grub_errno; > + > + return grub_efi_set_watchdog_timer(timeout); Please do not forget about spaces between function names and "(". > +} > + > +static void > +grub_efi_watchdog_timer_init(void) > +{ > + struct grub_module_header *header; > + unsigned long efi_watchdog_timeout = 0; Again, please use correct UEFI type... > + > + FOR_MODULES (header) Missing "{}" for FOR_MODULES() and... > + if (header->type == OBJ_TYPE_EFI_WATCHDOG_TIMEOUT) ... incorrect "if" indention. Please take a look at grub-core/kern/efi/sb.c for more details... > + { > + char *efi_wdt_orig_addr; > + static char *efi_wdt_addr; > + static grub_off_t efi_wdt_size = 0; > + > + efi_wdt_orig_addr = (char *) header + sizeof (struct grub_module_header); > + efi_wdt_size = header->size - sizeof (struct grub_module_header); > + efi_wdt_addr = grub_malloc (efi_wdt_size); Please do not ignore grub_malloc() errors. In general you need proper error handling for every function which may fail. > + grub_memmove (efi_wdt_addr, efi_wdt_orig_addr, efi_wdt_size); > + efi_watchdog_timeout = (unsigned long) *efi_wdt_addr; > + break; > + } > + > + grub_efi_set_watchdog_timer(efi_watchdog_timeout); > +} > + > + Please drop this redundant empty line... > void > grub_efi_init (void) > { > @@ -105,10 +183,12 @@ grub_efi_init (void) > grub_shim_lock_verifier_setup (); > } > > - efi_call_4 (grub_efi_system_table->boot_services->set_watchdog_timer, > - 0, 0, 0, NULL); > + grub_efi_watchdog_timer_init(); > > grub_efidisk_init (); > + > + cmd_list = grub_register_command ("efi-watchdog", grub_cmd_efi_watchdog, 0, > + N_("Enable/Disable system's watchdog timer.")); > } > > void (*grub_efi_net_config) (grub_efi_handle_t hnd, > @@ -146,4 +226,5 @@ grub_efi_fini (void) > { > grub_efidisk_fini (); > grub_console_fini (); > + grub_unregister_command (cmd_list); > } > diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h > index 83d958f9945e..372f995b74f4 100644 > --- a/include/grub/efi/efi.h > +++ b/include/grub/efi/efi.h > @@ -124,4 +124,6 @@ struct grub_net_card; > grub_efi_handle_t > grub_efinet_get_device_handle (struct grub_net_card *card); > > +/* EFI Watchdog armed by grub, in minutes */ > +#define EFI_WATCHDOG_MINUTES 0 > #endif /* ! GRUB_EFI_EFI_HEADER */ > diff --git a/include/grub/kernel.h b/include/grub/kernel.h > index abbca5ea3359..172599c58b23 100644 > --- a/include/grub/kernel.h > +++ b/include/grub/kernel.h > @@ -30,7 +30,8 @@ enum > OBJ_TYPE_PREFIX, > OBJ_TYPE_PUBKEY, > OBJ_TYPE_DTB, > - OBJ_TYPE_DISABLE_SHIM_LOCK > + OBJ_TYPE_DISABLE_SHIM_LOCK, > + OBJ_TYPE_EFI_WATCHDOG_TIMEOUT, > }; > > /* The module header. */ > diff --git a/include/grub/util/install.h b/include/grub/util/install.h > index 7df3191f47ef..1276742d09e6 100644 > --- a/include/grub/util/install.h > +++ b/include/grub/util/install.h > @@ -67,6 +67,8 @@ > N_("SBAT metadata"), 0 }, \ > { "disable-shim-lock", GRUB_INSTALL_OPTIONS_DISABLE_SHIM_LOCK, 0, 0, \ > N_("disable shim_lock verifier"), 0 }, \ > + { "efi-watchdog-timeout", GRUB_INSTALL_OPTIONS_EFI_WATCHDOG_TIMEOUT, N_("EFI_WATCHDOG_TIMEOUT"), 0, \ > + N_("arm efi watchdog at EFI_WATCHDOG_TIMEOUT seconds"), 0 }, \ > { "verbose", 'v', 0, 0, \ > N_("print verbose messages."), 1 } > > @@ -128,7 +130,8 @@ enum grub_install_options { > GRUB_INSTALL_OPTIONS_INSTALL_CORE_COMPRESS, > GRUB_INSTALL_OPTIONS_DTB, > GRUB_INSTALL_OPTIONS_SBAT, > - GRUB_INSTALL_OPTIONS_DISABLE_SHIM_LOCK > + GRUB_INSTALL_OPTIONS_DISABLE_SHIM_LOCK, > + GRUB_INSTALL_OPTIONS_EFI_WATCHDOG_TIMEOUT > }; > > extern char *grub_install_source_directory; > @@ -190,7 +193,8 @@ grub_install_generate_image (const char *dir, const char *prefix, > const struct grub_install_image_target_desc *image_target, > int note, > grub_compression_t comp, const char *dtb_file, > - const char *sbat_path, const int disable_shim_lock); > + const char *sbat_path, const int disable_shim_lock, > + unsigned long efi_watchdog_timeout); > > const struct grub_install_image_target_desc * > grub_install_get_image_target (const char *arg); > diff --git a/util/grub-install-common.c b/util/grub-install-common.c > index 4e212e690c52..dab94799a6c3 100644 > --- a/util/grub-install-common.c > +++ b/util/grub-install-common.c > @@ -460,11 +460,13 @@ static char **pubkeys; > static size_t npubkeys; > static char *sbat; > static int disable_shim_lock; > +static unsigned long efi_watchdog_timeout; > static grub_compression_t compression; > > int > grub_install_parse (int key, char *arg) > { > + const char *const_arg = arg; > switch (key) > { > case 'C': > @@ -562,6 +564,11 @@ grub_install_parse (int key, char *arg) > grub_util_error (_("Unrecognized compression `%s'"), arg); > case GRUB_INSTALL_OPTIONS_GRUB_MKIMAGE: > return 1; > + case GRUB_INSTALL_OPTIONS_EFI_WATCHDOG_TIMEOUT: > + efi_watchdog_timeout = grub_strtoul (arg, &const_arg, 0); Please do not ignore errors... > + if (grub_errno) > + grub_util_error (_("timeout argument must be an integer : %s"), arg); > + return 1; > default: > return 0; > } > @@ -661,9 +668,10 @@ grub_install_make_image_wrap_file (const char *dir, const char *prefix, > " --output '%s' " > " --dtb '%s' " > "--sbat '%s' " > + "--efi-watchdog-timeout '%lu' " > "--format '%s' --compression '%s' %s %s %s\n", > dir, prefix, > - outname, dtb ? : "", sbat ? : "", mkimage_target, > + outname, dtb ? : "", sbat ? : "", efi_watchdog_timeout, mkimage_target, > compnames[compression], note ? "--note" : "", > disable_shim_lock ? "--disable-shim-lock" : "", s); Could you be more consistent and add your argument behind the --disable-shim-lock one? > free (s); > @@ -676,7 +684,7 @@ grub_install_make_image_wrap_file (const char *dir, const char *prefix, > modules.entries, memdisk_path, > pubkeys, npubkeys, config_path, tgt, > note, compression, dtb, sbat, > - disable_shim_lock); > + disable_shim_lock, efi_watchdog_timeout); > while (dc--) > grub_install_pop_module (); > } > diff --git a/util/grub-mkimage.c b/util/grub-mkimage.c > index c0d559937020..fce7941a58fe 100644 > --- a/util/grub-mkimage.c > +++ b/util/grub-mkimage.c > @@ -83,6 +83,7 @@ static struct argp_option options[] = { > {"compression", 'C', "(xz|none|auto)", 0, N_("choose the compression to use for core image"), 0}, > {"sbat", 's', N_("FILE"), 0, N_("SBAT metadata"), 0}, > {"disable-shim-lock", GRUB_INSTALL_OPTIONS_DISABLE_SHIM_LOCK, 0, 0, N_("disable shim_lock verifier"), 0}, > + {"efi-watchdog-timeout", GRUB_INSTALL_OPTIONS_EFI_WATCHDOG_TIMEOUT, 0, 0, N_("efi watchdog timeout"), 0}, > {"verbose", 'v', 0, 0, N_("print verbose messages."), 0}, > { 0, 0, 0, 0, 0, 0 } > }; > @@ -128,6 +129,7 @@ struct arguments > char *sbat; > int note; > int disable_shim_lock; > + unsigned long efi_watchdog_timeout; > const struct grub_install_image_target_desc *image_target; > grub_compression_t comp; > }; > @@ -138,6 +140,7 @@ argp_parser (int key, char *arg, struct argp_state *state) > /* Get the input argument from argp_parse, which we > know is a pointer to our arguments structure. */ > struct arguments *arguments = state->input; > + const char *const_arg = arg; > > switch (key) > { > @@ -239,6 +242,12 @@ argp_parser (int key, char *arg, struct argp_state *state) > arguments->disable_shim_lock = 1; > break; > > + case GRUB_INSTALL_OPTIONS_EFI_WATCHDOG_TIMEOUT: > + arguments->efi_watchdog_timeout = grub_strtoul (arg, &const_arg, 0); > + if (grub_errno) > + grub_util_error (_("timeout argument must be an integer : %s"), arg); > + break; > + > case 'v': > verbosity++; > break; > @@ -325,7 +334,7 @@ main (int argc, char *argv[]) > arguments.npubkeys, arguments.config, > arguments.image_target, arguments.note, > arguments.comp, arguments.dtb, > - arguments.sbat, arguments.disable_shim_lock); > + arguments.sbat, arguments.disable_shim_lock, arguments.efi_watchdog_timeout); > > if (grub_util_file_sync (fp) < 0) > grub_util_error (_("cannot sync `%s': %s"), arguments.output ? : "stdout", > diff --git a/util/mkimage.c b/util/mkimage.c > index a26cf76f72f2..91b03801e628 100644 > --- a/util/mkimage.c > +++ b/util/mkimage.c > @@ -870,12 +870,12 @@ grub_install_generate_image (const char *dir, const char *prefix, > size_t npubkeys, char *config_path, > const struct grub_install_image_target_desc *image_target, > int note, grub_compression_t comp, const char *dtb_path, > - const char *sbat_path, int disable_shim_lock) > + const char *sbat_path, int disable_shim_lock, unsigned long efi_watchdog_timeout) > { > char *kernel_img, *core_img; > size_t total_module_size, core_size; > size_t memdisk_size = 0, config_size = 0; > - size_t prefix_size = 0, dtb_size = 0, sbat_size = 0; > + size_t prefix_size = 0, dtb_size = 0, sbat_size = 0, efi_watchdog_timeout_size = 0; > char *kernel_path; > size_t offset; > struct grub_util_path_list *path_list, *p; > @@ -929,6 +929,12 @@ grub_install_generate_image (const char *dir, const char *prefix, > if (sbat_path != NULL && image_target->id != IMAGE_EFI) > grub_util_error (_(".sbat section can be embedded into EFI images only")); > > + if (efi_watchdog_timeout) > + { > + efi_watchdog_timeout_size = ALIGN_ADDR(sizeof (efi_watchdog_timeout)); > + total_module_size += efi_watchdog_timeout_size + sizeof (struct grub_module_header); > + } > + > if (disable_shim_lock) > total_module_size += sizeof (struct grub_module_header); > > @@ -1078,6 +1084,19 @@ grub_install_generate_image (const char *dir, const char *prefix, > offset += sizeof (*header); > } > > + if (efi_watchdog_timeout) > + { > + struct grub_module_header *header; > + > + header = (struct grub_module_header *) (kernel_img + offset); > + header->type = grub_host_to_target32 (OBJ_TYPE_EFI_WATCHDOG_TIMEOUT); > + header->size = grub_host_to_target32 (efi_watchdog_timeout_size + sizeof (*header)); After looking at this it seems to me the timeout should be u32 thing internally in the GRUB. Daniel [1] https://lists.gnu.org/archive/html/grub-devel/2021-06/msg00017.html [2] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments