[PATCH] i386-pc: build verifiers API as module

[PATCH] i386-pc: build verifiers API as module

[Michael Chang]

Given no core functions on i386-pc would require verifiers to work and
the only consumer of the verifier API is the pgp module, it looks good
to me that we can move the verifiers out of the kernel image and let
moddep.lst to auto-load it when pgp is loaded on i386-pc platform.

This helps to reduce the size of core image and thus can relax the
tension of exploading on some i386-pc system with very short MBR gap
size. See also a very comprehensive summary from Colin [1] about the
details.

[1] https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00240.html

Reported-by: Colin Watson <cjwatson@debian.org>
Signed-off-by: Michael Chang <mchang@suse.com>
---
 configure.ac                |  1 +
 grub-core/Makefile.am       |  2 ++
 grub-core/Makefile.core.def |  8 +++++++-
 grub-core/kern/main.c       |  4 ++++
 grub-core/kern/verifiers.c  | 11 +++++++++++
 include/grub/verify.h       |  9 +++++++++
 6 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 74719416c..bbb91e80b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1970,6 +1970,7 @@ AC_SUBST(BUILD_LIBM)
 AM_CONDITIONAL([COND_real_platform], [test x$platform != xnone])
 AM_CONDITIONAL([COND_emu], [test x$platform = xemu])
 AM_CONDITIONAL([COND_i386_pc], [test x$target_cpu = xi386 -a x$platform = xpc])
+AM_CONDITIONAL([COND_NOT_i386_pc], [test x$target_cpu != xi386 -o x$platform != xpc])
 AM_CONDITIONAL([COND_i386_efi], [test x$target_cpu = xi386 -a x$platform = xefi])
 AM_CONDITIONAL([COND_ia64_efi], [test x$target_cpu = xia64 -a x$platform = xefi])
 AM_CONDITIONAL([COND_i386_qemu], [test x$target_cpu = xi386 -a x$platform = xqemu])
diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index ee88e44e9..fca0df9de 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -93,7 +93,9 @@ KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/partition.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/stack_protector.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/term.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/time.h
+if COND_NOT_i386_pc
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/verify.h
+endif
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/mm_private.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/net.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/memory.h
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 8022e1c0a..77fdccdb1 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -141,7 +141,7 @@ kernel = {
   common = kern/rescue_parser.c;
   common = kern/rescue_reader.c;
   common = kern/term.c;
-  common = kern/verifiers.c;
+  nopc = kern/verifiers.c;
 
   noemu = kern/compiler-rt.c;
   noemu = kern/mm.c;
@@ -946,6 +946,12 @@ module = {
   cppflags = '-I$(srcdir)/lib/posix_wrap';
 };
 
+module = {
+  name = verifiers;
+  common = kern/verifiers.c;
+  enable = i386_pc;
+};
+
 module = {
   name = hdparm;
   common = commands/hdparm.c;
diff --git a/grub-core/kern/main.c b/grub-core/kern/main.c
index 73967e2f5..c7c6d2d0b 100644
--- a/grub-core/kern/main.c
+++ b/grub-core/kern/main.c
@@ -29,7 +29,9 @@
 #include <grub/command.h>
 #include <grub/reader.h>
 #include <grub/parser.h>
+#ifndef GRUB_MACHINE_PCBIOS
 #include <grub/verify.h>
+#endif
 
 #ifdef GRUB_MACHINE_PCBIOS
 #include <grub/machine/memory.h>
@@ -275,8 +277,10 @@ grub_main (void)
   grub_printf ("Welcome to GRUB!\n\n");
   grub_setcolorstate (GRUB_TERM_COLOR_STANDARD);
 
+#ifndef GRUB_MACHINE_PCBIOS
   /* Init verifiers API. */
   grub_verifiers_init ();
+#endif
 
   grub_load_config ();
 
diff --git a/grub-core/kern/verifiers.c b/grub-core/kern/verifiers.c
index 75d7994cf..85887917d 100644
--- a/grub-core/kern/verifiers.c
+++ b/grub-core/kern/verifiers.c
@@ -221,8 +221,19 @@ grub_verify_string (char *str, enum grub_verify_string_type type)
   return GRUB_ERR_NONE;
 }
 
+#ifdef GRUB_MACHINE_PCBIOS
+GRUB_MOD_INIT(verifiers)
+#else
 void
 grub_verifiers_init (void)
+#endif
 {
   grub_file_filter_register (GRUB_FILE_FILTER_VERIFY, grub_verifiers_open);
 }
+
+#ifdef GRUB_MACHINE_PCBIOS
+GRUB_MOD_FINI(verifiers)
+{
+  grub_file_filter_unregister (GRUB_FILE_FILTER_VERIFY);
+}
+#endif
diff --git a/include/grub/verify.h b/include/grub/verify.h
index cd129c398..6fde244fc 100644
--- a/include/grub/verify.h
+++ b/include/grub/verify.h
@@ -64,10 +64,14 @@ struct grub_file_verifier
   grub_err_t (*verify_string) (char *str, enum grub_verify_string_type type);
 };
 
+#ifdef GRUB_MACHINE_PCBIOS
+extern struct grub_file_verifier *grub_file_verifiers;
+#else
 extern struct grub_file_verifier *EXPORT_VAR (grub_file_verifiers);
 
 extern void
 grub_verifiers_init (void);
