[PATCH v8 2/4] Fix incorrect return code of boot option update

[Heinrich Schuchardt]

On 6/6/23 21:43, Raymond Mao wrote:
> Hi Ilias,
>
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
>>>                                             NULL, &count,
>>>                                             (efi_handle_t **)&volume_handles);
>>>          if (ret != EFI_SUCCESS)
>>> -               return ret;
>>> +               goto out;
>>
>> I missed that in my original review, but I think this change is wrong.
>> If you follow the goto out tag now instead of returning directly
>> there's a chance efi_locate_handle_buffer_int() will return
>> EFI_NOT_FOUND.  The goto out tag will convert that to EFI_SUCCESS
>> although it's an actual error.
> This is what we intend to do. When there are no removable medias plug-in,
> efi_locate_handle_buffer_int() will return EFI_NOT_FOUND.
> And if we don't convert it to EFI_SUCCESS here,
> efi_bootmgr_update_media_device_boot_option()
> will return EFI_NOT_FOUND and then efi_init_obj_list() fails.
> This leads to the efi init failure and it will just prompt 'Error:
> Cannot initialize UEFI sub-system'
> when you run any efidebug commands.
>
> Regards,
> Raymond
>
>
> On Tue, 6 Jun 2023 at 15:24, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
>>
>> Hi Raymond
>>
>> On Tue, 6 Jun 2023 at 19:37, Raymond Mao <raymond.mao at linaro.org> wrote:
>>>
>>> Correct the return code for out-of-memory and no boot option found
>>>
>>> Signed-off-by: Raymond Mao <raymond.mao at linaro.org>
>>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
>>> ---
>>> Changes in v7
>>> - new patch file created
>>>
>>>   cmd/bootmenu.c               | 2 +-
>>>   cmd/eficonfig.c              | 2 +-
>>>   lib/efi_loader/efi_bootmgr.c | 8 ++++++--
>>>   3 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
>>> index 01daddca7b..987b16889f 100644
>>> --- a/cmd/bootmenu.c
>>> +++ b/cmd/bootmenu.c
>>> @@ -352,7 +352,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
>>>                   * a architecture-specific default image name such as BOOTAA64.EFI.
>>>                   */
>>>                  efi_ret = efi_bootmgr_update_media_device_boot_option();
>>> -               if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
>>> +               if (efi_ret != EFI_SUCCESS)
>>>                          goto cleanup;
>>>
>>>                  ret = prepare_uefi_bootorder_entry(menu, &iter, &i);
>>> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
>>> index 82a80306f4..e6e8a0a488 100644
>>> --- a/cmd/eficonfig.c
>>> +++ b/cmd/eficonfig.c
>>> @@ -2314,7 +2314,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a
>>>                  return CMD_RET_FAILURE;
>>>
>>>          ret = efi_bootmgr_update_media_device_boot_option();
>>> -       if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
>>> +       if (ret != EFI_SUCCESS)
>>>                  return ret;
>>>
>>>          while (1) {
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index 97d5b8dc2b..28e800499c 100644
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
>>>                                             NULL, &count,
>>>                                             (efi_handle_t **)&volume_handles);
>>>          if (ret != EFI_SUCCESS)
>>> -               return ret;
>>> +               goto out;
>>
>> I missed that in my original review, but I think this change is wrong.
>> If you follow the goto out tag now instead of returning directly
>> there's a chance efi_locate_handle_buffer_int() will return
>> EFI_NOT_FOUND.  The goto out tag will convert that to EFI_SUCCESS
>> although it's an actual error.

It was due to my reply
https://lore.kernel.org/u-boot/e951264b-f952-3e83-dacc-bbe2fa37715f@gmx.de/
that Raymond moved the EFI_NOT_FOUND handling into this function. As
said non-availability of a file system on removable media should not
stop the initialization of the EFI sub-system or the probing of block
devices.

Why is efi_bootmgr_update_media_device_boot_option() not invoked when
block devices are removed?

Best regards

Heinrich

>>
>> Thanks
>> /Ilias
>>>
>>>          opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
>>> -       if (!opt)
>>> +       if (!opt) {
>>> +               ret = EFI_OUT_OF_RESOURCES;
>>>                  goto out;
>>> +       }
>>>
>>>          /* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
>>>          ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);
>>> @@ -720,5 +722,7 @@ out:
>>>          free(opt);
>>>          efi_free_pool(volume_handles);
>>>
>>> +       if (ret == EFI_NOT_FOUND)
>>> +               return EFI_SUCCESS;
>>>          return ret;
>>>   }
>>> --
>>> 2.25.1
>>>