On Mon, May 3, 2021 at 6:09 AM Michael Chang via Grub-devel
<grub-devel@gnu.org> wrote:
>
> 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.
>
> Thank you for implementing this. I think it has addressed my question in
> other discussion that backup/restore may be inadvertently triggered in
> some cases which is not desirable, for eg when efibootmgr errors out as
> you also depicted.
>
> >
> > 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.
> >
> > Also add modinfo.sh and *.efi to the cleanup/backup/restore codepath,
> > to ensure it is also cleaned / backed up / restored.
>
> There's a corner case that the installation can be done via blocklist,
> which means restore files doesn't help at all to recover error, given
> the blocklist recorded in the images no longer apply to the lately
> restored core.img.
>
> I know the blocklist is neither recommended nor enabled by default so it
> is good for me to neglect that. Just to complete the message here in
> case anyone else would hold an opinion otherwise.
>
That is correct. At the moment point of no return for bios updates is
set after trying to install core.img into
mbr/blocklist/grub-pc-gpt-partition. There is no backup of
mbr/blocklists/grub-pc-gpt-partitions performed, nor is restoration
attempted.
At the very least this patch prevents machines to have i386-pc
core.img & modules missmatched when the wrong drive is specified, the
gap is too small, drive does not exist, drive is read-only etc. I.e.
when no update of mbr/blocklist/grub-pc-gpt-partition has actually
occurred and it bailed at the pre-update checks stage.
It would be neat to attempt backup of those things, and restore them
upon partial update. However, I fear if after all the pre-update
validations one fails to update mbr/blocklist/grub-pc-gpt-partition it
probably is hardware / drive failure and the backup we read or the
backup we try to restore will also fail.
> Thanks,
> Michael
>
> >
> > Signed-off-by: Dimitri John Ledkov <x...@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) */
> > +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_BACKUP,
> > + CREATE_BACKUP,
> > + RESTORE_BACKUP,
> > +};
> > +
> > +static size_t backup_dirs_len = 0;
> > +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] = "";
> > +
> > + 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, ".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;
> > + 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 */
> > + 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)
> > + 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[backup_dirs_len] = strdup (dir);
> > + backup_dirs_len++;
> > + if (backup_process == 0)
> > + {
> > + 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;
> > +
> > fprintf (stderr, "%s\n", _("Installation finished. No error reported."));
> >
> > /* Free resources. */
> > diff --git a/util/grub-mknetdir.c b/util/grub-mknetdir.c
> > index 602574d52e..c9ea345b37 100644
> > --- a/util/grub-mknetdir.c
> > +++ b/util/grub-mknetdir.c
> > @@ -159,6 +159,9 @@ process_input_dir (const char *input_dir, enum
> > grub_install_plat platform)
> > grub_install_make_image_wrap (input_dir, prefix, output,
> > 0, load_cfg,
> > targets[platform].mkimage_target, 0);
> > +
> > + grub_install_backup_ponr = 1;
> > +
> > grub_install_pop_module ();
> >
> > /* TRANSLATORS: First %s is replaced by platform name. Second one by
> > filename. */
> > diff --git a/util/grub-mkrescue.c b/util/grub-mkrescue.c
> > index 51831027f7..cc87fde38e 100644
> > --- a/util/grub-mkrescue.c
> > +++ b/util/grub-mkrescue.c
> > @@ -530,6 +530,9 @@ main (int argc, char *argv[])
> > boot_grub, plat);
> > source_dirs[plat] = xstrdup (grub_install_source_directory);
> > }
> > +
> > + grub_install_backup_ponr = 1;
> > +
> > if (system_area == SYS_AREA_AUTO || grub_install_source_directory)
> > {
> > if (source_dirs[GRUB_INSTALL_PLATFORM_I386_PC]
> > diff --git a/util/grub-mkstandalone.c b/util/grub-mkstandalone.c
> > index edf309717c..3d23ee71e5 100644
> > --- a/util/grub-mkstandalone.c
> > +++ b/util/grub-mkstandalone.c
> > @@ -318,6 +318,8 @@ main (int argc, char *argv[])
> > grub_install_copy_files (grub_install_source_directory,
> > boot_grub, plat);
> >
> > + grub_install_backup_ponr = 1;
> > +
> > char *memdisk_img = grub_util_make_temporary_file ();
> >
> > memdisk = grub_util_fopen (memdisk_img, "wb");
> > --
> > 2.27.0
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
--
Regards,
Dimitri.
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
