Re: [PATCH] util/grub.d/30_os-prober.in: Fix GRUB_OS_PROBER_SKIP_LIST for non-EFI

[Pascal Hambourg]

On 27/04/2024 at 08:32, Glenn Washburn wrote:
> On Fri, 19 Jan 2024 11:28:55 +0100
> Pascal Hambourg <pascal@plouf.fr.eu.org> wrote:
>
> > GRUB documentation states:
> >
> > 'GRUB_OS_PROBER_SKIP_LIST'
> >    List of space-separated FS UUIDs of filesystems to be ignored from
> >    os-prober output. For efi chainloaders it's <UUID>@<EFI FILE>
> >
> > But the actual behaviour does not match this description. Setting
> >
> >    GRUB_OS_PROBER_SKIP_LIST="<UUID>"
> >
> > does not ignore non-EFI boot loaders detected on filesystem <UUID>.
> > In order to skip non-EFI boot loaders, you must set
> >
> >    GRUB_OS_PROBER_SKIP_LIST="<UUID>@<DEVICE>"
> >
> > which is both absurd (UUID and device are redundant) and wrong
> > (device names such as /dev/sd* may not be persistent across boots).
> >
> > This patch fixes the detection of "@<EFI FILE>" in the device string
> > reported by os-prober.
> >
> > Fixes: 55e706c9 (Add GRUB_OS_PROBER_SKIP_LIST to selectively skipping systems)
> >
> > Note: the UUID matching regex uses word boundaries '\b' but '@' is
> > not a word character. As a consequence, setting
> >
> >    GRUB_OS_PROBER_SKIP_LIST="<UUID>@<ANYTHING>"
> >
> > will ignore non-EFI boot loaders detected on filesystem <UUID> even
> > though the goal is to ignore a given EFI boot loader only.
> > However I think this is desirable to preserve the behaviour of
> > existing setups which use GRUB_OS_PROBER_SKIP_LIST="<UUID>@<DEVICE>".

Additionally, '-' may be present in UUIDs and is not a word character 
either, so the regex may also match UUID substrings instead of whole 
UUIDs only. For example a FAT UUID may match an ext4 UUID substring.

> It would be good to have this in a comment in an appropriate place in
> the code. I know of no complaints about this,

I found these in grub lists:
<https://lists.gnu.org/archive/html/help-grub/2018-11/msg00005.html>
<https://lists.gnu.org/archive/html/bug-grub/2022-12/msg00002.html>

And in a few forum threads:
<https://www.linuxquestions.org/questions/linux-desktop-74/solved-grub-detecting-2-windows10-only-one-really-works-4175678998/>
<https://forum.manjaro.org/t/after-installing-manjaro-xfce-there-are-two-options-for-boot-in-my-uefi-one-is-uefi-os-and-another-one-is-my-ssd-which-one-should-i-choose/12592/15>
<https://bbs.archlinux.org/viewtopic.php?pid=2075691#p2075691>
<https://superuser.com/questions/237565/how-to-hide-recovery-partition-from-grub2>
<https://askubuntu.com/questions/453038/update-grub-ignoring-some-options-in-etc-default-grub>

I guess some people have come to believe the current behaviour is 
normal, for example 
<https://wiki.phobos-net.fr/doku.php/sysadmin/grub2/supprimer_options_demarrage>

> but I think it would be
> good to work on transitioning to what should be done.

What is the "correct behavior" regarding items containing '@' ?
Is it acceptable that "<UUID>@<EFIPATH>" skips non-EFI boot loaders, as 
implemented by this patch ?
Or should only "<UUID>" and "<UUID>@/dev/*" skip non-EFI boot loaders ?

Is is acceptable to skip a boot loader if its UUID is only a substring 
of a UUID in GRUB_OS_PROBER_SKIP_LIST (which is highly unlikely) ?

> Perhaps the
> correct behavior should be available with a envvar and wait a full
> release to to enable by default giving people time to transition.

What do you mean by "available with a envvar" ?
- Define a new variable such as GRUB_OS_PROBER_UUID_SKIP_LIST whose 
processing implements the correct behaviour and preserve 
GRUB_OS_PROBER_SKIP_LIST current processing ?
- Define a new variable such as GRUB_OS_PROBER_SKIP_LIST_FIX whose value 
affects the processing of GRUB_OS_PROBER_SKIP_LIST ?
- Something else ?

> > Signed-off-by: Pascal Hambourg <pascal@plouf.fr.eu.org>
> > ---
> >   util/grub.d/30_os-prober.in | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/util/grub.d/30_os-prober.in b/util/grub.d/30_os-prober.in
> > index e9e217208..a24c01334 100644
> > --- a/util/grub.d/30_os-prober.in
> > +++ b/util/grub.d/30_os-prober.in
> > @@ -125,7 +125,7 @@ for OS in ${OSPROBED} ; do
> >     if UUID="`${grub_probe} --target=fs_uuid --device ${DEVICE%@*}`"; then
> >       EXPUUID="$UUID"
> >   
> > -    if [ x"${DEVICE#*@}" != x ] ; then
> > +    if [ x"${DEVICE%@*}" != x"${DEVICE}" ] ; then
> >         EXPUUID="${EXPUUID}@${DEVICE#*@}"
> >       fi