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

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@linaro.org>
---
 lib/efi_loader/efi_boottime.c | 45 +++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 12 deletions(-)

Heinrich

[...]

>
> 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)

This is obviously wrong, I'll send a v2 once you are fine with the rest. Sorry!


[...]

/Ilias

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@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;
>   	}
>

[...]
> > 
> > +/**
> > + * 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.
> 

'force' is here for a reason(see below), although the if is wrong.

The correct one is obviously
if (!list_empty(&handle->protocols) && !force)
    ....

> > +		return;
> 
> Please, return a failure code.

I am not sure why you want that, but I'll have a closer look 

> 
> > +	/* 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.
> 

Yes it would, in fact I tried this on my initial patches and I mention it
on the patch description .  However the selftests started failing.  The reason
is that efi_reinstall_protocol_interface() does not remove the handle and we have
specific selftests for that.  I don't really know or remember why that was
done like that,  does the EFI spec describe this?  I thought
UninstallProtocolInterface was always supposed to delete the handle if it
had no more protocols

> > 
> > -	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.
> 

We agree again, but I am not trying to fix that atm.  The reason for the
special check and the force flag is that efi_delete_handle() is already
doing something similar, which is cwnot optimal, but that's the current
state. It bails out only if the *handle* doesn't exist and doesn't care
if the protocols got removed.  If you prefer having a bigger cleanup + fixes 
let me know.

Thanks
/Ilias
> 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;
> >   	}
> > 
>