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

[Dimitri John Ledkov]

On Mon, May 3, 2021 at 6:09 AM Michael Chang via Grub-devel
<grub-devel@gnu.org> wrote:
>
> 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.
>

That is correct. At the moment point of no return for bios updates is
set after trying to install core.img into
mbr/blocklist/grub-pc-gpt-partition. There is no backup of
mbr/blocklists/grub-pc-gpt-partitions performed, nor is restoration
attempted.

At the very least this patch prevents machines to have i386-pc
core.img & modules missmatched when the wrong drive is specified, the
gap is too small, drive does not exist, drive is read-only etc. I.e.
when no update of mbr/blocklist/grub-pc-gpt-partition has actually
occurred and it bailed at the pre-update checks stage.

It would be neat to attempt backup of those things, and restore them
upon partial update. However, I fear if after all the pre-update
validations one fails to update mbr/blocklist/grub-pc-gpt-partition it
probably is hardware / drive failure and the backup we read or the
backup we try to restore will also fail.


> 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
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



-- 
Regards,

Dimitri.