diff mbox series

[v2] efi_loader: delete handle from events when a protocol is uninstalled

Message ID 20230824142109.2401528-1-ilias.apalodimas@linaro.org
State Accepted
Commit cc889bd0754e50a3cd50e8ed3094388480fbec86
Headers show
Series [v2] efi_loader: delete handle from events when a protocol is uninstalled | expand

Commit Message

Ilias Apalodimas Aug. 24, 2023, 2:21 p.m. UTC
When a notification event is registered for a protocol the handle of the
protocol is added in our event notification list.  When all the protocols
of the handle are uninstalled we delete the handle but we do not remove
it from the event notification list.

Clean up the protocol removal functions and add a wrapper which
- Removes the to-be deleted handle from any lists it participates
- Remove the handle if no more protocols are present

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Changes since v1:
- use the non safe list traversing macros on efi_register_notify_events since
  we don't delete anythig from that list
- Add a comment explaining the code that preserves the handle in case of a
  removal failure

 lib/efi_loader/efi_boottime.c | 89 +++++++++++++++++++++++++----------
 1 file changed, 65 insertions(+), 24 deletions(-)

--
2.37.2

Comments

Simon Glass Aug. 24, 2023, 2:41 p.m. UTC | #1
Hi Ilias,

On Thu, 24 Aug 2023 at 08:21, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> When a notification event is registered for a protocol the handle of the
> protocol is added in our event notification list.  When all the protocols
> of the handle are uninstalled we delete the handle but we do not remove
> it from the event notification list.
>
> Clean up the protocol removal functions and add a wrapper which
> - Removes the to-be deleted handle from any lists it participates
> - Remove the handle if no more protocols are present
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> Changes since v1:
> - use the non safe list traversing macros on efi_register_notify_events since
>   we don't delete anythig from that list
> - Add a comment explaining the code that preserves the handle in case of a
>   removal failure
>
>  lib/efi_loader/efi_boottime.c | 89 +++++++++++++++++++++++++----------
>  1 file changed, 65 insertions(+), 24 deletions(-)

Is there a test for this behaviour?

Regards,
Simon
Ilias Apalodimas Aug. 24, 2023, 2:47 p.m. UTC | #2
Hi Simon,

On Thu, 24 Aug 2023 at 17:41, Simon Glass <sjg@google.com> wrote:
>
> Hi Ilias,
>
> On Thu, 24 Aug 2023 at 08:21, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > When a notification event is registered for a protocol the handle of the
> > protocol is added in our event notification list.  When all the protocols
> > of the handle are uninstalled we delete the handle but we do not remove
> > it from the event notification list.
> >
> > Clean up the protocol removal functions and add a wrapper which
> > - Removes the to-be deleted handle from any lists it participates
> > - Remove the handle if no more protocols are present
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > Changes since v1:
> > - use the non safe list traversing macros on efi_register_notify_events since
> >   we don't delete anythig from that list
> > - Add a comment explaining the code that preserves the handle in case of a
> >   removal failure
> >
> >  lib/efi_loader/efi_boottime.c | 89 +++++++++++++++++++++++++----------
> >  1 file changed, 65 insertions(+), 24 deletions(-)
>
> Is there a test for this behaviour?

Not in -master.  I've sent it in the past [0].  But instead of merging
that series, we decided with Heinrich that a bigger cleanup was
needed.
So we had to do a bigger cleanup of protocol handling [1] [2]

[1] https://lore.kernel.org/u-boot/20230620061932.113292-1-ilias.apalodimas@linaro.org/
[2] https://lore.kernel.org/u-boot/a222ec75-32ef-9dce-0855-afde2702c618@gmx.de/

Once the last part gets merged, I'll rebase the test and send it over

Thanks
/Ilias
>
> Regards,
> Simon
Ilias Apalodimas Aug. 24, 2023, 2:49 p.m. UTC | #3
On Thu, 24 Aug 2023 at 17:47, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Thu, 24 Aug 2023 at 17:41, Simon Glass <sjg@google.com> wrote:
> >
> > Hi Ilias,
> >
> > On Thu, 24 Aug 2023 at 08:21, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > When a notification event is registered for a protocol the handle of the
> > > protocol is added in our event notification list.  When all the protocols
> > > of the handle are uninstalled we delete the handle but we do not remove
> > > it from the event notification list.
> > >
> > > Clean up the protocol removal functions and add a wrapper which
> > > - Removes the to-be deleted handle from any lists it participates
> > > - Remove the handle if no more protocols are present
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > > Changes since v1:
> > > - use the non safe list traversing macros on efi_register_notify_events since
> > >   we don't delete anythig from that list
> > > - Add a comment explaining the code that preserves the handle in case of a
> > >   removal failure
> > >
> > >  lib/efi_loader/efi_boottime.c | 89 +++++++++++++++++++++++++----------
> > >  1 file changed, 65 insertions(+), 24 deletions(-)
> >
> > Is there a test for this behaviour?
>
> Not in -master.  I've sent it in the past [0].  But instead of merging
> that series, we decided with Heinrich that a bigger cleanup was
> needed.
> So we had to do a bigger cleanup of protocol handling [1] [2]
>

