[PATCH 1/2] efi_loader: delete handle from events when a protocol is uninstalled

[Heinrich Schuchardt]

On 6/1/23 14:06, 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
>
> It's worth noting that all our variants of efi_uninstall_protocol_*()
> and uninstall_multiple_protocol_*() end up calling
> efi_uninstall_protocol().  We could add the cleanup in that function
> only,  however efi_reinstall_protocol_interface() expects the handle not
> to be removed.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> ---
>   lib/efi_loader/efi_boottime.c | 45 +++++++++++++++++++++++++----------
>   1 file changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index d5065f296aee..450524ddca2f 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -197,6 +197,35 @@ static bool efi_event_is_queued(struct efi_event *event)
>   	return !!event->queue_link.next;
>   }
>
> +/**
> + * efi_handle_cleanup() - Clean the deleted handle from the various lists
> + * @handle: handle to remove
> + * @force:  force removal even if protocols are still installed on the handle
> + *
> + * Return: status code
> + */
> +static void efi_handle_cleanup(efi_handle_t handle, bool force)
> +{
> +	struct efi_register_notify_event *item, *next;
> +
> +	if (!list_empty(&handle->protocols) || force)

I guess the parameter force can be dropped from the interface.

> +		return;

Please, return a failure code.

> +	/* 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);
> +}
> +
>   /**
>    * efi_process_event_queue() - process event queue
>    */
> @@ -627,8 +656,7 @@ void efi_delete_handle(efi_handle_t handle)
>   		return;
>   	}

It would be safer to move the checks if a protocol interface can be
removed from efi_uninstall_protocol() to efi_remove_protocol(). That way
we are sure that no driver has attached to the protocols that we try to
remove.

>
> -	list_del(&handle->link);
> -	free(handle);
> +	efi_handle_cleanup(handle, true);

I don't think we should use a special treatment here. If the protocols
cannot be deleted, because a driver has attached we should not delete
the handle.

The function should return a status code. Then efi_disk_delete_raw() can
return an error for EVT_DM_PRE_REMOVE and stop the device from being
deleted.

Best regards

Heinrich

>   }
>
>   /**
> @@ -1396,11 +1424,8 @@ static efi_status_t EFIAPI efi_uninstall_protocol_interface
>   	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);
> -	}
> +	/* Check if we have to purge the handle from various lists */
> +	efi_handle_cleanup(handle, false);
>   out:
>   	return EFI_EXIT(ret);
>   }
> @@ -2777,11 +2802,7 @@ efi_uninstall_multiple_protocol_interfaces_int(efi_handle_t handle,
>   		i++;
>   	}
>   	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);
> -		}
> +		efi_handle_cleanup(handle, false);
>   		goto out;
>   	}
>