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

[Daniel Kiper]

Ugh... I have just realized that this will not build properly if
HAVE_ATEXIT is not defined. Please look below...

On Mon, May 24, 2021 at 10:59:34AM +0100, Dimitri John Ledkov wrote:
> From: Dimitri John Ledkov <xnox@ubuntu.com>
>
> 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>
> ---
>
> changes since v4:
>  + change ponr from size_t to int (there is no bool in grub yet)
>  + declare loop variable at the start of the function
>  + format one more comment correctly
>  + add comment about significance to set ponr
>
>  configure.ac                |   2 +-
>  include/grub/util/install.h |  13 ++++
>  util/grub-install-common.c  | 143 ++++++++++++++++++++++++++++++++----
>  util/grub-install.c         |  40 ++++++++--
>  util/grub-mknetdir.c        |   3 +
>  util/grub-mkrescue.c        |   3 +
>  util/grub-mkstandalone.c    |   2 +
>  7 files changed, 182 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..b2a0175374 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 int grub_install_backup_ponr;

I would use a function which sets this variable and then

#ifdef HAVE_ATEXIT
extern void
grub_set_install_backup_ponr (void);
#else
static inline void
grub_set_install_backup_ponr (void)
{
}
#endif

> +
>  #endif
> diff --git a/util/grub-install-common.c b/util/grub-install-common.c
> index b4f28840f1..d493e4a119 100644
> --- a/util/grub-install-common.c
> +++ b/util/grub-install-common.c
> @@ -185,38 +185,151 @@ 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
> +};
> +

#ifdef HAVE_ATEXIT

> +static size_t backup_dirs_size = 0;
> +static char **backup_dirs = NULL;
> +static pid_t backup_process = 0;
> +int grub_install_backup_ponr = 0;

#endif

[...]

#ifdef HAVE_ATEXIT

void
grub_set_install_backup_ponr (void)
{
  grub_install_backup_ponr = 1;
}

> +static void
> +restore_backup_atexit (void)
> +{
> +  size_t i;
> +  /*
> +   * some child inherited atexit handler, did not clear it, and called
> +   * it; thus skip clean or restore logic.
> +   */
> +  if (backup_process != getpid ())
> +    return;
> +
> +  for (i = 0; i < backup_dirs_size; 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)
> +     clean_grub_dir_real (backup_dirs[i], CLEAN_BACKUP);
> +      else
> +     {
> +       clean_grub_dir_real (backup_dirs[i], CLEAN_NEW);
> +       clean_grub_dir_real (backup_dirs[i], RESTORE_BACKUP);
> +     }
> +      free (backup_dirs[i]);
> +    }
> +
> +  backup_dirs_size = 0;
> +
> +  free (backup_dirs);
> +}
> +
> +static void
> +append_to_backup_dirs (const char *dir)
> +{
> +  backup_dirs = xrealloc (backup_dirs, sizeof (char *) * (backup_dirs_size + 
> 1));
> +  backup_dirs[backup_dirs_size] = strdup (dir);
> +  backup_dirs_size++;
> +  if (!backup_process)
> +    {
> +      atexit (restore_backup_atexit);
> +      backup_process = getpid ();
> +    }
> +}

#else

static void
append_to_backup_dirs (const char *dir __attribute__ ((unused)))
{
}

#endif

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

Please drop this #if...

> +  append_to_backup_dirs (di);
> +#endif
> +}

I think we need at least these changes. Otherwise if HAVE_ATEXIT is not
defined we will have unused code which will compiler complain about...
Please try to build the GRUB with and without HAVE_ATEXIT defined after
fixing this issue.

Daniel