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

[Dimitri John Ledkov <xnox@ubuntu.com>]

[-- Attachment #1: Type: text/plain, Size: 9324 bytes --]

On Tue, 8 Dec 2020, 03:17 Michael Chang, <mchang@suse.com> wrote:

> On Mon, Dec 07, 2020 at 12:37:28PM +0000, 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 on_exit handle to restore the backup if any errors occur, or
> > remove the backup if everything was successful. If on_exit is not
> > available, the backup remains on disk for manual recovery.
> >
> > 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.
>
> I'd love to have the described problem fixed too, as it helps a lot in
> the update path to survive grub-install error which can be contributed
> by many different reasons, even though grub has to stay with old version
> but still much better than unbootable system.
>
> But here I have a concern, as to what will happen if the error comes
> after image installation ? For example, the efibootmgr may fail to
> register boot entry after copying the images to efi partition. It looks
> to me that the restoring backup would be triggerd incorrectly for the
> new image to load restored old backups.
>
> Would you please help to clarify that ?
>


On Ubuntu we install secureboot signed prebuilt EFI images only which
prohibit module loading. Hence usually it is irrelevant if the EFI grub
modules are old or new.

And we do not call efibootmgr at all, as it does excessive writes to
efivars. Instead we don't even try to update efivars if there is no need.
It was submitted to this mailing list but I am not sure on the acceptance
status https://lists.gnu.org/archive/html/grub-devel/2019-03/msg00123.html

Above two things make EFI updates less fatal, as it is harder for a
prebuilt signed grub, which does not use modules, to fail booting. Unlike
i386-pc case.

And to answer your immediate question - the new EFI image will be used for
boot without rolling back.

Also, I am not sure how everyone installs signed grubs. Thus adding support
for ESP backup and restore might be futile upstream. As far as I can tell
it is safe to call grub-install on Ubuntu, whereas it might not be on other
distros. As Ubuntu preserves / reinstalls signed grub EFI images, instead
of generating a new random one and putting it on ESP thus causing failure
to boot with secureboot due to effectively replacing signed grub with an
unsigned one. And for resilient boot we have support for maintaining and
synchronising multiple ESP for Linux raid.

I kind of wish we'd prebuilt more core images at package build time and
simply copy them around. Not just the ones that have signing. Instead of
invoking mkimage on every installed system.

Nonetheless, it should be possible to hook up more files and directories to
perform backup, cleanup and restore on. Is there is a desire. The function
calls simply need to be added in the appropriate places.


> >
> > Also add modinfo.sh to the cleanup/backup/restore codepath, to ensure
> > it is also cleaned / backed up / restored.
> >
> > Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
> > ---
> >
> >  Changes since v1: as reported on linux-ext4 mailing list and debugged
> >  by Colin Watson, the grub_util_path_concat_ext call was incorrect in
> >  the restore backup case as there was no extention needed. Instead the
> >  call is corrected to just grub_util_path_concat.
> >
> >  This patch is now shipped in Ubuntu & Debian in multiple series. It
> >  would be nice to have this merged upstream, as it greatly improves
> >  grub upgrades and prevents missmatches of MBR & modules.
> >
> >  configure.ac               |   2 +-
> >  util/grub-install-common.c | 105 +++++++++++++++++++++++++++++++------
> >  2 files changed, 91 insertions(+), 16 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 7c10a4db7..71cd414c3 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 on_exit)
> >  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/util/grub-install-common.c b/util/grub-install-common.c
> > index 277eaf4e2..74584b92b 100644
> > --- a/util/grub-install-common.c
> > +++ b/util/grub-install-common.c
> > @@ -185,38 +185,113 @@ 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 = 0,
> > +  CLEAN_BACKUP = 1,
> > +  CREATE_BACKUP = 2,
> > +  RESTORE_BACKUP = 3,
> > +};
> > +
> >  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, ".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_on_exit (int status, void *arg)
> > +{
> > +  if (status == 0)
> > +    {
> > +      clean_grub_dir_real ((char *) arg, CLEAN_BACKUP);
> > +    }
> > +  else
> > +    {
> > +      clean_grub_dir_real ((char *) arg, CLEAN);
> > +      clean_grub_dir_real ((char *) arg, RESTORE_BACKUP);
> > +    }
> > +  free (arg);
> > +  arg = NULL;
> > +}
> > +
> > +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_ON_EXIT)
> > +  on_exit (restore_backup_on_exit, strdup (di));
> > +#endif
> > +}
> > +
> >  struct install_list
> >  {
> >    int is_default;
> > --
> > 2.27.0
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
>
>

[-- Attachment #2: Type: text/html, Size: 12847 bytes --]