| Message ID | 20230824115500.2388636-1-ilias.apalodimas@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi_loader: delete handle from events when a protocol is uninstalled | expand |
On 24.08.23 13:55, Ilias Apalodimas 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> > --- > lib/efi_loader/efi_boottime.c | 80 ++++++++++++++++++++++++----------- > 1 file changed, 56 insertions(+), 24 deletions(-) > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > index 0e89c8505b11..96950790ba2a 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, *next; > + > + if (!list_empty(&handle->protocols)) > + return EFI_ACCESS_DENIED; > + /* The handle is about to be freed. Remove it from events */ > + list_for_each_entry_safe(item, next, &efi_register_notify_events, link) { You are not deleting from efi_register_notify_events. Why do you use the safe version of the iterator? > + 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,25 @@ 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*); > + 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; Is your idea that if 5 protocols are to be removed but only the first 3 are installed your want to reinstall the first 3? Can we have some code comment describing what the function should do? In 5 years nobody will remember why the code was implemented this way. Best regards Heinrich > 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 +3744,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; >
Hi Henrich > > > > +/** > > + * 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, *next; > > + > > + if (!list_empty(&handle->protocols)) > > + return EFI_ACCESS_DENIED; > > + /* The handle is about to be freed. Remove it from events */ > > + list_for_each_entry_safe(item, next, &efi_register_notify_events, link) { > > You are not deleting from efi_register_notify_events. I am not sure I understand this. I am not trying to delete the event here, but the handle on which the notified protocol was installed. > Why do you use the safe version of the iterator? No particular reason, I just tend to prefer it over the unsafe one. The latter is perfectly fine here. [...] > > > > - /* If the last protocol has been removed, delete the handle. */ > > - if (list_empty(&handle->protocols)) { > > > size_t i = 0; > > @@ -2795,25 +2827,25 @@ 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*); > > + 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; > > Is your idea that if 5 protocols are to be removed but only the first 3 > are installed your want to reinstall the first 3? Not only that. Image you install 2 protocols and then someone tries to uninstall 3. If the invalid guid/protocol is at the beginning or the middle of the list you'll be fine. The current code will reinstall the previously removed protocols. However, if the invalid protocol is last, you will have removed 2 valid protocols and the handle will be empty. In that case, we will delete the handle before trying to reinstall the uninstalled protocols. > > Can we have some code comment describing what the function should do? In > 5 years nobody will remember why the code was implemented this way. > Of course [...] Cheers /Ilias
On 24.08.23 15:06, Ilias Apalodimas wrote: > Hi Henrich > > >>> >>> +/** >>> + * 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, *next; >>> + >>> + if (!list_empty(&handle->protocols)) >>> + return EFI_ACCESS_DENIED; >>> + /* The handle is about to be freed. Remove it from events */ >>> + list_for_each_entry_safe(item, next, &efi_register_notify_events, link) { >> >> You are not deleting from efi_register_notify_events. > > I am not sure I understand this. I am not trying to delete the event > here, but the handle on which the notified protocol was installed. > >> Why do you use the safe version of the iterator? > > No particular reason, I just tend to prefer it over the unsafe one. > The latter is perfectly fine here. The save version is only needed if you delete the current item or insert in the current position. > > [...] > >>> >>> - /* If the last protocol has been removed, delete the handle. */ >>> - if (list_empty(&handle->protocols)) { >> >>> size_t i = 0; >>> @@ -2795,25 +2827,25 @@ 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*); >>> + 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; >> >> Is your idea that if 5 protocols are to be removed but only the first 3 >> are installed your want to reinstall the first 3? > > Not only that. Image you install 2 protocols and then someone tries to > uninstall 3. If the invalid guid/protocol is at the beginning or the > middle of the list you'll be fine. The current code will reinstall > the previously removed protocols. > However, if the invalid protocol is last, you will have removed 2 > valid protocols and the handle will be empty. In that case, we will > delete the handle before trying to reinstall the uninstalled > protocols. This is just what I meant. Best regards Heirnich > >> >> Can we have some code comment describing what the function should do? In >> 5 years nobody will remember why the code was implemented this way. >> > > Of course > > [...] > > Cheers > /Ilias
On Thu, 24 Aug 2023 at 17:02, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 24.08.23 15:06, Ilias Apalodimas wrote: > > Hi Henrich > > > > > >>> > >>> +/** > >>> + * 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, *next; > >>> + > >>> + if (!list_empty(&handle->protocols)) > >>> + return EFI_ACCESS_DENIED; > >>> + /* The handle is about to be freed. Remove it from events */ > >>> + list_for_each_entry_safe(item, next, &efi_register_notify_events, link) { > >> > >> You are not deleting from efi_register_notify_events. > > > > I am not sure I understand this. I am not trying to delete the event > > here, but the handle on which the notified protocol was installed. > > > >> Why do you use the safe version of the iterator? > > > > No particular reason, I just tend to prefer it over the unsafe one. > > The latter is perfectly fine here. > > The save version is only needed if you delete the current item or insert > in the current position. Ok, I'll change that on v2 > > > > > [...] > > > >>> > >>> - /* If the last protocol has been removed, delete the handle. */ > >>> - if (list_empty(&handle->protocols)) { > >> > >>> size_t i = 0; > >>> @@ -2795,25 +2827,25 @@ 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*); > >>> + 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; > >> > >> Is your idea that if 5 protocols are to be removed but only the first 3 > >> are installed your want to reinstall the first 3? > > > > Not only that. Image you install 2 protocols and then someone tries to > > uninstall 3. If the invalid guid/protocol is at the beginning or the > > middle of the list you'll be fine. The current code will reinstall > > the previously removed protocols. > > However, if the invalid protocol is last, you will have removed 2 > > valid protocols and the handle will be empty. In that case, we will > > delete the handle before trying to reinstall the uninstalled > > protocols. > > This is just what I meant. Ok, I'll just adjust my explanation and add it as a comment in v2 Thanks /Ilias > > Best regards > > Heirnich > > > > >> > >> Can we have some code comment describing what the function should do? In > >> 5 years nobody will remember why the code was implemented this way. > >> > > > > Of course > > > > [...] > > > > Cheers > > /Ilias >
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 0e89c8505b11..96950790ba2a 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, *next; + + if (!list_empty(&handle->protocols)) + return EFI_ACCESS_DENIED; + /* The handle is about to be freed. Remove it from events */ + list_for_each_entry_safe(item, next, &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,25 @@ 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*); + 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 +3744,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;
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> --- lib/efi_loader/efi_boottime.c | 80 ++++++++++++++++++++++++----------- 1 file changed, 56 insertions(+), 24 deletions(-)