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

[Ilias Apalodimas]

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