Akashi-san On Fri, 30 Sept 2022 at 09:41, AKASHI Takahiro <[email protected]> wrote: > > Ilias, > > On Fri, Sep 30, 2022 at 09:18:35AM +0300, Ilias Apalodimas wrote: > > Akashi-san > > > > On Fri, 30 Sept 2022 at 04:47, AKASHI Takahiro > > <[email protected]> wrote: > > > > > > On Fri, Sep 30, 2022 at 01:57:48AM +0200, Heinrich Schuchardt wrote: > > > > Don't try to delete a non-existent handle. > > > > > > It is okay as a safe guard, but it doesn't fix underlying issues. > > > > I dont think we safeguard anything. That code path won't try to delete > > anything regardless? > > > > > > > > efi_initrd_register() is called only in efi_bootmgr_load(), and so > > > efi_initrd_deregister() should be called only at the paired location. > > > > There's a reason for that. > > > > > > - Remove efi_initrd_deregister() from do_bootefi_exec() > > > - do_efibootmgr() should look like > > > efi_bootmgr_load() > > > do_bootefi_exec() > > > efi_initrd_deregister() > > > Otherwise, do_bootefi_exec() always tries to free a handle in > > > the other cases (i.e. bootefi <addr>). > > > > > > Another issue is there in try_load_entry() called by efi_bootmgr_load(). > > > (after efi_initrd_register()) > > > if (size) { > > > *load_options = malloc(size); > > > if (!*load_options) { > > > ret = EFI_OUT_OF_RESOURCES; > > > goto error; > > > } > > > ... > > > > > > If malloc() fails, we should call efi_initrd_deregister() within > > > try_load_entry(). > > > > > > Should I submit a patch? > > > > The whole implementation on the *kernel* assumes the protocol is > > present if the file it's pointing is real and existing. You also need > > to have a single instance of the protocol installed. IOW if you > > install the protocol and the initrd is not there, the kernel won't > > fallback on the dt /chosen/ node or the initrd= in the cmdline. > > Yes, I confirmed that before I made my comment. > > > The > > whole initrd loading logic depends on BootCurrnent, which iirc is not > > set yet on the flow you are proposing. > > I don't get your point. > > In do_efibootmgr(), what I suggested above is: > - efi_bootmgr_load() installs LOAD_FILE2_PROTOCOL if initrd file exists, > and if this function fails, LOAD_FILE2_PROTOCOL must be uninstalled any way. > - after returning from UEFI app invoked by do_bootefi_exec(), > - we should simply uninstall LOAD_FILE2_RPTOCOL by calling > efi_initrd_deregister().
You are saying efi_bootmgr_load() installs the protocol., but the point here is it's try_load_entry() which installs the protocol, because we need BootCurrent to have a valid value before we do that. > > In "bootefi <addr>" case, efi_bootmgr_load() is not called, so > LOAD_FILE2_PROTOCOL won't be installed for loading initrd file. > Why do we have to call efi_initrd_deregister() in that case? We don't. FWIW I am not against the cleanup. I am just trying to make sure you have all the details in your head before you move forward. > > Regarding BootCurrent, I don't think it has nothing to do with the discussion > above. The whole initrd loading and checking code depends on BootCurrent being set, so unless the protocol installation is called after BootCurrent being set in try_load_entry() it will fail. What that code path does is check BootCurrent's LoadOptions. The initrd DP is encoded in the FIlePathList (iirc it's on position [1] of that array) and that's how it tries to reason about the file being there or not. Thanks /Ilias > Anyhow it *is* set before reaching "if (size) ...". > > -Takahiro Akashi > > > > Regards > > /Ilias > > > > > > -Takahiro Akashi > > > > > > > Signed-off-by: Heinrich Schuchardt <[email protected]> > > > > --- > > > > lib/efi_loader/efi_load_initrd.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/lib/efi_loader/efi_load_initrd.c > > > > b/lib/efi_loader/efi_load_initrd.c > > > > index c5e6652e66..3d6044f760 100644 > > > > --- a/lib/efi_loader/efi_load_initrd.c > > > > +++ b/lib/efi_loader/efi_load_initrd.c > > > > @@ -230,6 +230,9 @@ efi_status_t efi_initrd_register(void) > > > > */ > > > > void efi_initrd_deregister(void) > > > > { > > > > + if (!efi_initrd_handle) > > > > + return; > > > > + > > > > efi_delete_handle(efi_initrd_handle); > > > > efi_initrd_handle = NULL; > > > > } > > > > -- > > > > 2.37.2 > > > >
