On 9/30/22 08:54, Ilias Apalodimas wrote:
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?
I don't like to see a message
"Can't remove invalid handle %p\n"
whenever I return from an EFI binary.
Best regards
Heinrich
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
