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

[Daniel Kiper]

On Wed, May 12, 2021 at 08:24:44PM +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 patch only handles backup and restore of files copied to
> /boot/grub. This patch does not perform backup (or restoration) of MBR
> itself or blocklists. Thus when installing i386-pc platform,
> corruption may still occur with MBR and blocklists which will not be
> attempted to be automatically recovered.
>
> 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>
> ---
>  configure.ac                |   2 +-
>  include/grub/util/install.h |  13 ++++
>  util/grub-install-common.c  | 140 ++++++++++++++++++++++++++++++++----
>  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..d6e9ce4460 100644
> --- a/include/grub/util/install.h
> +++ b/include/grub/util/install.h
> @@ -275,4 +275,17 @@ 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;

Hmmm... The size_t does not look like a good fit for this. I would define
grub_install_backup_ponr as an int.

>  #endif
> diff --git a/util/grub-install-common.c b/util/grub-install-common.c
> index b4f28840f1..84c1db8674 100644
> --- a/util/grub-install-common.c
> +++ b/util/grub-install-common.c
> @@ -185,38 +185,148 @@ 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_NEW,
> +  CLEAN_BACKUP,
> +  CREATE_BACKUP,
> +  RESTORE_BACKUP
> +};
> +
> +static size_t backup_dirs_size = 0;
> +static char **backup_dirs = NULL;
> +static pid_t backup_process = 0;
> +size_t grub_install_backup_ponr = 0;

Ditto...

[...]

> +static void
> +restore_backup_atexit (void)
> +{
> +  /*
> +   * some child inherited atexit handler, did not clear it, and called
> +   * it; thus skip clean or restore logic.
> +   */
> +  if (backup_process != getpid ())
> +    return;
> +
> +  for (size_t i = 0; i < backup_dirs_size; i++)

I would define "i" at the beginning of this function.

> +    {
> +      /* if past point of no return simply clean the
> +         backups.  Otherwise cleanup newly installed files,
> +         and restore the backups */

Please fix the formatting of this comment...

[...]

> 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

[...]

> @@ -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;

I would add a comment why this is needed. I think something similar to
what you sent in the other email is fine.

Daniel