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

[Ilias Apalodimas]

Hi Heinrich,


On Tue, 6 Jun 2023 at 22:56, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> 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.

Yea, I had a similar comment on an earlier version.  EFI_NOT_FOUND was
ignored by the caller in the past, so the current change is fine.

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

_remove() is being called before we exit to the OS, so we would end up
constantly adding/removing boot options and writing to flashes, which
is something we want to avoid.  The downside is that the device
removal and the boot options that might have been added, will be
removed on the next boot.  It's still a problem, but I don't think
it's that important at the moment.
Thanks
/Ilias
>
> 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
> >>>
>