diff mbox series

[2/2] efi_selftest: check for deleted handles in the event notification list

Message ID 20230601120625.1843555-2-ilias.apalodimas@linaro.org
State New
Headers show
Series [1/2] efi_loader: delete handle from events when a protocol is uninstalled | expand

Commit Message

Ilias Apalodimas June 1, 2023, 12:06 p.m. UTC 
A previous patch is fixing a problem where handles were added in
an event notification list, but were never deleted when the handle got
deleted.  Add a test case which calls
- RegisterProtocolNotify
- IstallMultipleProtocolInterface
- UnIstallMultipleProtocolInterface
- LocateHandleBuffer(ByRegisterNotify)
The last call should return EFI_NOT_FOUND

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Heinrich this is not rebased on top of
https://lore.kernel.org/u-boot/20230601070609.14977-1-heinrich.schuchardt@canonical.com/

 .../efi_selftest_register_notify.c            | 61 ++++++++++++++++++-
 1 file changed, 59 insertions(+), 2 deletions(-)

--
2.39.2

Comments

Heinrich Schuchardt June 1, 2023, 4:27 p.m. UTC | #1 
On 6/1/23 14:06, Ilias Apalodimas wrote:
> A previous patch is fixing a problem where handles were added in
> an event notification list, but were never deleted when the handle got
> deleted.  Add a test case which calls
> - RegisterProtocolNotify
> - IstallMultipleProtocolInterface
> - UnIstallMultipleProtocolInterface
> - LocateHandleBuffer(ByRegisterNotify)
> The last call should return EFI_NOT_FOUND
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> Heinrich this is not rebased on top of
> https://lore.kernel.org/u-boot/20230601070609.14977-1-heinrich.schuchardt@canonical.com/
>
>   .../efi_selftest_register_notify.c            | 61 ++++++++++++++++++-
>   1 file changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/lib/efi_selftest/efi_selftest_register_notify.c b/lib/efi_selftest/efi_selftest_register_notify.c
> index ad763dd6cb8b..d133c7c50b71 100644
> --- a/lib/efi_selftest/efi_selftest_register_notify.c
> +++ b/lib/efi_selftest/efi_selftest_register_notify.c
> @@ -33,8 +33,14 @@ static efi_guid_t guid1 =
>   static efi_guid_t guid2 =
>   	EFI_GUID(0xf909f2bb, 0x90a8, 0x0d77,
>   		 0x94, 0x0c, 0x3e, 0xa8, 0xea, 0x38, 0xd6, 0x6f);
> +static efi_guid_t guid3 =
> +	EFI_GUID(0xfa09f2cb, 0x70a8, 0x0d77,
> +		 0x9a, 0x2c, 0x31, 0x18, 0xca, 0x41, 0xa6, 0x6e);
> +
>   static struct context context;
>   static struct efi_event *event;
> +static struct efi_event *test_uninstall_event;
> +static void *test_uninstall_key;
>
>   /*
>    * Notification function, increments the notification count if parameter
> @@ -63,6 +69,17 @@ static void EFIAPI notify(struct efi_event *event, void *context)
>   	}
>   }
>
> +/*
> + * Notification function, does nothing and is used to test UninstallProtocol
> + *
> + * @event	notified event
> + * @context	pointer to the notification count
> + */
> +static void EFIAPI test_uninstall_notify(struct efi_event *event, void *context)
> +{
> +	return;
> +}
> +
>   /*
>    * Setup unit test.
>    *
> @@ -91,6 +108,21 @@ static int setup(const efi_handle_t img_handle,
>   		return EFI_ST_FAILURE;
>   	}
>
> +	ret = boottime->create_event(EVT_NOTIFY_SIGNAL,

You could use the existing notify function.

> +				     TPL_CALLBACK, test_uninstall_notify, NULL,
> +				     &test_uninstall_event);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("could not create event\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	ret = boottime->register_protocol_notify(&guid3, test_uninstall_event,
> +						 &test_uninstall_key);

I would prefer to use the same guid1 protocol. Deleting one handle
should not remove the other handle from the handle queue if both have
the same protocol installed.

> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("could not register event\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
>   	return EFI_ST_SUCCESS;
>   }
>
> @@ -121,8 +153,10 @@ static int teardown(void)
>   static int execute(void)
>   {
>   	efi_status_t ret;
> -	efi_handle_t handle1 = NULL, handle2 = NULL;
> -	struct interface interface1, interface2;
> +	efi_handle_t handle1 = NULL, handle2 = NULL, handle3 = NULL;
> +	struct interface interface1, interface2, interface3;
> +	efi_uintn_t handle_count;
> +	efi_handle_t *handles;
>
>   	ret = boottime->install_protocol_interface(&handle1, &guid1,
>   						   EFI_NATIVE_INTERFACE,
> @@ -224,6 +258,29 @@ static int execute(void)
>   		return EFI_ST_FAILURE;
>   	}
>
> +	ret = boottime->install_protocol_interface(&handle3, &guid3,
> +						   EFI_NATIVE_INTERFACE,
> +						   &interface3);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("could not install interface\n");
> +		return EFI_ST_FAILURE;
> +	}

Please, check the number of invocations of the notify function.

> +
> +	ret = boottime->uninstall_multiple_protocol_interfaces
> +			(handle3, &guid3, &interface3, NULL);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("UninstallMultipleProtocolInterfaces failed\n");
> +		return EFI_ST_FAILURE;
> +	}

And here again. We don't expect UninstallMultipleProtocolInterfaces() to
trigger the event.

Best regards

Heinrich

> +
> +	ret = boottime->locate_handle_buffer(BY_REGISTER_NOTIFY, NULL,
> +					     test_uninstall_key,
> +					     &handle_count, &handles);
> +	if (ret != EFI_NOT_FOUND) {
> +		efi_st_error("UninstallMultipleProtocolInterfaces failed to delete event handles\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
>   	return EFI_ST_SUCCESS;
>   }
>
> --
> 2.39.2
>
Ilias Apalodimas June 1, 2023, 7:32 p.m. UTC | #2 
Hi Heinrich, 

[...]
> > + * @event	notified event
> > + * @context	pointer to the notification count
> > + */
> > +static void EFIAPI test_uninstall_notify(struct efi_event *event, void *context)
> > +{
> > +	return;
> > +}
> > +
> >   /*
> >    * Setup unit test.
> >    *
> > @@ -91,6 +108,21 @@ static int setup(const efi_handle_t img_handle,
> >   		return EFI_ST_FAILURE;
> >   	}
> > 
> > +	ret = boottime->create_event(EVT_NOTIFY_SIGNAL,
> 
> You could use the existing notify function.
> 

The reason I created a new one is that the current one calls
locate_handle_buffer().  See below 

> > +				     TPL_CALLBACK, test_uninstall_notify, NULL,
> > +				     &test_uninstall_event);
> > +	if (ret != EFI_SUCCESS) {
> > +		efi_st_error("could not create event\n");
> > +		return EFI_ST_FAILURE;
> > +	}
> > +
> > +	ret = boottime->register_protocol_notify(&guid3, test_uninstall_event,
> > +						 &test_uninstall_key);
> 
> I would prefer to use the same guid1 protocol. Deleting one handle
> should not remove the other handle from the handle queue if both have
> the same protocol installed.
> 

Unless I am missing something, that would not work. The notifier function
will call locate_handle_buffer and will retrieve all handles.  As a result
the subsequent call will return EFI_NOT_FOUND.  You've already sent a ptch
covering that, the purpose of this one is check that the handler will be
clearer from the events list if the protocol gets uninstalled. 
We could use guid2 but I do prefer habing distinct protocols for every
test.

> > +	if (ret != EFI_SUCCESS) {
> > +		efi_st_error("could not register event\n");
> > +		return EFI_ST_FAILURE;
> > +	}
> > +
> >   	return EFI_ST_SUCCESS;
> >   }
> > 
> > @@ -121,8 +153,10 @@ static int teardown(void)
> >   static int execute(void)
> >   {
> >   	efi_status_t ret;
> > -	efi_handle_t handle1 = NULL, handle2 = NULL;
> > -	struct interface interface1, interface2;
> > +	efi_handle_t handle1 = NULL, handle2 = NULL, handle3 = NULL;
> > +	struct interface interface1, interface2, interface3;
> > +	efi_uintn_t handle_count;
> > +	efi_handle_t *handles;
> > 
> >   	ret = boottime->install_protocol_interface(&handle1, &guid1,
> >   						   EFI_NATIVE_INTERFACE,
> > @@ -224,6 +258,29 @@ static int execute(void)
> >   		return EFI_ST_FAILURE;
> >   	}
> > 
> > +	ret = boottime->install_protocol_interface(&handle3, &guid3,
> > +						   EFI_NATIVE_INTERFACE,
> > +						   &interface3);
> > +	if (ret != EFI_SUCCESS) {
> > +		efi_st_error("could not install interface\n");
> > +		return EFI_ST_FAILURE;
> > +	}
> 
> Please, check the number of invocations of the notify function.
> 

I can add that, but the tests already check that case in detail. 

> > +
> > +	ret = boottime->uninstall_multiple_protocol_interfaces
> > +			(handle3, &guid3, &interface3, NULL);
> > +	if (ret != EFI_SUCCESS) {
> > +		efi_st_error("UninstallMultipleProtocolInterfaces failed\n");
> > +		return EFI_ST_FAILURE;
> > +	}
> 
> And here again. We don't expect UninstallMultipleProtocolInterfaces() to
> trigger the event.
> 

ditto

Thanks
/Ilias
> Best regards
> 
> Heinrich
> 
> > +
> > +	ret = boottime->locate_handle_buffer(BY_REGISTER_NOTIFY, NULL,
> > +					     test_uninstall_key,
> > +					     &handle_count, &handles);
> > +	if (ret != EFI_NOT_FOUND) {
> > +		efi_st_error("UninstallMultipleProtocolInterfaces failed to delete event handles\n");
> > +		return EFI_ST_FAILURE;
> > +	}
> > +
> >   	return EFI_ST_SUCCESS;
> >   }
> > 
> > --
> > 2.39.2
> > 
>
Heinrich Schuchardt June 1, 2023, 9:59 p.m. UTC | #3 
Am 1. Juni 2023 21:32:04 MESZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>:
>Hi Heinrich, 
>
>[...]
>> > + * @event	notified event
>> > + * @context	pointer to the notification count
>> > + */
>> > +static void EFIAPI test_uninstall_notify(struct efi_event *event, void *context)
>> > +{
>> > +	return;
>> > +}
>> > +
>> >   /*
>> >    * Setup unit test.
>> >    *
>> > @@ -91,6 +108,21 @@ static int setup(const efi_handle_t img_handle,
>> >   		return EFI_ST_FAILURE;
>> >   	}
>> > 
>> > +	ret = boottime->create_event(EVT_NOTIFY_SIGNAL,
>> 
>> You could use the existing notify function.
>> 
>
>The reason I created a new one is that the current one calls
>locate_handle_buffer().  See below 
>
>> > +				     TPL_CALLBACK, test_uninstall_notify, NULL,
>> > +				     &test_uninstall_event);
>> > +	if (ret != EFI_SUCCESS) {
>> > +		efi_st_error("could not create event\n");
>> > +		return EFI_ST_FAILURE;
>> > +	}
>> > +
>> > +	ret = boottime->register_protocol_notify(&guid3, test_uninstall_event,
>> > +						 &test_uninstall_key);
>> 
>> I would prefer to use the same guid1 protocol. Deleting one handle
>> should not remove the other handle from the handle queue if both have
>> the same protocol installed.
>> 
>
>Unless I am missing something, that would not work. The notifier function
>will call locate_handle_buffer and will retrieve all handles.  As a result

This would not match the specification.

LocateHandle(ByRegistrationKey) must only retrieve at maximum a single handle per call irrespective of the queue size. LocateHandleBuffer() should do the same.

Regards

Heinrich 

>the subsequent call will return EFI_NOT_FOUND.  You've already sent a ptch
>covering that, the purpose of this one is check that the handler will be
>clearer from the events list if the protocol gets uninstalled. 
>We could use guid2 but I do prefer habing distinct protocols for every
>test.
>
>> > +	if (ret != EFI_SUCCESS) {
>> > +		efi_st_error("could not register event\n");
>> > +		return EFI_ST_FAILURE;
>> > +	}
>> > +
>> >   	return EFI_ST_SUCCESS;
>> >   }
>> > 
>> > @@ -121,8 +153,10 @@ static int teardown(void)
>> >   static int execute(void)
>> >   {
>> >   	efi_status_t ret;
>> > -	efi_handle_t handle1 = NULL, handle2 = NULL;
>> > -	struct interface interface1, interface2;
>> > +	efi_handle_t handle1 = NULL, handle2 = NULL, handle3 = NULL;
>> > +	struct interface interface1, interface2, interface3;
>> > +	efi_uintn_t handle_count;
>> > +	efi_handle_t *handles;
>> > 
>> >   	ret = boottime->install_protocol_interface(&handle1, &guid1,
>> >   						   EFI_NATIVE_INTERFACE,
>> > @@ -224,6 +258,29 @@ static int execute(void)
>> >   		return EFI_ST_FAILURE;
>> >   	}
>> > 
>> > +	ret = boottime->install_protocol_interface(&handle3, &guid3,
>> > +						   EFI_NATIVE_INTERFACE,
>> > +						   &interface3);
>> > +	if (ret != EFI_SUCCESS) {
>> > +		efi_st_error("could not install interface\n");
>> > +		return EFI_ST_FAILURE;
>> > +	}
>> 
>> Please, check the number of invocations of the notify function.
>> 
>
>I can add that, but the tests already check that case in detail. 
>
>> > +
>> > +	ret = boottime->uninstall_multiple_protocol_interfaces
>> > +			(handle3, &guid3, &interface3, NULL);
>> > +	if (ret != EFI_SUCCESS) {
>> > +		efi_st_error("UninstallMultipleProtocolInterfaces failed\n");
>> > +		return EFI_ST_FAILURE;
>> > +	}
>> 
>> And here again. We don't expect UninstallMultipleProtocolInterfaces() to
>> trigger the event.
>> 
>
>ditto
>
>Thanks
>/Ilias
>> Best regards
>> 
>> Heinrich
>> 
>> > +
>> > +	ret = boottime->locate_handle_buffer(BY_REGISTER_NOTIFY, NULL,
>> > +					     test_uninstall_key,
>> > +					     &handle_count, &handles);
>> > +	if (ret != EFI_NOT_FOUND) {
>> > +		efi_st_error("UninstallMultipleProtocolInterfaces failed to delete event handles\n");
>> > +		return EFI_ST_FAILURE;
>> > +	}
>> > +
>> >   	return EFI_ST_SUCCESS;
>> >   }
>> > 
>> > --
>> > 2.39.2
>> > 
>>
diff mbox series

Patch

diff --git a/lib/efi_selftest/efi_selftest_register_notify.c b/lib/efi_selftest/efi_selftest_register_notify.c
index ad763dd6cb8b..d133c7c50b71 100644
--- a/lib/efi_selftest/efi_selftest_register_notify.c
+++ b/lib/efi_selftest/efi_selftest_register_notify.c
@@ -33,8 +33,14 @@  static efi_guid_t guid1 =
 static efi_guid_t guid2 =
 	EFI_GUID(0xf909f2bb, 0x90a8, 0x0d77,
 		 0x94, 0x0c, 0x3e, 0xa8, 0xea, 0x38, 0xd6, 0x6f);
+static efi_guid_t guid3 =
+	EFI_GUID(0xfa09f2cb, 0x70a8, 0x0d77,
+		 0x9a, 0x2c, 0x31, 0x18, 0xca, 0x41, 0xa6, 0x6e);
+
 static struct context context;
 static struct efi_event *event;
+static struct efi_event *test_uninstall_event;
+static void *test_uninstall_key;

 /*
  * Notification function, increments the notification count if parameter
@@ -63,6 +69,17 @@  static void EFIAPI notify(struct efi_event *event, void *context)
 	}
 }

+/*
+ * Notification function, does nothing and is used to test UninstallProtocol
+ *
+ * @event	notified event
+ * @context	pointer to the notification count
+ */
+static void EFIAPI test_uninstall_notify(struct efi_event *event, void *context)
+{
+	return;
+}
+
 /*
  * Setup unit test.
  *
@@ -91,6 +108,21 @@  static int setup(const efi_handle_t img_handle,
 		return EFI_ST_FAILURE;
 	}

+	ret = boottime->create_event(EVT_NOTIFY_SIGNAL,
+				     TPL_CALLBACK, test_uninstall_notify, NULL,
+				     &test_uninstall_event);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("could not create event\n");
+		return EFI_ST_FAILURE;
+	}
+
+	ret = boottime->register_protocol_notify(&guid3, test_uninstall_event,
+						 &test_uninstall_key);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("could not register event\n");
+		return EFI_ST_FAILURE;
+	}
+
 	return EFI_ST_SUCCESS;
 }

@@ -121,8 +153,10 @@  static int teardown(void)
 static int execute(void)
 {
 	efi_status_t ret;
-	efi_handle_t handle1 = NULL, handle2 = NULL;
-	struct interface interface1, interface2;
+	efi_handle_t handle1 = NULL, handle2 = NULL, handle3 = NULL;
+	struct interface interface1, interface2, interface3;
+	efi_uintn_t handle_count;
+	efi_handle_t *handles;

 	ret = boottime->install_protocol_interface(&handle1, &guid1,
 						   EFI_NATIVE_INTERFACE,
@@ -224,6 +258,29 @@  static int execute(void)
 		return EFI_ST_FAILURE;
 	}

+	ret = boottime->install_protocol_interface(&handle3, &guid3,
+						   EFI_NATIVE_INTERFACE,
+						   &interface3);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("could not install interface\n");
+		return EFI_ST_FAILURE;
+	}
+
+	ret = boottime->uninstall_multiple_protocol_interfaces
+			(handle3, &guid3, &interface3, NULL);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("UninstallMultipleProtocolInterfaces failed\n");
+		return EFI_ST_FAILURE;
+	}
+
+	ret = boottime->locate_handle_buffer(BY_REGISTER_NOTIFY, NULL,
+					     test_uninstall_key,
+					     &handle_count, &handles);
+	if (ret != EFI_NOT_FOUND) {
+		efi_st_error("UninstallMultipleProtocolInterfaces failed to delete event handles\n");
+		return EFI_ST_FAILURE;
+	}
+
 	return EFI_ST_SUCCESS;
 }