+#endif
 
 static inline void
 grub_verifier_register (struct grub_file_verifier *ver)
@@ -81,7 +85,12 @@ grub_verifier_unregister (struct grub_file_verifier *ver)
   grub_list_remove (GRUB_AS_LIST (ver));
 }
 
+#ifdef GRUB_MACHINE_PCBIOS
+grub_err_t
+grub_verify_string (char *str, enum grub_verify_string_type type);
+#else
 extern grub_err_t
 EXPORT_FUNC (grub_verify_string) (char *str, enum grub_verify_string_type type);
+#endif
 
 #endif /* ! GRUB_VERIFY_HEADER */
-- 
2.26.2

Re: [PATCH] i386-pc: build verifiers API as module

[Colin Watson]

On Thu, Mar 18, 2021 at 03:14:34PM +0800, Michael Chang via Grub-devel wrote:
> Given no core functions on i386-pc would require verifiers to work and
> the only consumer of the verifier API is the pgp module, it looks good
> to me that we can move the verifiers out of the kernel image and let
> moddep.lst to auto-load it when pgp is loaded on i386-pc platform.
> 
> This helps to reduce the size of core image and thus can relax the
> tension of exploading on some i386-pc system with very short MBR gap
> size. See also a very comprehensive summary from Colin [1] about the
> details.

Thanks for working on this!  It's certainly awkward to have to deal with
this sort of thing, but apparently not as awkward as I'd feared, and
it's better than the alternative.

> +AM_CONDITIONAL([COND_NOT_i386_pc], [test x$target_cpu != xi386 -o x$platform != xpc])

You could drop this and instead just do "if !COND_i386_pc" in
grub-core/Makefile.am.

> diff --git a/grub-core/kern/verifiers.c b/grub-core/kern/verifiers.c
> index 75d7994cf..85887917d 100644
> --- a/grub-core/kern/verifiers.c
> +++ b/grub-core/kern/verifiers.c
> @@ -221,8 +221,19 @@ grub_verify_string (char *str, enum grub_verify_string_type type)
>    return GRUB_ERR_NONE;
>  }
>  
> +#ifdef GRUB_MACHINE_PCBIOS
> +GRUB_MOD_INIT(verifiers)
> +#else
>  void
>  grub_verifiers_init (void)
> +#endif

I think a comment here (or somewhere in the actual code, anyway, I don't
mind where) would be useful so that people trying to work out what's
going on don't have to hunt through commit logs to find out.

If these minor comments are fixed:

Reviewed-by: Colin Watson <cjwatson@debian.org>

-- 
Colin Watson (he/him)                              [cjwatson@debian.org]

Re: [PATCH] i386-pc: build verifiers API as module

[Michael Chang]

On Thu, Mar 18, 2021 at 09:23:40AM +0000, Colin Watson wrote:
> On Thu, Mar 18, 2021 at 03:14:34PM +0800, Michael Chang via Grub-devel wrote:
> > Given no core functions on i386-pc would require verifiers to work and
> > the only consumer of the verifier API is the pgp module, it looks good
> > to me that we can move the verifiers out of the kernel image and let
> > moddep.lst to auto-load it when pgp is loaded on i386-pc platform.
> > 
> > This helps to reduce the size of core image and thus can relax the
> > tension of exploading on some i386-pc system with very short MBR gap
> > size. See also a very comprehensive summary from Colin [1] about the
> > details.
> 
> Thanks for working on this!  It's certainly awkward to have to deal with
> this sort of thing, but apparently not as awkward as I'd feared, and
> it's better than the alternative.

If I remember correctly in the past every core image size increase on
i386-pc build is counted. IMHO we should keep up with that practice at
least in reviewing new release ... 

> 
> > +AM_CONDITIONAL([COND_NOT_i386_pc], [test x$target_cpu != xi386 -o x$platform != xpc])
> 
> You could drop this and instead just do "if !COND_i386_pc" in
> grub-core/Makefile.am.

Indeed it looks superfluous to invent COND_NOT_i386_pc here. I will fix
in next patch.

> 
> > diff --git a/grub-core/kern/verifiers.c b/grub-core/kern/verifiers.c
> > index 75d7994cf..85887917d 100644
> > --- a/grub-core/kern/verifiers.c
> > +++ b/grub-core/kern/verifiers.c
> > @@ -221,8 +221,19 @@ grub_verify_string (char *str, enum grub_verify_string_type type)
> >    return GRUB_ERR_NONE;
> >  }
> >  
> > +#ifdef GRUB_MACHINE_PCBIOS
> > +GRUB_MOD_INIT(verifiers)
> > +#else
> >  void
> >  grub_verifiers_init (void)
> > +#endif
> 
> I think a comment here (or somewhere in the actual code, anyway, I don't
> mind where) would be useful so that people trying to work out what's
> going on don't have to hunt through commit logs to find out.

OK. I will add the comment to explain what's going on in next patch.

> 
> If these minor comments are fixed:
> 
> Reviewed-by: Colin Watson <cjwatson@debian.org>

Thanks. I will also add your Reviewed-by ...

Regards,
Michael

> 
> -- 
> Colin Watson (he/him)                              [cjwatson@debian.org]
>