Forgot the first link... apologize for the noise
[0] https://lore.kernel.org/u-boot/20230601120625.1843555-2-ilias.apalodimas@linaro.org/

> [1] https://lore.kernel.org/u-boot/20230620061932.113292-1-ilias.apalodimas@linaro.org/
> [2] https://lore.kernel.org/u-boot/a222ec75-32ef-9dce-0855-afde2702c618@gmx.de/
>
> Once the last part gets merged, I'll rebase the test and send it over
>
> Thanks
> /Ilias
> >
> > Regards,
> > Simon
Heinrich Schuchardt Aug. 24, 2023, 3:25 p.m. UTC | #4
On 24.08.23 16:49, Ilias Apalodimas wrote:
> On Thu, 24 Aug 2023 at 17:47, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
>>
>> Hi Simon,
>>
>> On Thu, 24 Aug 2023 at 17:41, Simon Glass <sjg@google.com> wrote:
>>>
>>> Hi Ilias,
>>>
>>> On Thu, 24 Aug 2023 at 08:21, Ilias Apalodimas
>>> <ilias.apalodimas@linaro.org> wrote:
>>>>
>>>> When a notification event is registered for a protocol the handle of the
>>>> protocol is added in our event notification list.  When all the protocols
>>>> of the handle are uninstalled we delete the handle but we do not remove
>>>> it from the event notification list.
>>>>
>>>> Clean up the protocol removal functions and add a wrapper which
>>>> - Removes the to-be deleted handle from any lists it participates
>>>> - Remove the handle if no more protocols are present
>>>>
>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>> ---
>>>> Changes since v1:
>>>> - use the non safe list traversing macros on efi_register_notify_events since
>>>>    we don't delete anythig from that list
>>>> - Add a comment explaining the code that preserves the handle in case of a
>>>>    removal failure
>>>>
>>>>   lib/efi_loader/efi_boottime.c | 89 +++++++++++++++++++++++++----------
>>>>   1 file changed, 65 insertions(+), 24 deletions(-)
>>>
>>> Is there a test for this behaviour?
>>
>> Not in -master.  I've sent it in the past [0].  But instead of merging
>> that series, we decided with Heinrich that a bigger cleanup was
>> needed.
>> So we had to do a bigger cleanup of protocol handling [1] [2]
>>
>
> Forgot the first link... apologize for the noise
> [0] https://lore.kernel.org/u-boot/20230601120625.1843555-2-ilias.apalodimas@linaro.org/
>
>> [1] https://lore.kernel.org/u-boot/20230620061932.113292-1-ilias.apalodimas@linaro.org/

This series is already merged.

>> [2] https://lore.kernel.org/u-boot/a222ec75-32ef-9dce-0855-afde2702c618@gmx.de/
>>
>> Once the last part gets merged, I'll rebase the test and send it over

This patch is also merged into origin main.

Best regards

Heinrich

>>
>> Thanks
>> /Ilias
>>>
>>> Regards,
>>> Simon
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 0e89c8505b11..0b7579cb5af1 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -61,7 +61,7 @@  static volatile gd_t *efi_gd, *app_gd;

 static efi_status_t efi_uninstall_protocol
 			(efi_handle_t handle, const efi_guid_t *protocol,
-			 void *protocol_interface);
+			 void *protocol_interface, bool preserve);

 /* 1 if inside U-Boot code, 0 if inside EFI payload code */
 static int entry_count = 1;
@@ -207,6 +207,36 @@  static bool efi_event_is_queued(struct efi_event *event)
 	return !!event->queue_link.next;
 }

+/**
+ * efi_purge_handle() - Clean the deleted handle from the various lists
+ * @handle: handle to remove
+ *
+ * Return: status code
+ */
+static efi_status_t efi_purge_handle(efi_handle_t handle)
+{
+	struct efi_register_notify_event *item;
+
+	if (!list_empty(&handle->protocols))
+		return EFI_ACCESS_DENIED;
+	/* The handle is about to be freed. Remove it from events */
+	list_for_each_entry(item, &efi_register_notify_events, link) {
+		struct efi_protocol_notification *hitem, *hnext;
+
+		list_for_each_entry_safe(hitem, hnext, &item->handles, link) {
+			if (handle == hitem->handle) {
+				list_del(&hitem->link);
+				free(hitem);
+			}
+		}
+	}
+	/* The last protocol has been removed, delete the handle. */
+	list_del(&handle->link);
+	free(handle);
+
+	return EFI_SUCCESS;
+}
+
 /**
  * efi_process_event_queue() - process event queue
  */
