Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Tony Krowiak <akrowiak@linux.ibm.com>]

On 5/23/21 6:57 PM, Jason Gunthorpe wrote:
> On Fri, May 21, 2021 at 03:36:48PM -0400, Tony Krowiak wrote:
>> +static struct kvm_s390_crypto_hook
>> +*kvm_arch_crypto_find_hook(enum kvm_s390_crypto_hook_type type)
>> +{
>> +	struct kvm_s390_crypto_hook *crypto_hook;
>> +
>> +	list_for_each_entry(crypto_hook, &crypto_hooks, node) {
>> +		if (crypto_hook->type == type)
>> +			return crypto_hook;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +int kvm_arch_crypto_register_hook(struct kvm_s390_crypto_hook *hook)
>> +{
>> +	struct kvm_s390_crypto_hook *crypto_hook;
>> +
>> +	mutex_lock(&crypto_hooks_lock);
>> +	crypto_hook = kvm_arch_crypto_find_hook(hook->type);
>> +	if (crypto_hook) {
>> +		if (crypto_hook->owner != hook->owner)
>> +			return -EACCES;
>> +		list_replace(&crypto_hook->node, &hook->node);
> This is all dead code right? This is only called from a module init
> function so it can't be called twice.

That is true only if you are considering the current case.
Is it guaranteed that only the vfio_ap module
will call this function and is there a guarantee that it will
always and only be called from the vfio_ap module init
function? For example, suppose a hook is added to
intercept the AP NQAP or DQAP instruction? We don't
know how or when those handlers will be registered
or unregistered. We don't even know if the handlers
will be part of the vfio_ap module or not.

> Just always fail if the hook is
> already used and delete the owner stuff.

I suppose that is reasonable and simpler.

>
> But this is alot of complicated and unused code to solve a lock
> ordering problem..

If you have a better solution, I'm all ears. I've been down this
road a couple of times now and solving lock ordering for
multiple asynchronous processes is not trivial. This seems like
a reasonable solution and provides for flexibility for including
additional hooks to handle interception of other AP instructions.

>
> Jason