Re: [RFC PATCH v2 1/1] kern/dl: Add module version check

[Glenn Washburn]

On Fri, 23 Dec 2022 16:18:43 +0800
Zhang Boyang <zhangboyang.id@gmail.com> wrote:

> Hi,

Hi, sorry I missed this til now.

> On 2022/12/22 14:31, Glenn Washburn wrote:
> > On Wed, 21 Dec 2022 20:11:57 +0800
> > Zhang Boyang <zhangboyang.id@gmail.com> wrote:
> >> +      grub_printf_ (N_("warning: module %.63s has incorrect
> >> version: %.63s != %s\n"),
> >> +              mod->name, modver, PACKAGE_VERSION);
> > 
> > I don't quite like this, but I could live with it. I mostly don't
> > like it because I don't want to get spammed with a lot of these
> > warning messages. But maybe this is the best of the bad set of
> > options.
> > 
> 
> I sent a V3 patch at 
> https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00280.html ,
> in which can disable this warning at build time, however I personally
> don't like it.
> 
> I think preloading modules is a good solution for you. e.g. run
> `insmod foobar` before switching $root/$prefix. This also eliminate
> the possibility of GRUB crashing because of mismatched modules. For a 
> comprehensive list of modules, please see $GRUB_MODULES in 
> https://salsa.debian.org/grub-team/grub/-/blob/master/debian/build-efi-images
> 
> If you think this solution is good to you, I want to drop V3 and
> submit a improved V2 as V4, because it's cleaner.

I don't like that because it requires a priori knowledge of what
modules are being loaded in the sourced config file. However, I do
already preload some modules. Perhaps out of scope for this
patch, but I've been thinking about have a variable modules_prefix,
which is used only to load modules, or some way of decoupling the
module load path from $root. Then I can set that and not worry about how
$root is changed in the script.

> > One thing to note is I believe that this will take GRUB out
> > of graphical mode and put it into text mode. This doesn't affect me
> > (right now) though.
> > 
> 
> I tested on i386-pc and x86_64-efi and there is no such behavior.

Your test was with loading a mismatched module, where the warning is
triggered?

> > Another thing is that, for the use case of loading a grub.cfg from a
> > different GRUB installation, this message from version mismatched
> > modules loaded in the sourcing of the script will be displayed for a
> > fraction of a second until the menu from the sourced grub.cfg is
> > displayed. So it may not be helpful for the user.
> > 
> 
> What about adding a 100ms sleep when the warning is displayed? By the 
> way, this can also be workaround by preloading modules.

Is that long enough to make it readable? It would be nice if GRUB had
a log buffer for the debug log messages. Then it could be put in the
debug log and the user could see that there was a version mismatch.

Anyway, since its just a warning message an doesn't prevent the module
from loading (when not in lockdown), its not really worse than the
current state. Except for getting spammed, which isn't a huge deal. So I
can live with this implementation.

Glenn

> 
> > This isn't likely an issue for loopback.cfg because usually the
> > module loading is done implicitly and all the commands are
> > generally inside the menu entries. In this case, printing to the
> > screen seems like a good idea because directly after the module
> > loading the commands will run, which might crash GRUB. So if the
> > crash hangs the machine the user will see this message and have a
> > clue.
> >
> 
> Best Regards,
> Zhang Boyang
> 
> > Glenn
> >