* [PATCH 1/1] efi_loader: fix efi_initrd_deregister()
@ 2022-09-29 23:57 Heinrich Schuchardt
2022-09-30 1:47 ` AKASHI Takahiro
2022-09-30 6:15 ` Ilias Apalodimas
0 siblings, 2 replies; 10+ messages in thread
From: Heinrich Schuchardt @ 2022-09-29 23:57 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: u-boot, Heinrich Schuchardt
Don't try to delete a non-existent handle.
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] efi_loader: fix efi_initrd_deregister()
2022-09-29 23:57 [PATCH 1/1] efi_loader: fix efi_initrd_deregister() Heinrich Schuchardt
@ 2022-09-30 1:47 ` AKASHI Takahiro
2022-09-30 6:18 ` Ilias Apalodimas
2022-09-30 6:15 ` Ilias Apalodimas
1 sibling, 1 reply; 10+ messages in thread
From: AKASHI Takahiro @ 2022-09-30 1:47 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: Ilias Apalodimas, u-boot
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.
efi_initrd_register() is called only in efi_bootmgr_load(), and so
efi_initrd_deregister() should be called only at the paired location.
- 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?
-Takahiro Akashi
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] efi_loader: fix efi_initrd_deregister()
2022-09-29 23:57 [PATCH 1/1] efi_loader: fix efi_initrd_deregister() Heinrich Schuchardt
2022-09-30 1:47 ` AKASHI Takahiro
@ 2022-09-30 6:15 ` Ilias Apalodimas
1 sibling, 0 replies; 10+ messages in thread
From: Ilias Apalodimas @ 2022-09-30 6:15 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: u-boot
Hi Heinrich
On Fri, 30 Sept 2022 at 02:58, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Don't try to delete a non-existent handle.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> 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;
> +
I am not sure what we gain with this. efi_delete_handle() won't run
anyway since it will return EFI_INVALID_PARAMETER no?
I think the rabbit hole is a bit deeper here. I would like to forbid
users calling efi_delete_handle(). In theory and if we want be
pedantic on the EFI spec, we should only use Install/UninstallProtocol
(or even better the multiple variant since it's the only one that
checks for duplicate DPs according to the spec) and have the
UninstallProtocol check if there are remaining protocols in the
handle. If there aren't we should delete the handle as well.
Regards
/Ilias
> efi_delete_handle(efi_initrd_handle);
> efi_initrd_handle = NULL;
> }
> --
> 2.37.2
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] efi_loader: fix efi_initrd_deregister()
2022-09-30 1:47 ` AKASHI Takahiro
@ 2022-09-30 6:18 ` Ilias Apalodimas
2022-09-30 6:41 ` AKASHI Takahiro
0 siblings, 1 reply; 10+ messages in thread
From: Ilias Apalodimas @ 2022-09-30 6:18 UTC (permalink / raw)
To: AKASHI Takahiro, Heinrich Schuchardt, Ilias Apalodimas, u-boot
Akashi-san
On Fri, 30 Sept 2022 at 04:47, AKASHI Takahiro
<takahiro.akashi@linaro.org> 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. The
whole initrd loading logic depends on BootCurrnent, which iirc is not
set yet on the flow you are proposing.
Regards
/Ilias
>
> -Takahiro Akashi
>
> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > ---
> > 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
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] efi_loader: fix efi_initrd_deregister()
2022-09-30 6:18 ` Ilias Apalodimas
@ 2022-09-30 6:41 ` AKASHI Takahiro
2022-09-30 6:54 ` Ilias Apalodimas
0 siblings, 1 reply; 10+ messages in thread
From: AKASHI Takahiro @ 2022-09-30 6:41 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: Heinrich Schuchardt, u-boot
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
> <takahiro.akashi@linaro.org> 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().
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?
Regarding BootCurrent, I don't think it has nothing to do with the discussion above.
Anyhow it *is* set before reaching "if (size) ...".
-Takahiro Akashi
> Regards
> /Ilias
> >
> > -Takahiro Akashi
> >
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > ---
> > > 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
> > >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] efi_loader: fix efi_initrd_deregister()
2022-09-30 6:41 ` AKASHI Takahiro
@ 2022-09-30 6:54 ` Ilias Apalodimas
2022-09-30 7:18 ` Heinrich Schuchardt
0 siblings, 1 reply; 10+ messages in thread
From: Ilias Apalodimas @ 2022-09-30 6:54 UTC (permalink / raw)
To: AKASHI Takahiro, Ilias Apalodimas, Heinrich Schuchardt, u-boot
Akashi-san
On Fri, 30 Sept 2022 at 09:41, AKASHI Takahiro
<takahiro.akashi@linaro.org> 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
> > <takahiro.akashi@linaro.org> 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 <heinrich.schuchardt@canonical.com>
> > > > ---
> > > > 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
> > > >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] efi_loader: fix efi_initrd_deregister()
2022-09-30 6:54 ` Ilias Apalodimas
@ 2022-09-30 7:18 ` Heinrich Schuchardt
2022-09-30 7:29 ` Ilias Apalodimas
0 siblings, 1 reply; 10+ messages in thread
From: Heinrich Schuchardt @ 2022-09-30 7:18 UTC (permalink / raw)
To: Ilias Apalodimas, AKASHI Takahiro; +Cc: u-boot
On 9/30/22 08:54, Ilias Apalodimas wrote:
> Akashi-san
>
> On Fri, 30 Sept 2022 at 09:41, AKASHI Takahiro
> <takahiro.akashi@linaro.org> 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
>>> <takahiro.akashi@linaro.org> 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 <heinrich.schuchardt@canonical.com>
>>>>> ---
>>>>> 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
>>>>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] efi_loader: fix efi_initrd_deregister()
2022-09-30 7:18 ` Heinrich Schuchardt
@ 2022-09-30 7:29 ` Ilias Apalodimas
2022-09-30 7:55 ` Heinrich Schuchardt
0 siblings, 1 reply; 10+ messages in thread
From: Ilias Apalodimas @ 2022-09-30 7:29 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: AKASHI Takahiro, u-boot
Hi Heinrich
On Fri, 30 Sept 2022 at 10:18, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 9/30/22 08:54, Ilias Apalodimas wrote:
> > Akashi-san
> >
> > On Fri, 30 Sept 2022 at 09:41, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> 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
> >>> <takahiro.akashi@linaro.org> 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"
Fair enough. Let's see if Akashi can clean up uninstalling the
protocol, otherwise I am fine with this patch
Cheers
/Ilias
>
> 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 <heinrich.schuchardt@canonical.com>
> >>>>> ---
> >>>>> 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
> >>>>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] efi_loader: fix efi_initrd_deregister()
2022-09-30 7:29 ` Ilias Apalodimas
@ 2022-09-30 7:55 ` Heinrich Schuchardt
2022-09-30 7:59 ` Ilias Apalodimas
0 siblings, 1 reply; 10+ messages in thread
From: Heinrich Schuchardt @ 2022-09-30 7:55 UTC (permalink / raw)
To: Ilias Apalodimas, AKASHI Takahiro; +Cc: u-boot
On 9/30/22 09:29, Ilias Apalodimas wrote:
> Hi Heinrich
>
> On Fri, 30 Sept 2022 at 10:18, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> On 9/30/22 08:54, Ilias Apalodimas wrote:
>>> Akashi-san
>>>
>>> On Fri, 30 Sept 2022 at 09:41, AKASHI Takahiro
>>> <takahiro.akashi@linaro.org> 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
>>>>> <takahiro.akashi@linaro.org> 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"
>
> Fair enough. Let's see if Akashi can clean up uninstalling the
> protocol, otherwise I am fine with this patch
Thanks Takahiro for looking into this.
For 2022.01 I will put this patch into a pull request to avoid
irritating users by the message. For further changes it is too late in
this cycle.
Best regards
Heinrich
>
> Cheers
> /Ilias
>>
>> whenever I return from an EFI binary.
>>
>> Best regards
>>
>> Heinrich
>>
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] efi_loader: fix efi_initrd_deregister()
2022-09-30 7:55 ` Heinrich Schuchardt
@ 2022-09-30 7:59 ` Ilias Apalodimas
0 siblings, 0 replies; 10+ messages in thread
From: Ilias Apalodimas @ 2022-09-30 7:59 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: AKASHI Takahiro, u-boot
On Fri, 30 Sept 2022 at 10:55, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 9/30/22 09:29, Ilias Apalodimas wrote:
> > Hi Heinrich
> >
> > On Fri, 30 Sept 2022 at 10:18, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> On 9/30/22 08:54, Ilias Apalodimas wrote:
> >>> Akashi-san
> >>>
> >>> On Fri, 30 Sept 2022 at 09:41, AKASHI Takahiro
> >>> <takahiro.akashi@linaro.org> 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
> >>>>> <takahiro.akashi@linaro.org> 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"
> >
> > Fair enough. Let's see if Akashi can clean up uninstalling the
> > protocol, otherwise I am fine with this patch
>
> Thanks Takahiro for looking into this.
>
> For 2022.01 I will put this patch into a pull request to avoid
> irritating users by the message. For further changes it is too late in
> this cycle.
>
I guess you mean .10
You can add my Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Best regards
>
> Heinrich
>
> >
> > Cheers
> > /Ilias
> >>
> >> whenever I return from an EFI binary.
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-09-30 7:59 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-29 23:57 [PATCH 1/1] efi_loader: fix efi_initrd_deregister() Heinrich Schuchardt
2022-09-30 1:47 ` AKASHI Takahiro
2022-09-30 6:18 ` Ilias Apalodimas
2022-09-30 6:41 ` AKASHI Takahiro
2022-09-30 6:54 ` Ilias Apalodimas
2022-09-30 7:18 ` Heinrich Schuchardt
2022-09-30 7:29 ` Ilias Apalodimas
2022-09-30 7:55 ` Heinrich Schuchardt
2022-09-30 7:59 ` Ilias Apalodimas
2022-09-30 6:15 ` Ilias Apalodimas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox