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

[Daniel Kiper]

Hey,

In general much better but...

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

I think this last sentence does not parse with the rest of paragraph.

Additionally, may I ask you to add a blurb here about the blocklist?
I think about most important things from the email exchange between
Micheal and you.

> Also add modinfo.sh and *.efi 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 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) */

Could you format comments like it is documented here:
  https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments

> +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 what? s/CLEAN/CLEAN_NEW/?

> +  CLEAN_BACKUP,
> +  CREATE_BACKUP,
> +  RESTORE_BACKUP,

Please drop this redundant "," at the end.

> +};
> +
> +static size_t backup_dirs_len = 0;

s/backup_dirs_len/backup_dirs_nr/?

or

s/backup_dirs_len/backup_dirs_size/?

> +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] = "";

const char *suffix = "";

> +  if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP))
> +    strcpy (suffix, "~");

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;

s/0/'\0'/

The final result is the same but it is a bit clearer what you mean...

> +           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 */

Please fix this comment as I mentioned above...

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

I would drop "== 1" here.

> +     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 = xrealloc (backup_dirs, sizeof (char *) * (backup_dirs_len + 1));

You do not need "if (backup_dirs_len == 0)" because xrealloc() is able
to serve you properly in both cases you care about. And even more... :-)

> +  backup_dirs[backup_dirs_len] = strdup (dir);
> +  backup_dirs_len++;
> +  if (backup_process == 0)

if (!backup_process)

> +    {
> +      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;
> +

Do we really need this one?

>    fprintf (stderr, "%s\n", _("Installation finished. No error reported."));
>
>    /* Free resources.  */

Daniel