Re: [PATCH] grub-install: Add backup and restore

[Michael Chang]

On Thu, Apr 29, 2021 at 12:36:37PM +0100, Dimitri John Ledkov wrote:
> Refactor clean_grub_dir to create a backup of all the files, instead
> of just irrevocably removing them as the first action. If available,
> register atexit handle to restore the backup if errors occur before
> point of no return, or remove the backup if everything was
> successful. If atexit is not available, the backup remains on disk for
> manual recovery.
> 
> Some platforms defined a point of no return, i.e. after modules & core
> images were updated. Failures from any commands after that stage are
> ignored, and backup is cleanedup. For example, on EFI platforms update
> is not reverted when efibootmgr fails.

Thank you for implementing this. I think it has addressed my question in
other discussion that backup/restore may be inadvertently triggered in
some cases which is not desirable, for eg when efibootmgr errors out as
you also depicted.

> 
> Extra care is taken to ensure atexit handler is only invoked by the
> parent process and not any children forks. Some older grub codebases
> can invoke parent atexit hooks from forks, which can mess up the
> backup.
> 
> This allows safer upgrades of MBR & modules, such that
> modules/images/fonts/translations are consistent with MBR in case of
> errors. For example accidental grub-install /dev/non-existent-disk
> currently clobbers and upgrades modules in /boot/grub, despite not
> actually updating any MBR. This increases peak disk-usage slightly, by
> requiring temporarily twice the disk space to complete grub-install.
> 
> Also add modinfo.sh and *.efi to the cleanup/backup/restore codepath,
> to ensure it is also cleaned / backed up / restored.

There's a corner case that the installation can be done via blocklist,
which means restore files doesn't help at all to recover error, given
the blocklist recorded in the images no longer apply to the lately
restored core.img.

I know the blocklist is neither recommended nor enabled by default so it
is good for me to neglect that. Just to complete the message here in
case anyone else would hold an opinion otherwise. 

Thanks,
Michael

