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