On Tue, Dec 08, 2020 at 05:58:40AM +0000, Dimitri John Ledkov wrote:
> On Tue, 8 Dec 2020, 03:17 Michael Chang, <mch...@suse.com> wrote:
>
> > On Mon, Dec 07, 2020 at 12:37:28PM +0000, 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 on_exit handle to restore the backup if any errors occur, or
> > > remove the backup if everything was successful. If on_exit is not
> > > available, the backup remains on disk for manual recovery.
> > >
> > > 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'd love to have the described problem fixed too, as it helps a lot in
> > the update path to survive grub-install error which can be contributed
> > by many different reasons, even though grub has to stay with old version
> > but still much better than unbootable system.
> >
> > But here I have a concern, as to what will happen if the error comes
> > after image installation ? For example, the efibootmgr may fail to
> > register boot entry after copying the images to efi partition. It looks
> > to me that the restoring backup would be triggerd incorrectly for the
> > new image to load restored old backups.
> >
> > Would you please help to clarify that ?
> >
>
>
> On Ubuntu we install secureboot signed prebuilt EFI images only which
> prohibit module loading. Hence usually it is irrelevant if the EFI grub
> modules are old or new.
>
> And we do not call efibootmgr at all, as it does excessive writes to
> efivars. Instead we don't even try to update efivars if there is no need.
> It was submitted to this mailing list but I am not sure on the acceptance
> status https://lists.gnu.org/archive/html/grub-devel/2019-03/msg00123.html
>
> Above two things make EFI updates less fatal, as it is harder for a
> prebuilt signed grub, which does not use modules, to fail booting. Unlike
> i386-pc case.
My concern is exactly that these are not upstreamed yet so accepting the
patch would be premature. Either it has to be adapted to satisfy the
expected behavior of existing codebase or wait until those dependent
patches gets merged.
Besides, other architecture like openfirmware would update nvram in a
similar fashion to uefi but cannot be covered by the aforementioned
efivars enhacement patch.
>
> And to answer your immediate question - the new EFI image will be used for
> boot without rolling back.
>
> Also, I am not sure how everyone installs signed grubs. Thus adding support
> for ESP backup and restore might be futile upstream. As far as I can tell
> it is safe to call grub-install on Ubuntu, whereas it might not be on other
> distros. As Ubuntu preserves / reinstalls signed grub EFI images, instead
> of generating a new random one and putting it on ESP thus causing failure
> to boot with secureboot due to effectively replacing signed grub with an
> unsigned one. And for resilient boot we have support for maintaining and
> synchronising multiple ESP for Linux raid.
Installing signed grub can happen in parallel with any unsigned one
installed by grub-install as long as they didn't overlap in the position
on EFI System partition. For that matters the efibootmgr is used to
manage them.
The signed grub is so far not controlled by grub-install, so should be
disregared at this moment. The entire backup/recovery should be done
only for those made aware by grub-install.
>
> I kind of wish we'd prebuilt more core images at package build time and
> simply copy them around. Not just the ones that have signing. Instead of
> invoking mkimage on every installed system.
>
> Nonetheless, it should be possible to hook up more files and directories to
> perform backup, cleanup and restore on. Is there is a desire. The function
> calls simply need to be added in the appropriate places.
IMHO the hook should be unregistered right after the state is considered
complete for new image and modules, or it would be triggered
inadvertently if any error comes after that and hence render the system
incompete again by restoring old backup.
Thanks,
Michael
>
>
> > >
> > > Also add modinfo.sh to the cleanup/backup/restore codepath, to ensure
> > > it is also cleaned / backed up / restored.
> > >
> > > Signed-off-by: Dimitri John Ledkov <x...@ubuntu.com>
> > > ---
> > >
> > > Changes since v1: as reported on linux-ext4 mailing list and debugged
> > > by Colin Watson, the grub_util_path_concat_ext call was incorrect in
> > > the restore backup case as there was no extention needed. Instead the
> > > call is corrected to just grub_util_path_concat.
> > >
> > > This patch is now shipped in Ubuntu & Debian in multiple series. It
> > > would be nice to have this merged upstream, as it greatly improves
> > > grub upgrades and prevents missmatches of MBR & modules.
> > >
> > > configure.ac | 2 +-
> > > util/grub-install-common.c | 105 +++++++++++++++++++++++++++++++------
> > > 2 files changed, 91 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/configure.ac b/configure.ac
> > > index 7c10a4db7..71cd414c3 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 on_exit)
> > > 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/util/grub-install-common.c b/util/grub-install-common.c
> > > index 277eaf4e2..74584b92b 100644
> > > --- a/util/grub-install-common.c
> > > +++ b/util/grub-install-common.c
> > > @@ -185,38 +185,113 @@ 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 = 0,
> > > + CLEAN_BACKUP = 1,
> > > + CREATE_BACKUP = 2,
> > > + RESTORE_BACKUP = 3,
> > > +};
> > > +
> > > 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, ".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_on_exit (int status, void *arg)
> > > +{
> > > + if (status == 0)
> > > + {
> > > + clean_grub_dir_real ((char *) arg, CLEAN_BACKUP);
> > > + }
> > > + else
> > > + {
> > > + clean_grub_dir_real ((char *) arg, CLEAN);
> > > + clean_grub_dir_real ((char *) arg, RESTORE_BACKUP);
> > > + }
> > > + free (arg);
> > > + arg = NULL;
> > > +}
> > > +
> > > +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_ON_EXIT)
> > > + on_exit (restore_backup_on_exit, strdup (di));
> > > +#endif
> > > +}
> > > +
> > > struct install_list
> > > {
> > > int is_default;
> > > --
> > > 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
