diff mbox series

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

Message ID 20221005152603.3085754-3-ilias.apalodimas@linaro.org
State New
Headers show
Series Clean up protocol installation API | expand

Commit Message

Ilias Apalodimas Oct. 5, 2022, 3:26 p.m. UTC 
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(-)

Comments

AKASHI Takahiro Oct. 6, 2022, 1:53 a.m. UTC | #1 
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.

-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);
>  	efi_free_pool(file_path);
>  	return ret;
>  }
> -- 
> 2.34.1
>
Heinrich Schuchardt Oct. 6, 2022, 2:26 a.m. UTC | #2 
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.

Best regards

Heinrich

>>   	efi_free_pool(file_path);
>>   	return ret;
>>   }
>> --
>> 2.34.1
>>
Ilias Apalodimas Oct. 6, 2022, 7:38 a.m. UTC | #3 
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
Heinrich Schuchardt Oct. 6, 2022, 9:43 a.m. UTC | #4 
On 10/6/22 09:38, Ilias Apalodimas wrote:
> 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

We only install the device path file_path on mem_handle. The loaded
image itself is referenced by handle. So the only thing you missed here
was checking the return value.

Best regards

Heinrich

>
>>
>> Best regards
>>
>> Heinrich
>>
>>>>    	efi_free_pool(file_path);
>>>>    	return ret;
>>>>    }
>>>> --
>>>> 2.34.1
>>>>
>>
>
> Thanks
> /Ilias
diff mbox series

Patch

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);
 		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);
 	efi_free_pool(file_path);
 	return ret;
 }