> 
> Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
> ---
> 
>  Changes since v2:
>  - switch from on_exit, to atexit
>  - introduce point of no return flag, as atexit doesn't know about
>    exit status and at times it is desired to declare point of no
>    return earlier and ignore some error.
>  - address review feedback wrt styling
>  - improve reliablity, verify that only parent process calls atexit
>    hook
> 
>  configure.ac                |   2 +-
>  include/grub/util/install.h |  11 +++
>  util/grub-install-common.c  | 142 ++++++++++++++++++++++++++++++++----
>  util/grub-install.c         |  33 +++++++--
>  util/grub-mknetdir.c        |   3 +
>  util/grub-mkrescue.c        |   3 +
>  util/grub-mkstandalone.c    |   2 +
>  7 files changed, 172 insertions(+), 24 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 74719416c4..a5e94f360e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -419,7 +419,7 @@ else
>  fi
>  
>  # Check for functions and headers.
> -AC_CHECK_FUNCS(posix_memalign memalign getextmntent)
> +AC_CHECK_FUNCS(posix_memalign memalign getextmntent atexit)
>  AC_CHECK_HEADERS(sys/param.h sys/mount.h sys/mnttab.h limits.h)
>  
>  # glibc 2.25 still includes sys/sysmacros.h in sys/types.h but emits 
> deprecation
> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> index b93c73caac..d729060ce7 100644
> --- a/include/grub/util/install.h
> +++ b/include/grub/util/install.h
> @@ -275,4 +275,15 @@ extern char *grub_install_copy_buffer;
>  int
>  grub_install_is_short_mbrgap_supported (void);
>  
> +/* grub-install-common tries to make backups of modules & auxilary
> +files, and restore the backup upon failure to install core.img. There
> +are platforms with additional actions after modules & core got
> +installed in place. It is a point of no return, as core.img cannot be
> +reverted from this point onwards, and new modules should be kept
> +installed. Before performing these additional actions raise
> +grub_install_backup_ponr flag, this way failure to perform additional
> +actions will not result in reverting new modules to the old
> +ones. (i.e. in case efivars updates fails) */
> +extern size_t grub_install_backup_ponr;
> +
>  #endif
> diff --git a/util/grub-install-common.c b/util/grub-install-common.c
> index b4f28840f1..db7feae7d3 100644
> --- a/util/grub-install-common.c
> +++ b/util/grub-install-common.c
> @@ -185,38 +185,150 @@ grub_install_mkdir_p (const char *dst)
>    free (t);
>  }
>  
> +static int
> +strcmp_ext (const char *a, const char *b, const char *ext)
> +{
> +  char *bsuffix = grub_util_path_concat_ext (1, b, ext);
> +  int r = strcmp (a, bsuffix);
> +
> +  free (bsuffix);
> +  return r;
> +}
> +
> +enum clean_grub_dir_mode
> +{
> +  CLEAN,
> +  CLEAN_BACKUP,
> +  CREATE_BACKUP,
> +  RESTORE_BACKUP,
> +};
> +
> +static size_t backup_dirs_len = 0;
> +static char **backup_dirs;
> +static pid_t backup_process = 0;
> +size_t grub_install_backup_ponr = 0;
> +
>  static void
> -clean_grub_dir (const char *di)
> +clean_grub_dir_real (const char *di, enum clean_grub_dir_mode mode)
>  {
>    grub_util_fd_dir_t d;
>    grub_util_fd_dirent_t de;
> +  char suffix[2] = "";
> +
> +  if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP))
> +    strcpy (suffix, "~");
>  
>    d = grub_util_fd_opendir (di);
>    if (!d)
> -    grub_util_error (_("cannot open directory `%s': %s"),
> -                  di, grub_util_fd_strerror ());
> +    {
> +      if (mode == CLEAN_BACKUP)
> +     return;
> +      grub_util_error (_("cannot open directory `%s': %s"),
> +                    di, grub_util_fd_strerror ());
> +    }
>  
>    while ((de = grub_util_fd_readdir (d)))
>      {
>        const char *ext = strrchr (de->d_name, '.');
> -      if ((ext && (strcmp (ext, ".mod") == 0
> -                || strcmp (ext, ".lst") == 0
> -                || strcmp (ext, ".img") == 0
> -                || strcmp (ext, ".mo") == 0)
> -        && strcmp (de->d_name, "menu.lst") != 0)
> -       || strcmp (de->d_name, "efiemu32.o") == 0
> -       || strcmp (de->d_name, "efiemu64.o") == 0)
> +
> +      if ((ext && (strcmp_ext (ext, ".mod", suffix) == 0
> +                || strcmp_ext (ext, ".lst", suffix) == 0
> +                || strcmp_ext (ext, ".img", suffix) == 0
> +                || strcmp_ext (ext, ".efi", suffix) == 0
> +                || strcmp_ext (ext, ".mo", suffix) == 0)
> +        && strcmp_ext (de->d_name, "menu.lst", suffix) != 0)
> +       || strcmp_ext (de->d_name, "modinfo.sh", suffix) == 0
> +       || strcmp_ext (de->d_name, "efiemu32.o", suffix) == 0
> +       || strcmp_ext (de->d_name, "efiemu64.o", suffix) == 0)
>       {
> -       char *x = grub_util_path_concat (2, di, de->d_name);
> -       if (grub_util_unlink (x) < 0)
> -         grub_util_error (_("cannot delete `%s': %s"), x,
> -                          grub_util_fd_strerror ());
> -       free (x);
> +       char *srcf = grub_util_path_concat (2, di, de->d_name);
> +
> +       if (mode == CREATE_BACKUP)
> +         {
> +           char *dstf = grub_util_path_concat_ext (2, di, de->d_name, "~");
> +
> +           if (grub_util_rename (srcf, dstf) < 0)
> +             grub_util_error (_("cannot backup `%s': %s"), srcf,
> +                              grub_util_fd_strerror ());
> +           free (dstf);
> +         }
> +       else if (mode == RESTORE_BACKUP)
> +         {
> +           char *dstf = grub_util_path_concat (2, di, de->d_name);
> +
> +           dstf[strlen (dstf) - 1] = 0;
> +           if (grub_util_rename (srcf, dstf) < 0)
> +             grub_util_error (_("cannot restore `%s': %s"), dstf,
> +                              grub_util_fd_strerror ());
> +           free (dstf);
> +         }
> +       else
> +         {
> +           if (grub_util_unlink (srcf) < 0)
> +             grub_util_error (_("cannot delete `%s': %s"), srcf,
> +                              grub_util_fd_strerror ());
> +         }
> +       free (srcf);
>       }
>      }
>    grub_util_fd_closedir (d);
>  }
>  
> +static void
> +restore_backup_atexit (void)
> +{
> +  /* some child inherited atexit handler, did not clear it, and
> +   * called it, skip clean or restore logic */
> +  if (backup_process != getpid ())
> +    return;
> +
> +  for (size_t i = 0; i < backup_dirs_len; i++)
> +    {
> +      /* if past point of no return simply clean the
> +         backups.  Otherwise cleanup newly installed files,
> +         and restore the backups */
> +      if (grub_install_backup_ponr == 1)
> +     clean_grub_dir_real (backup_dirs[i], CLEAN_BACKUP);
> +      else
> +     {
> +       clean_grub_dir_real (backup_dirs[i], CLEAN);
> +       clean_grub_dir_real (backup_dirs[i], RESTORE_BACKUP);
> +     }
> +      free (backup_dirs[i]);
> +    }
> +
> +  backup_dirs_len = 0;
> +
> +  free (backup_dirs);
> +}
> +
> +static void
> +append_to_backup_dirs (const char *dir)
> +{
> +  if (backup_dirs_len == 0)
> +    backup_dirs = xmalloc (sizeof (char *) * (backup_dirs_len + 1));
> +  else
> +    backup_dirs =
> +      xrealloc (backup_dirs, sizeof (char *) * (backup_dirs_len + 1));
> +  backup_dirs[backup_dirs_len] = strdup (dir);
> +  backup_dirs_len++;
> +  if (backup_process == 0)
> +    {
> +      atexit (restore_backup_atexit);
> +      backup_process = getpid ();
> +    }
> +}
> +
> +static void
> +clean_grub_dir (const char *di)
> +{
> +  clean_grub_dir_real (di, CLEAN_BACKUP);
> +  clean_grub_dir_real (di, CREATE_BACKUP);
> +#if defined(HAVE_ATEXIT)
> +  append_to_backup_dirs (di);
> +#endif
> +}
> +
>  struct install_list
>  {
>    int is_default;
> diff --git a/util/grub-install.c b/util/grub-install.c
> index a0babe3eff..f85cf473ff 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -1719,10 +1719,14 @@ main (int argc, char *argv[])
>                       
>       /*  Now perform the installation.  */
>       if (install_bootsector)
> -       grub_util_bios_setup (platdir, "boot.img", "core.img",
> -                             install_drive, force,
> -                             fs_probe, allow_floppy, add_rs_codes,
> -                             !grub_install_is_short_mbrgap_supported ());
> +       {
> +         grub_util_bios_setup (platdir, "boot.img", "core.img",
> +                               install_drive, force,
> +                               fs_probe, allow_floppy, add_rs_codes,
> +                               !grub_install_is_short_mbrgap_supported ());
> +
> +         grub_install_backup_ponr = 1;
> +       }
>       break;
>        }
>      case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275:
> @@ -1746,10 +1750,14 @@ main (int argc, char *argv[])
>                       
>       /*  Now perform the installation.  */
>       if (install_bootsector)
> -       grub_util_sparc_setup (platdir, "boot.img", "core.img",
> -                              install_drive, force,
> -                              fs_probe, allow_floppy,
> -                              0 /* unused */, 0 /* unused */ );
> +       {
> +         grub_util_sparc_setup (platdir, "boot.img", "core.img",
> +                                install_drive, force,
> +                                fs_probe, allow_floppy,
> +                                0 /* unused */, 0 /* unused */ );
> +
> +         grub_install_backup_ponr = 1;
> +       }
>       break;
>        }
>  
> @@ -1776,6 +1784,8 @@ main (int argc, char *argv[])
>         grub_elf = grub_util_path_concat (2, core_services, "grub.elf");
>         grub_install_copy_file (imgfile, grub_elf, 1);
>  
> +       grub_install_backup_ponr = 1;
> +
>         f = grub_util_fopen (mach_kernel, "a+");
>         if (!f)
>           grub_util_error (_("Can't create file: %s"), strerror (errno));
> @@ -1878,6 +1888,8 @@ main (int argc, char *argv[])
>         boot_efi = grub_util_path_concat (2, core_services, "boot.efi");
>         grub_install_copy_file (imgfile, boot_efi, 1);
>  
> +       grub_install_backup_ponr = 1;
> +
>         f = grub_util_fopen (mach_kernel, "r+");
>         if (!f)
>           grub_util_error (_("Can't create file: %s"), strerror (errno));
> @@ -1916,6 +1928,9 @@ main (int argc, char *argv[])
>        {
>       char *dst = grub_util_path_concat (2, efidir, efi_file);
>       grub_install_copy_file (imgfile, dst, 1);
> +
> +     grub_install_backup_ponr = 1;
> +
>       free (dst);
>        }
>        if (!removable && update_nvram)
> @@ -1967,6 +1982,8 @@ main (int argc, char *argv[])
>        break;
>      }
>  
> +  grub_install_backup_ponr = 1;
> +
>    fprintf (stderr, "%s\n", _("Installation finished. No error reported."));
>  
>    /* Free resources.  */
> diff --git a/util/grub-mknetdir.c b/util/grub-mknetdir.c
> index 602574d52e..c9ea345b37 100644
> --- a/util/grub-mknetdir.c
> +++ b/util/grub-mknetdir.c
> @@ -159,6 +159,9 @@ process_input_dir (const char *input_dir, enum 
> grub_install_plat platform)
>    grub_install_make_image_wrap (input_dir, prefix, output,
>                               0, load_cfg,
>                               targets[platform].mkimage_target, 0);
> +
> +  grub_install_backup_ponr = 1;
> +
>    grub_install_pop_module ();
>  
>    /* TRANSLATORS: First %s is replaced by platform name. Second one by 
> filename.  */
> diff --git a/util/grub-mkrescue.c b/util/grub-mkrescue.c
> index 51831027f7..cc87fde38e 100644
> --- a/util/grub-mkrescue.c
> +++ b/util/grub-mkrescue.c
> @@ -530,6 +530,9 @@ main (int argc, char *argv[])
>                              boot_grub, plat);
>        source_dirs[plat] = xstrdup (grub_install_source_directory);
>      }
> +
> +  grub_install_backup_ponr = 1;
> +
>    if (system_area == SYS_AREA_AUTO || grub_install_source_directory)
>      {
>        if (source_dirs[GRUB_INSTALL_PLATFORM_I386_PC]
> diff --git a/util/grub-mkstandalone.c b/util/grub-mkstandalone.c
> index edf309717c..3d23ee71e5 100644
> --- a/util/grub-mkstandalone.c
> +++ b/util/grub-mkstandalone.c
> @@ -318,6 +318,8 @@ main (int argc, char *argv[])
>    grub_install_copy_files (grub_install_source_directory,
>                          boot_grub, plat);
>  
> +  grub_install_backup_ponr = 1;
> +
>    char *memdisk_img = grub_util_make_temporary_file ();
>  
>    memdisk = grub_util_fopen (memdisk_img, "wb");
> -- 
> 2.27.0
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel