Re: [PATCH] Mandatory install device check for PowerPC

[Michal Suchánek]

Hello,

thanks for the patch!

On Sat, Nov 09, 2024 at 11:20:08AM +0530, avnish wrote:
> Hi Vladimir,
> Thank you so much for your response!
> 
> I have fine tuned the patch as per the last discussion (sorry, I missed the
> v2 tag). This latest patch will add install device check only to PowerPC
> machines. PowerMacs aren't affected by this change. The check is added when
> platform is detected as "GRUB_INSTALL_PLATFORM_POWERPC_IEEE1275" along with
> machine detected as non PowerMac. As per my Power platform analysis,
> currently in "grub_install.c", it detects PowerMacs based on the file system
> detected (HFS or HFS+) and set the "is_prep" as 0 based on this finding.
> This new check will only be applicable to PowerPC. And in case of PowerMacs,
> it will allow grub_install even without mentioning the install device.
> Thank you!
> 
> 
> Regards,
> Avnish Chouhan
> > ------------------------------
> > 
> > Message: 5
> > Date: Fri, 8 Nov 2024 15:07:29 +0300
> > From: "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>
> > To: The development of GNU GRUB <grub-devel@gnu.org>
> > Subject: Re: [PATCH] Mandatory install device check for PowerPC
> > Message-ID:
> >     <CAEaD8JMqP4_uP5cZutSMGWvGMxbHAvNh10VCMO4ZcbqvLAQ9zw@mail.gmail.com>
> > Content-Type: text/plain; charset="utf-8"
> > 
> > As discussed in another thread, this breaks installing from x86 onto
> > removable disk for PPC Mac which is a supported workflow

Please be more specific. I cannot find how this version of the patch
still breaks other platforms. Given that you are talking about
cross-installation form x86 this should be eeasy to test given detailed
description.

> > 
> > Le ven. 8 nov. 2024, 14:13, Avnish Chouhan <avnish@linux.ibm.com> a
> > écrit :
> > 
> > > This patch adds a check on install_device while installing grub for
> > > PowerPC.
> > > If install_device is not mentioned in grub2-install and machine is
> > > detected
> > > as PowerPC, the error will be thrown and it will terminates the
> > > grub2-install
> > > operation. Running grub2-install on PowerPC without the
> > > install_device may
> > > result in bootlist corruption. When no install device is specified, it
> > > attempts
> > > to load images from the filesystem, which leads to nvram bootlist
> > > corruption.
> > > The idea is to fail the operation and avoid creating the invalid boot
> > > entry.
> > > 
> > > Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
> > > ---
> > >  grub-install.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)

Before here there is this code:

if (install_device)
  is_prep = 1;

This is the root of the problem. The code sets is_prep based on user
input, and when the input is wrong is_prep remains wrongly unset,
leading to bogus entry written to bootlist, and the system becoming
unbootable.

Instead this code shuld be removed, and is_prep initialized to 1.

> > > 
> > > diff --git a/util/grub-install.c b/util/grub-install.c
> > > index 7dc5657..a049f53 100644
> > > --- a/util/grub-install.c
> > > +++ b/util/grub-install.c
> > > @@ -1289,6 +1289,17 @@ main (int argc, char *argv[])
> > >               is_prep = 0;

Here is_prep is unset when a Mac boot partition is found. With that
initializing is_prep to 1 gives sound logic for determining if the
system looks like a PowerMac or not.

There is the possibility that grub-install would run on a PowerMac, and
the Mac boot partition is not given by the user nor autodetected but
there is not much that can be done about that given cross-installation
is supported. This case can't work currently either.

> > >             }
> > >         }
> > > +      else

This logic is still not sound. Though unlikely grub-install can find
something that looks like a Mac boot partition (is_guess is 1) but is
not one (does not unset is_prep).

Instead of

else if (!install_device)

if (is_prep && !install_device)

can be used when is_prep is initialized to true unconditionally. Then
the code above sets is_prep to false when a Mac partition is found, and
if none is found and install device is not set it's an error.

> > > +        {
> > > +         /*
> > > +          * As the machine has been detected as PowerPC and not the
> > > PowerMac. We need to check
> > > +          * whether the install_device has been mentioned while
> > > installing. If no device has been
> > > +          * mentioned, we need to exit and mark it as an error as the
> > > install_device is required for
> > > +          * PowerPC installation. An installation with no device
> > > mentioned may lead to corruptions.
> > > +          */
> > > +           if (!install_device)
> > > +             grub_util_error ("%s", _("install device isn't specified
> > > required for PowerPC"));

This message is rather awkward and misleading.

I think something like "install device required on PReP platform" is as
good as it gets.

The platform naming is quite unfortunate. The port to Power family of
CPUs is called powerpc or ppc but PowerPC refers to the desktop family
of CPUs found mainly in PowerMac hardware.

PReP is a standard originally meant for use with PowerPC based hardware,
and that's where the partition name comes from but was superseded by
CHRP and PAPR. PAPR specifically is used for server and embedded CPUs,
not desktop, and that's what is supported besides PowerMac. I doubt
current version of GRUB would work on the original PReP hardware.

Thanks

Michal