@@ -615,7 +645,7 @@  static efi_status_t efi_remove_all_protocols(const efi_handle_t handle)
 		efi_status_t ret;

 		ret = efi_uninstall_protocol(handle, &protocol->guid,
-					     protocol->protocol_interface);
+					     protocol->protocol_interface, true);
 		if (ret != EFI_SUCCESS)
 			return ret;
 	}
@@ -639,10 +669,7 @@  efi_status_t efi_delete_handle(efi_handle_t handle)
 		return ret;
 	}

-	list_del(&handle->link);
-	free(handle);
-
-	return ret;
+	return efi_purge_handle(handle);
 }

 /**
@@ -1356,6 +1383,8 @@  reconnect:
  * @handle:             handle from which the protocol shall be removed
  * @protocol:           GUID of the protocol to be removed
  * @protocol_interface: interface to be removed
+ * @preserve:		preserve or delete the handle and remove it from any
+ *			list it participates if no protocols remain
  *
  * This function DOES NOT delete a handle without installed protocol.
  *
@@ -1363,7 +1392,7 @@  reconnect:
  */
 static efi_status_t efi_uninstall_protocol
 			(efi_handle_t handle, const efi_guid_t *protocol,
-			 void *protocol_interface)
+			 void *protocol_interface, bool preserve)
 {
 	struct efi_handler *handler;
 	struct efi_open_protocol_info_item *item;
@@ -1397,6 +1426,14 @@  static efi_status_t efi_uninstall_protocol
 		goto out;
 	}
 	r = efi_remove_protocol(handle, protocol, protocol_interface);
+	if (r != EFI_SUCCESS)
+		return r;
+	/*
+	 * We don't care about the return value here since the
+	 * handle might have more protocols installed
+	 */
+	if (!preserve)
+		efi_purge_handle(handle);
 out:
 	return r;
 }
@@ -1422,15 +1459,10 @@  static efi_status_t EFIAPI efi_uninstall_protocol_interface

 	EFI_ENTRY("%p, %pUs, %p", handle, protocol, protocol_interface);

-	ret = efi_uninstall_protocol(handle, protocol, protocol_interface);
+	ret = efi_uninstall_protocol(handle, protocol, protocol_interface, false);
 	if (ret != EFI_SUCCESS)
 		goto out;

-	/* If the last protocol has been removed, delete the handle. */
-	if (list_empty(&handle->protocols)) {
-		list_del(&handle->link);
-		free(handle);
-	}
 out:
 	return EFI_EXIT(ret);
 }
@@ -2785,7 +2817,7 @@  static efi_status_t EFIAPI
 efi_uninstall_multiple_protocol_interfaces_int(efi_handle_t handle,
 					       efi_va_list argptr)
 {
-	const efi_guid_t *protocol;
+	const efi_guid_t *protocol, *next_protocol;
 	void *protocol_interface;
 	efi_status_t ret = EFI_SUCCESS;
 	size_t i = 0;
@@ -2795,25 +2827,34 @@  efi_uninstall_multiple_protocol_interfaces_int(efi_handle_t handle,
 		return EFI_INVALID_PARAMETER;

 	efi_va_copy(argptr_copy, argptr);
+	protocol = efi_va_arg(argptr, efi_guid_t*);
 	for (;;) {
-		protocol = efi_va_arg(argptr, efi_guid_t*);
+		/*
+		 * If efi_uninstall_protocol() fails we need to be able to
+		 * reinstall the previously uninstalled protocols on the same
+		 * handle.
+		 * Instead of calling efi_uninstall_protocol(...,..., false)
+		 * and potentially removing the handle, only allow the handle
+		 * removal on the last protocol that we requested to uninstall.
+		 * That way we can preserve  the handle in case the latter fails
+		 */
+		bool preserve = true;
+
 		if (!protocol)
 			break;
 		protocol_interface = efi_va_arg(argptr, void*);
+		next_protocol = efi_va_arg(argptr, efi_guid_t*);
+		if (!next_protocol)
+			preserve = false;
 		ret = efi_uninstall_protocol(handle, protocol,
-					     protocol_interface);
+					     protocol_interface, preserve);
 		if (ret != EFI_SUCCESS)
 			break;
 		i++;
+		protocol = next_protocol;
 	}
-	if (ret == EFI_SUCCESS) {
-		/* If the last protocol has been removed, delete the handle. */
-		if (list_empty(&handle->protocols)) {
-			list_del(&handle->link);
-			free(handle);
-		}
+	if (ret == EFI_SUCCESS)
 		goto out;
-	}

 	/* If an error occurred undo all changes. */
 	for (; i; --i) {
@@ -3712,7 +3753,7 @@  static efi_status_t EFIAPI efi_reinstall_protocol_interface(
 		  new_interface);

 	/* Uninstall protocol but do not delete handle */
-	ret = efi_uninstall_protocol(handle, protocol, old_interface);
+	ret = efi_uninstall_protocol(handle, protocol, old_interface, true);
 	if (ret != EFI_SUCCESS)
 		goto out;