Re: [RFC PATCH 2/2] cmd: replace efi_create_handle/add_protocol with InstallMultipleProtocol

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

On Thu, Oct 06, 2022 at 04:26:37AM +0200, Heinrich Schuchardt wrote:
> On 10/6/22 03:53, AKASHI Takahiro wrote:
> > On Wed, Oct 05, 2022 at 06:26:02PM +0300, Ilias Apalodimas wrote:
> > > In general handles should only be deleted if the last remaining protocol
> > > is removed.  Instead of explicitly calling
> > > efi_create_handle -> efi_add_protocol -> efi_delete_handle which blindly
> > > removes all protocols from a handle before removing it,  use
> > > InstallMultiple/UninstallMultiple which adheres to the EFI spec and only
> > > deletes a handle if there are no additional protocols present
> > > 
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > >   cmd/bootefi.c | 13 ++++++-------
> > >   1 file changed, 6 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > > index 3041873afbbc..4ab68868cc7e 100644
> > > --- a/cmd/bootefi.c
> > > +++ b/cmd/bootefi.c
> > > @@ -509,12 +509,9 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
> > >   		 * Make sure that device for device_path exist
> > >   		 * in load_image(). Otherwise, shell and grub will fail.
> > >   		 */
> > > -		ret = efi_create_handle(&mem_handle);
> > > -		if (ret != EFI_SUCCESS)
> > > -			goto out;
> > > -
> > > -		ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> > > -				       file_path);
> > > +		ret = efi_install_multiple_protocol_interfaces(&mem_handle,
> > > +							       &efi_guid_device_path,
> > > +							       file_path, NULL);
> > 
> > nitpick: NULL -> NULL, NULL
> > UEFI spec seems to require "A variable argument list containing pairs of
> > protocol GUIDs and protocol interfaces" even if a protocol interface won't be
> > used with GUID as NULL.
> 
> The spec also has:
> "The pairs of arguments are removed in order from the variable argument
> list until a NULL protocol GUID value is found."
> 
> This is what a calls inside EDK II looks like:
> 
>   Status = CoreInstallMultipleProtocolInterfaces (
>              &mDecompressHandle,
>              &gEfiDecompressProtocolGuid,
>              &gEfiDecompress,
>              NULL
>              );
> 
>       gBS->UninstallMultipleProtocolInterfaces (
>              Controller,
>              &gEfiCallerIdGuid,
>              AtaBusDriverData,
>              NULL
>              );
> 
> So I am happy with a single NULL here.
> 
> > 
> > -Takahiro Akashi
> > 
> > >   		if (ret != EFI_SUCCESS)
> > >   			goto out;
> > >   		msg_path = file_path;
> > > @@ -542,7 +539,9 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
> > >   	ret = do_bootefi_exec(handle, load_options);
> > > 
> > >   out:
> > > -	efi_delete_handle(mem_handle);
> > > +	efi_uninstall_multiple_protocol_interfaces(mem_handle,
> > > +						   &efi_guid_device_path,
> > > +						   file_path, NULL);
> 
> We have installed a lot more protocols. The binary may have installed
> additional protocols. Consider the case of a boottime driver. To delete
> the handle we would have to remove all installed protocols.
> 
> UninstallMultipleProtocolInterfaces() may fail if one of the protocols
> was opened with ByDriver or ByChildcontroller. The return value has to
> be considered before freeing file_path.

Ok I'll fix it in v2

> 
> Best regards
> 
> Heinrich
> 
> > >   	efi_free_pool(file_path);
> > >   	return ret;
> > >   }
> > > --
> > > 2.34.1
> > > 
> 

Thanks
/Ilias