[v4,3/7] s390: zcrypt: driver callback to indicate resource in use

Commit Message

Anthony Krowiak June 13, 2019, 7:39 p.m. UTC

Introduces a new driver callback to prevent a root user from unbinding
an AP queue from its device driver if the queue is in use. This prevents
a root user from inadvertently taking a queue away from a guest and
giving it to the host, or vice versa. The callback will be invoked
whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
result in one or more AP queues being removed from its driver. If the
callback responds in the affirmative for any driver queried, the change
to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently,
there is only one non-default driver, the vfio_ap device driver. The
vfio_ap device driver manages AP queues passed through to one or more
guests and we don't want to unexpectedly take AP resources away from
guests which are most likely independently administered.

Signed-off-by: Tony Krowiak <[e-mail]>
---
 drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
 drivers/s390/crypto/ap_bus.h |   3 +
 2 files changed, 135 insertions(+), 6 deletions(-)

Comments

Harald Freudenberger June 17, 2019, 9:28 a.m. UTC | #1

On 13.06.19 21:39, Tony Krowiak wrote:
> Introduces a new driver callback to prevent a root user from unbinding
> an AP queue from its device driver if the queue is in use. This prevents
> a root user from inadvertently taking a queue away from a guest and
> giving it to the host, or vice versa. The callback will be invoked
> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
> result in one or more AP queues being removed from its driver. If the
> callback responds in the affirmative for any driver queried, the change
> to the apmask or aqmask will be rejected with a device in use error.
>
> For this patch, only non-default drivers will be queried. Currently,
> there is only one non-default driver, the vfio_ap device driver. The
> vfio_ap device driver manages AP queues passed through to one or more
> guests and we don't want to unexpectedly take AP resources away from
> guests which are most likely independently administered.
>
> Signed-off-by: Tony Krowiak <[e-mail]>
> ---
>  drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
>  drivers/s390/crypto/ap_bus.h |   3 +
>  2 files changed, 135 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
> index b9fc502c58c2..1b06f47d0d1c 100644
> --- a/drivers/s390/crypto/ap_bus.c
> +++ b/drivers/s390/crypto/ap_bus.c
> @@ -35,6 +35,7 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/debugfs.h>
>  #include <linux/ctype.h>
> +#include <linux/module.h>
>  
>  #include "ap_bus.h"
>  #include "ap_debug.h"
> @@ -998,9 +999,11 @@ int ap_parse_mask_str(const char *str,
>  	newmap = kmalloc(size, GFP_KERNEL);
>  	if (!newmap)
>  		return -ENOMEM;
> -	if (mutex_lock_interruptible(lock)) {
> -		kfree(newmap);
> -		return -ERESTARTSYS;
> +	if (lock) {
> +		if (mutex_lock_interruptible(lock)) {
> +			kfree(newmap);
> +			return -ERESTARTSYS;
> +		}
>  	}
>  
>  	if (*str == '+' || *str == '-') {
> @@ -1012,7 +1015,10 @@ int ap_parse_mask_str(const char *str,
>  	}
>  	if (rc == 0)
>  		memcpy(bitmap, newmap, size);
> -	mutex_unlock(lock);
> +
> +	if (lock)
> +		mutex_unlock(lock);
> +
>  	kfree(newmap);
>  	return rc;
>  }
> @@ -1199,12 +1205,72 @@ static ssize_t apmask_show(struct bus_type *bus, char *buf)
>  	return rc;
>  }
>  
> +int __verify_card_reservations(struct device_driver *drv, void *data)
> +{
> +	int rc = 0;
> +	struct ap_driver *ap_drv = to_ap_drv(drv);
> +	unsigned long *newapm = (unsigned long *)data;
> +
> +	/*
> +	 * If the reserved bits do not identify cards reserved for use by the
> +	 * non-default driver, there is no need to verify the driver is using
> +	 * the queues.
> +	 */
> +	if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
> +		return 0;
> +
> +	/* The non-default driver's module must be loaded */
> +	if (!try_module_get(drv->owner))
> +		return 0;
> +
> +	if (ap_drv->in_use)
> +		if (ap_drv->in_use(newapm, ap_perms.aqm))
> +			rc = -EADDRINUSE;
> +
> +	module_put(drv->owner);
> +
> +	return rc;
> +}
> +
> +static int apmask_commit(unsigned long *newapm)
> +{
> +	int rc;
> +	unsigned long reserved[BITS_TO_LONGS(AP_DEVICES)];
> +
> +	/*
> +	 * Check if any bits in the apmask have been set which will
> +	 * result in queues being removed from non-default drivers
> +	 */
> +	if (bitmap_andnot(reserved, newapm, ap_perms.apm, AP_DEVICES)) {
> +		rc = (bus_for_each_drv(&ap_bus_type, NULL, reserved,
> +				       __verify_card_reservations));
Why surround the bus_for_each_drv() with additional parenthesis ?
> +		if (rc)
> +			return rc;
> +	}
> +
> +	memcpy(ap_perms.apm, newapm, APMASKSIZE);
> +
> +	return 0;
> +}
> +
>  static ssize_t apmask_store(struct bus_type *bus, const char *buf,
>  			    size_t count)
>  {
>  	int rc;
> +	unsigned long newapm[BITS_TO_LONGS(AP_DEVICES)];
> +
> +	memcpy(newapm, ap_perms.apm, APMASKSIZE);
> +
> +	rc = ap_parse_mask_str(buf, newapm, AP_DEVICES, NULL);
> +	if (rc)
> +		return rc;
> +
> +	if (mutex_lock_interruptible(&ap_perms_mutex))
> +		return -ERESTARTSYS;
This lock is too late. Move it before the memcpy(newapm, ap_perms.apm, APMASKSIZE);
> +
> +	rc = apmask_commit(newapm);
> +	mutex_unlock(&ap_perms_mutex);
>  
> -	rc = ap_parse_mask_str(buf, ap_perms.apm, AP_DEVICES, &ap_perms_mutex);
>  	if (rc)
>  		return rc;
>  
> @@ -1230,12 +1296,72 @@ static ssize_t aqmask_show(struct bus_type *bus, char *buf)
>  	return rc;
>  }
>  
> +int __verify_queue_reservations(struct device_driver *drv, void *data)
> +{
> +	int rc = 0;
> +	struct ap_driver *ap_drv = to_ap_drv(drv);
> +	unsigned long *newaqm = (unsigned long *)data;
> +
> +	/*
> +	 * If the reserved bits do not identify queues reserved for use by the
> +	 * non-default driver, there is no need to verify the driver is using
> +	 * the queues.
> +	 */
> +	if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
> +		return 0;
> +
> +	/* The non-default driver's module must be loaded */
> +	if (!try_module_get(drv->owner))
> +		return 0;
> +
> +	if (ap_drv->in_use)
> +		if (ap_drv->in_use(ap_perms.apm, newaqm))
> +			rc = -EADDRINUSE;
> +
> +	module_put(drv->owner);
> +
> +	return rc;
> +}
> +
> +static int aqmask_commit(unsigned long *newaqm)
> +{
> +	int rc;
> +	unsigned long reserved[BITS_TO_LONGS(AP_DOMAINS)];
> +
> +	/*
> +	 * Check if any bits in the aqmask have been set which will
> +	 * result in queues being removed from non-default drivers
> +	 */
> +	if (bitmap_andnot(reserved, newaqm, ap_perms.aqm, AP_DOMAINS)) {
> +		rc = (bus_for_each_drv(&ap_bus_type, NULL, reserved,
> +				       __verify_queue_reservations));
Same here: please remove the surrounding ( ... ).
> +		if (rc)
> +			return rc;
> +	}
> +
> +	memcpy(ap_perms.aqm, newaqm, APMASKSIZE);
APMASKSIZE -> AQMASKSIZE
> +
> +	return 0;
> +}
> +
>  static ssize_t aqmask_store(struct bus_type *bus, const char *buf,
>  			    size_t count)
>  {
>  	int rc;
> +	unsigned long newaqm[BITS_TO_LONGS(AP_DEVICES)];
> +
> +	memcpy(newaqm, ap_perms.aqm, AQMASKSIZE);
> +
> +	rc = ap_parse_mask_str(buf, newaqm, AP_DOMAINS, NULL);
> +	if (rc)
> +		return rc;
> +
> +	if (mutex_lock_interruptible(&ap_perms_mutex))
> +		return -ERESTARTSYS;
Please move the lock before the memcpy(newaqm, ...)
> +
> +	rc = aqmask_commit(newaqm);
> +	mutex_unlock(&ap_perms_mutex);
>  
> -	rc = ap_parse_mask_str(buf, ap_perms.aqm, AP_DOMAINS, &ap_perms_mutex);
>  	if (rc)
>  		return rc;
>  
> diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
> index 6f3cf37776ca..0f865c5545f2 100644
> --- a/drivers/s390/crypto/ap_bus.h
> +++ b/drivers/s390/crypto/ap_bus.h
> @@ -137,6 +137,7 @@ struct ap_driver {
>  	void (*remove)(struct ap_device *);
>  	void (*suspend)(struct ap_device *);
>  	void (*resume)(struct ap_device *);
> +	bool (*in_use)(unsigned long *apm, unsigned long *aqm);
>  };
>  
>  #define to_ap_drv(x) container_of((x), struct ap_driver, driver)
> @@ -265,6 +266,8 @@ void ap_queue_reinit_state(struct ap_queue *aq);
>  struct ap_card *ap_card_create(int id, int queue_depth, int raw_device_type,
>  			       int comp_device_type, unsigned int functions);
>  
> +#define APMASKSIZE (BITS_TO_LONGS(AP_DEVICES) * sizeof(unsigned long))
> +#define AQMASKSIZE (BITS_TO_LONGS(AP_DOMAINS) * sizeof(unsigned long))
add one empty line here please
>  struct ap_perms {
>  	unsigned long ioctlm[BITS_TO_LONGS(AP_IOCTLS)];
>  	unsigned long apm[BITS_TO_LONGS(AP_DEVICES)];

Anthony Krowiak June 17, 2019, 2:37 p.m. UTC | #2

On 6/17/19 5:28 AM, Harald Freudenberger wrote:
> On 13.06.19 21:39, Tony Krowiak wrote:
>> Introduces a new driver callback to prevent a root user from unbinding
>> an AP queue from its device driver if the queue is in use. This prevents
>> a root user from inadvertently taking a queue away from a guest and
>> giving it to the host, or vice versa. The callback will be invoked
>> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
>> result in one or more AP queues being removed from its driver. If the
>> callback responds in the affirmative for any driver queried, the change
>> to the apmask or aqmask will be rejected with a device in use error.
>>
>> For this patch, only non-default drivers will be queried. Currently,
>> there is only one non-default driver, the vfio_ap device driver. The
>> vfio_ap device driver manages AP queues passed through to one or more
>> guests and we don't want to unexpectedly take AP resources away from
>> guests which are most likely independently administered.
>>
>> Signed-off-by: Tony Krowiak <[e-mail]>
>> ---
>>   drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
>>   drivers/s390/crypto/ap_bus.h |   3 +
>>   2 files changed, 135 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
>> index b9fc502c58c2..1b06f47d0d1c 100644
>> --- a/drivers/s390/crypto/ap_bus.c
>> +++ b/drivers/s390/crypto/ap_bus.c
>> @@ -35,6 +35,7 @@
>>   #include <linux/mod_devicetable.h>
>>   #include <linux/debugfs.h>
>>   #include <linux/ctype.h>
>> +#include <linux/module.h>
>>   
>>   #include "ap_bus.h"
>>   #include "ap_debug.h"
>> @@ -998,9 +999,11 @@ int ap_parse_mask_str(const char *str,
>>   	newmap = kmalloc(size, GFP_KERNEL);
>>   	if (!newmap)
>>   		return -ENOMEM;
>> -	if (mutex_lock_interruptible(lock)) {
>> -		kfree(newmap);
>> -		return -ERESTARTSYS;
>> +	if (lock) {
>> +		if (mutex_lock_interruptible(lock)) {
>> +			kfree(newmap);
>> +			return -ERESTARTSYS;
>> +		}
>>   	}
>>   
>>   	if (*str == '+' || *str == '-') {
>> @@ -1012,7 +1015,10 @@ int ap_parse_mask_str(const char *str,
>>   	}
>>   	if (rc == 0)
>>   		memcpy(bitmap, newmap, size);
>> -	mutex_unlock(lock);
>> +
>> +	if (lock)
>> +		mutex_unlock(lock);
>> +
>>   	kfree(newmap);
>>   	return rc;
>>   }
>> @@ -1199,12 +1205,72 @@ static ssize_t apmask_show(struct bus_type *bus, char *buf)
>>   	return rc;
>>   }
>>   
>> +int __verify_card_reservations(struct device_driver *drv, void *data)
>> +{
>> +	int rc = 0;
>> +	struct ap_driver *ap_drv = to_ap_drv(drv);
>> +	unsigned long *newapm = (unsigned long *)data;
>> +
>> +	/*
>> +	 * If the reserved bits do not identify cards reserved for use by the
>> +	 * non-default driver, there is no need to verify the driver is using
>> +	 * the queues.
>> +	 */
>> +	if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
>> +		return 0;
>> +
>> +	/* The non-default driver's module must be loaded */
>> +	if (!try_module_get(drv->owner))
>> +		return 0;
>> +
>> +	if (ap_drv->in_use)
>> +		if (ap_drv->in_use(newapm, ap_perms.aqm))
>> +			rc = -EADDRINUSE;
>> +
>> +	module_put(drv->owner);
>> +
>> +	return rc;
>> +}
>> +
>> +static int apmask_commit(unsigned long *newapm)
>> +{
>> +	int rc;
>> +	unsigned long reserved[BITS_TO_LONGS(AP_DEVICES)];
>> +
>> +	/*
>> +	 * Check if any bits in the apmask have been set which will
>> +	 * result in queues being removed from non-default drivers
>> +	 */
>> +	if (bitmap_andnot(reserved, newapm, ap_perms.apm, AP_DEVICES)) {
>> +		rc = (bus_for_each_drv(&ap_bus_type, NULL, reserved,
>> +				       __verify_card_reservations));
> Why surround the bus_for_each_drv() with additional parenthesis ?

You are right, it is not necessary.

>> +		if (rc)
>> +			return rc;
>> +	}
>> +
>> +	memcpy(ap_perms.apm, newapm, APMASKSIZE);
>> +
>> +	return 0;
>> +}
>> +
>>   static ssize_t apmask_store(struct bus_type *bus, const char *buf,
>>   			    size_t count)
>>   {
>>   	int rc;
>> +	unsigned long newapm[BITS_TO_LONGS(AP_DEVICES)];
>> +
>> +	memcpy(newapm, ap_perms.apm, APMASKSIZE);
>> +
>> +	rc = ap_parse_mask_str(buf, newapm, AP_DEVICES, NULL);
>> +	if (rc)
>> +		return rc;
>> +
>> +	if (mutex_lock_interruptible(&ap_perms_mutex))
>> +		return -ERESTARTSYS;
> This lock is too late. Move it before the memcpy(newapm, ap_perms.apm, APMASKSIZE);

I agree, it shall be moved.

>> +
>> +	rc = apmask_commit(newapm);
>> +	mutex_unlock(&ap_perms_mutex);
>>   
>> -	rc = ap_parse_mask_str(buf, ap_perms.apm, AP_DEVICES, &ap_perms_mutex);
>>   	if (rc)
>>   		return rc;
>>   
>> @@ -1230,12 +1296,72 @@ static ssize_t aqmask_show(struct bus_type *bus, char *buf)
>>   	return rc;
>>   }
>>   
>> +int __verify_queue_reservations(struct device_driver *drv, void *data)
>> +{
>> +	int rc = 0;
>> +	struct ap_driver *ap_drv = to_ap_drv(drv);
>> +	unsigned long *newaqm = (unsigned long *)data;
>> +
>> +	/*
>> +	 * If the reserved bits do not identify queues reserved for use by the
>> +	 * non-default driver, there is no need to verify the driver is using
>> +	 * the queues.
>> +	 */
>> +	if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
>> +		return 0;
>> +
>> +	/* The non-default driver's module must be loaded */
>> +	if (!try_module_get(drv->owner))
>> +		return 0;
>> +
>> +	if (ap_drv->in_use)
>> +		if (ap_drv->in_use(ap_perms.apm, newaqm))
>> +			rc = -EADDRINUSE;
>> +
>> +	module_put(drv->owner);
>> +
>> +	return rc;
>> +}
>> +
>> +static int aqmask_commit(unsigned long *newaqm)
>> +{
>> +	int rc;
>> +	unsigned long reserved[BITS_TO_LONGS(AP_DOMAINS)];
>> +
>> +	/*
>> +	 * Check if any bits in the aqmask have been set which will
>> +	 * result in queues being removed from non-default drivers
>> +	 */
>> +	if (bitmap_andnot(reserved, newaqm, ap_perms.aqm, AP_DOMAINS)) {
>> +		rc = (bus_for_each_drv(&ap_bus_type, NULL, reserved,
>> +				       __verify_queue_reservations));
> Same here: please remove the surrounding ( ... ).

Will do.

>> +		if (rc)
>> +			return rc;
>> +	}
>> +
>> +	memcpy(ap_perms.aqm, newaqm, APMASKSIZE);
> APMASKSIZE -> AQMASKSIZE
>> +
>> +	return 0;
>> +}
>> +
>>   static ssize_t aqmask_store(struct bus_type *bus, const char *buf,
>>   			    size_t count)
>>   {
>>   	int rc;
>> +	unsigned long newaqm[BITS_TO_LONGS(AP_DEVICES)];
>> +
>> +	memcpy(newaqm, ap_perms.aqm, AQMASKSIZE);
>> +
>> +	rc = ap_parse_mask_str(buf, newaqm, AP_DOMAINS, NULL);
>> +	if (rc)
>> +		return rc;
>> +
>> +	if (mutex_lock_interruptible(&ap_perms_mutex))
>> +		return -ERESTARTSYS;
> Please move the lock before the memcpy(newaqm, ...)

Will do.

>> +
>> +	rc = aqmask_commit(newaqm);
>> +	mutex_unlock(&ap_perms_mutex);
>>   
>> -	rc = ap_parse_mask_str(buf, ap_perms.aqm, AP_DOMAINS, &ap_perms_mutex);
>>   	if (rc)
>>   		return rc;
>>   
>> diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
>> index 6f3cf37776ca..0f865c5545f2 100644
>> --- a/drivers/s390/crypto/ap_bus.h
>> +++ b/drivers/s390/crypto/ap_bus.h
>> @@ -137,6 +137,7 @@ struct ap_driver {
>>   	void (*remove)(struct ap_device *);
>>   	void (*suspend)(struct ap_device *);
>>   	void (*resume)(struct ap_device *);
>> +	bool (*in_use)(unsigned long *apm, unsigned long *aqm);
>>   };
>>   
>>   #define to_ap_drv(x) container_of((x), struct ap_driver, driver)
>> @@ -265,6 +266,8 @@ void ap_queue_reinit_state(struct ap_queue *aq);
>>   struct ap_card *ap_card_create(int id, int queue_depth, int raw_device_type,
>>   			       int comp_device_type, unsigned int functions);
>>   
>> +#define APMASKSIZE (BITS_TO_LONGS(AP_DEVICES) * sizeof(unsigned long))
>> +#define AQMASKSIZE (BITS_TO_LONGS(AP_DOMAINS) * sizeof(unsigned long))
> add one empty line here please
>>   struct ap_perms {
>>   	unsigned long ioctlm[BITS_TO_LONGS(AP_IOCTLS)];
>>   	unsigned long apm[BITS_TO_LONGS(AP_DEVICES)];
>

Cornelia Huck June 18, 2019, 4:25 p.m. UTC | #3

On Thu, 13 Jun 2019 15:39:36 -0400
Tony Krowiak <[e-mail]> wrote:

> Introduces a new driver callback to prevent a root user from unbinding
> an AP queue from its device driver if the queue is in use. This prevents
> a root user from inadvertently taking a queue away from a guest and
> giving it to the host, or vice versa. The callback will be invoked
> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
> result in one or more AP queues being removed from its driver. If the
> callback responds in the affirmative for any driver queried, the change
> to the apmask or aqmask will be rejected with a device in use error.
> 
> For this patch, only non-default drivers will be queried. Currently,
> there is only one non-default driver, the vfio_ap device driver. The
> vfio_ap device driver manages AP queues passed through to one or more
> guests and we don't want to unexpectedly take AP resources away from
> guests which are most likely independently administered.
> 
> Signed-off-by: Tony Krowiak <[e-mail]>
> ---
>  drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
>  drivers/s390/crypto/ap_bus.h |   3 +
>  2 files changed, 135 insertions(+), 6 deletions(-)

Hm... I recall objecting to this patch before, fearing that it makes it
possible for a bad actor to hog resources that can't be removed by
root, even forcefully. (I have not had time to look at the intervening
versions, so I might be missing something.)

Is there a way for root to forcefully override this?

Anthony Krowiak June 19, 2019, 1:04 p.m. UTC | #4

On 6/18/19 12:25 PM, Cornelia Huck wrote:
> On Thu, 13 Jun 2019 15:39:36 -0400
> Tony Krowiak <[e-mail]> wrote:
> 
>> Introduces a new driver callback to prevent a root user from unbinding
>> an AP queue from its device driver if the queue is in use. This prevents
>> a root user from inadvertently taking a queue away from a guest and
>> giving it to the host, or vice versa. The callback will be invoked
>> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
>> result in one or more AP queues being removed from its driver. If the
>> callback responds in the affirmative for any driver queried, the change
>> to the apmask or aqmask will be rejected with a device in use error.
>>
>> For this patch, only non-default drivers will be queried. Currently,
>> there is only one non-default driver, the vfio_ap device driver. The
>> vfio_ap device driver manages AP queues passed through to one or more
>> guests and we don't want to unexpectedly take AP resources away from
>> guests which are most likely independently administered.
>>
>> Signed-off-by: Tony Krowiak <[e-mail]>
>> ---
>>   drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
>>   drivers/s390/crypto/ap_bus.h |   3 +
>>   2 files changed, 135 insertions(+), 6 deletions(-)
> 
> Hm... I recall objecting to this patch before, fearing that it makes it
> possible for a bad actor to hog resources that can't be removed by
> root, even forcefully. (I have not had time to look at the intervening
> versions, so I might be missing something.)
> 
> Is there a way for root to forcefully override this?

You recall correctly; however, after many internal crypto team
discussions, it was decided that this feature was important
and should be kept.

Allow me to first address your fear that a bad actor can hog
resources that can't be removed by root. With this enhancement,
there is nothing preventing a root user from taking resources
from a matrix mdev, it simply forces him/her to follow the
proper procedure. The resources to be removed must first be
unassigned from the matrix mdev to which they are assigned.
The AP bus's /sys/bus/ap/apmask and /sys/bus/ap/aqmask
sysfs attributes can then be edited to transfer ownership
of the resources to zcrypt.

The rationale for keeping this feature is:

* It is a bad idea to steal an adapter in use from a guest. In the worst
   case, the guest could end up without access to any crypto adapters
   without knowing why. This could lead to performance issues on guests
   that rely heavily on crypto such as guests used for blockchain
   transactions.

* There are plenty of examples in linux of the kernel preventing a root
   user from performing a task. For example, a module can't be removed
   if references are still held for it. Another example would be trying
   to bind a CEX4 adapter to a device driver not registered for CEX4;
   this action will also be rejected.

* The semantics are much cleaner and the logic is far less complicated.

* It forces the use of the proper procedure to change ownership of AP
   queues.


>

Anthony Krowiak June 26, 2019, 9:13 p.m. UTC | #5

On 6/19/19 9:04 AM, Tony Krowiak wrote:
> On 6/18/19 12:25 PM, Cornelia Huck wrote:
>> On Thu, 13 Jun 2019 15:39:36 -0400
>> Tony Krowiak <[e-mail]> wrote:
>>
>>> Introduces a new driver callback to prevent a root user from unbinding
>>> an AP queue from its device driver if the queue is in use. This prevents
>>> a root user from inadvertently taking a queue away from a guest and
>>> giving it to the host, or vice versa. The callback will be invoked
>>> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
>>> result in one or more AP queues being removed from its driver. If the
>>> callback responds in the affirmative for any driver queried, the change
>>> to the apmask or aqmask will be rejected with a device in use error.
>>>
>>> For this patch, only non-default drivers will be queried. Currently,
>>> there is only one non-default driver, the vfio_ap device driver. The
>>> vfio_ap device driver manages AP queues passed through to one or more
>>> guests and we don't want to unexpectedly take AP resources away from
>>> guests which are most likely independently administered.
>>>
>>> Signed-off-by: Tony Krowiak <[e-mail]>
>>> ---
>>>   drivers/s390/crypto/ap_bus.c | 138 
>>> +++++++++++++++++++++++++++++++++++++++++--
>>>   drivers/s390/crypto/ap_bus.h |   3 +
>>>   2 files changed, 135 insertions(+), 6 deletions(-)
>>
>> Hm... I recall objecting to this patch before, fearing that it makes it
>> possible for a bad actor to hog resources that can't be removed by
>> root, even forcefully. (I have not had time to look at the intervening
>> versions, so I might be missing something.)
>>
>> Is there a way for root to forcefully override this?
> 
> You recall correctly; however, after many internal crypto team
> discussions, it was decided that this feature was important
> and should be kept.
> 
> Allow me to first address your fear that a bad actor can hog
> resources that can't be removed by root. With this enhancement,
> there is nothing preventing a root user from taking resources
> from a matrix mdev, it simply forces him/her to follow the
> proper procedure. The resources to be removed must first be
> unassigned from the matrix mdev to which they are assigned.
> The AP bus's /sys/bus/ap/apmask and /sys/bus/ap/aqmask
> sysfs attributes can then be edited to transfer ownership
> of the resources to zcrypt.
> 
> The rationale for keeping this feature is:
> 
> * It is a bad idea to steal an adapter in use from a guest. In the worst
>    case, the guest could end up without access to any crypto adapters
>    without knowing why. This could lead to performance issues on guests
>    that rely heavily on crypto such as guests used for blockchain
>    transactions.
> 
> * There are plenty of examples in linux of the kernel preventing a root
>    user from performing a task. For example, a module can't be removed
>    if references are still held for it. Another example would be trying
>    to bind a CEX4 adapter to a device driver not registered for CEX4;
>    this action will also be rejected.
> 
> * The semantics are much cleaner and the logic is far less complicated.
> 
> * It forces the use of the proper procedure to change ownership of AP
>    queues.
>

Any feedback on this?

Tony K

> 
>>
>

Cornelia Huck June 27, 2019, 7:25 a.m. UTC | #6

On Wed, 26 Jun 2019 17:13:50 -0400
Tony Krowiak <[e-mail]> wrote:

> On 6/19/19 9:04 AM, Tony Krowiak wrote:
> > On 6/18/19 12:25 PM, Cornelia Huck wrote:  
> >> On Thu, 13 Jun 2019 15:39:36 -0400
> >> Tony Krowiak <[e-mail]> wrote:
> >>  
> >>> Introduces a new driver callback to prevent a root user from unbinding
> >>> an AP queue from its device driver if the queue is in use. This prevents
> >>> a root user from inadvertently taking a queue away from a guest and
> >>> giving it to the host, or vice versa. The callback will be invoked
> >>> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
> >>> result in one or more AP queues being removed from its driver. If the
> >>> callback responds in the affirmative for any driver queried, the change
> >>> to the apmask or aqmask will be rejected with a device in use error.
> >>>
> >>> For this patch, only non-default drivers will be queried. Currently,
> >>> there is only one non-default driver, the vfio_ap device driver. The
> >>> vfio_ap device driver manages AP queues passed through to one or more
> >>> guests and we don't want to unexpectedly take AP resources away from
> >>> guests which are most likely independently administered.
> >>>
> >>> Signed-off-by: Tony Krowiak <[e-mail]>
> >>> ---
> >>>   drivers/s390/crypto/ap_bus.c | 138 
> >>> +++++++++++++++++++++++++++++++++++++++++--
> >>>   drivers/s390/crypto/ap_bus.h |   3 +
> >>>   2 files changed, 135 insertions(+), 6 deletions(-)  
> >>
> >> Hm... I recall objecting to this patch before, fearing that it makes it
> >> possible for a bad actor to hog resources that can't be removed by
> >> root, even forcefully. (I have not had time to look at the intervening
> >> versions, so I might be missing something.)
> >>
> >> Is there a way for root to forcefully override this?  
> > 
> > You recall correctly; however, after many internal crypto team
> > discussions, it was decided that this feature was important
> > and should be kept.
> > 
> > Allow me to first address your fear that a bad actor can hog
> > resources that can't be removed by root. With this enhancement,
> > there is nothing preventing a root user from taking resources
> > from a matrix mdev, it simply forces him/her to follow the
> > proper procedure. The resources to be removed must first be
> > unassigned from the matrix mdev to which they are assigned.
> > The AP bus's /sys/bus/ap/apmask and /sys/bus/ap/aqmask
> > sysfs attributes can then be edited to transfer ownership
> > of the resources to zcrypt.
> > 
> > The rationale for keeping this feature is:
> > 
> > * It is a bad idea to steal an adapter in use from a guest. In the worst
> >    case, the guest could end up without access to any crypto adapters
> >    without knowing why. This could lead to performance issues on guests
> >    that rely heavily on crypto such as guests used for blockchain
> >    transactions.
> > 
> > * There are plenty of examples in linux of the kernel preventing a root
> >    user from performing a task. For example, a module can't be removed
> >    if references are still held for it. Another example would be trying
> >    to bind a CEX4 adapter to a device driver not registered for CEX4;
> >    this action will also be rejected.
> > 
> > * The semantics are much cleaner and the logic is far less complicated.
> > 
> > * It forces the use of the proper procedure to change ownership of AP
> >    queues.
> >  
> 
> Any feedback on this?

Had not yet time to look at this, sorry.


> 
> Tony K
> 
> >   
> >>  
> >   
>

Anthony Krowiak June 27, 2019, 12:59 p.m. UTC | #7

On 6/27/19 3:25 AM, Cornelia Huck wrote:
> On Wed, 26 Jun 2019 17:13:50 -0400
> Tony Krowiak <[e-mail]> wrote:
> 
>> On 6/19/19 9:04 AM, Tony Krowiak wrote:
>>> On 6/18/19 12:25 PM, Cornelia Huck wrote:
>>>> On Thu, 13 Jun 2019 15:39:36 -0400
>>>> Tony Krowiak <[e-mail]> wrote:
>>>>   
>>>>> Introduces a new driver callback to prevent a root user from unbinding
>>>>> an AP queue from its device driver if the queue is in use. This prevents
>>>>> a root user from inadvertently taking a queue away from a guest and
>>>>> giving it to the host, or vice versa. The callback will be invoked
>>>>> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
>>>>> result in one or more AP queues being removed from its driver. If the
>>>>> callback responds in the affirmative for any driver queried, the change
>>>>> to the apmask or aqmask will be rejected with a device in use error.
>>>>>
>>>>> For this patch, only non-default drivers will be queried. Currently,
>>>>> there is only one non-default driver, the vfio_ap device driver. The
>>>>> vfio_ap device driver manages AP queues passed through to one or more
>>>>> guests and we don't want to unexpectedly take AP resources away from
>>>>> guests which are most likely independently administered.
>>>>>
>>>>> Signed-off-by: Tony Krowiak <[e-mail]>
>>>>> ---
>>>>>    drivers/s390/crypto/ap_bus.c | 138
>>>>> +++++++++++++++++++++++++++++++++++++++++--
>>>>>    drivers/s390/crypto/ap_bus.h |   3 +
>>>>>    2 files changed, 135 insertions(+), 6 deletions(-)
>>>>
>>>> Hm... I recall objecting to this patch before, fearing that it makes it
>>>> possible for a bad actor to hog resources that can't be removed by
>>>> root, even forcefully. (I have not had time to look at the intervening
>>>> versions, so I might be missing something.)
>>>>
>>>> Is there a way for root to forcefully override this?
>>>
>>> You recall correctly; however, after many internal crypto team
>>> discussions, it was decided that this feature was important
>>> and should be kept.
>>>
>>> Allow me to first address your fear that a bad actor can hog
>>> resources that can't be removed by root. With this enhancement,
>>> there is nothing preventing a root user from taking resources
>>> from a matrix mdev, it simply forces him/her to follow the
>>> proper procedure. The resources to be removed must first be
>>> unassigned from the matrix mdev to which they are assigned.
>>> The AP bus's /sys/bus/ap/apmask and /sys/bus/ap/aqmask
>>> sysfs attributes can then be edited to transfer ownership
>>> of the resources to zcrypt.
>>>
>>> The rationale for keeping this feature is:
>>>
>>> * It is a bad idea to steal an adapter in use from a guest. In the worst
>>>     case, the guest could end up without access to any crypto adapters
>>>     without knowing why. This could lead to performance issues on guests
>>>     that rely heavily on crypto such as guests used for blockchain
>>>     transactions.
>>>
>>> * There are plenty of examples in linux of the kernel preventing a root
>>>     user from performing a task. For example, a module can't be removed
>>>     if references are still held for it. Another example would be trying
>>>     to bind a CEX4 adapter to a device driver not registered for CEX4;
>>>     this action will also be rejected.
>>>
>>> * The semantics are much cleaner and the logic is far less complicated.
>>>
>>> * It forces the use of the proper procedure to change ownership of AP
>>>     queues.
>>>   
>>
>> Any feedback on this?
> 
> Had not yet time to look at this, sorry.

No problem, just wanted to make sure it didn't get lost in the shuffle.

> 
> 
>>
>> Tony K
>>
>>>    
>>>>   
>>>    
>>
>

Cornelia Huck July 1, 2019, 7:26 p.m. UTC | #8

On Wed, 19 Jun 2019 09:04:18 -0400
Tony Krowiak <[e-mail]> wrote:

> On 6/18/19 12:25 PM, Cornelia Huck wrote:
> > On Thu, 13 Jun 2019 15:39:36 -0400
> > Tony Krowiak <[e-mail]> wrote:
> >   
> >> Introduces a new driver callback to prevent a root user from unbinding
> >> an AP queue from its device driver if the queue is in use. This prevents
> >> a root user from inadvertently taking a queue away from a guest and
> >> giving it to the host, or vice versa. The callback will be invoked
> >> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
> >> result in one or more AP queues being removed from its driver. If the
> >> callback responds in the affirmative for any driver queried, the change
> >> to the apmask or aqmask will be rejected with a device in use error.
> >>
> >> For this patch, only non-default drivers will be queried. Currently,
> >> there is only one non-default driver, the vfio_ap device driver. The
> >> vfio_ap device driver manages AP queues passed through to one or more
> >> guests and we don't want to unexpectedly take AP resources away from
> >> guests which are most likely independently administered.
> >>
> >> Signed-off-by: Tony Krowiak <[e-mail]>
> >> ---
> >>   drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
> >>   drivers/s390/crypto/ap_bus.h |   3 +
> >>   2 files changed, 135 insertions(+), 6 deletions(-)  
> > 
> > Hm... I recall objecting to this patch before, fearing that it makes it
> > possible for a bad actor to hog resources that can't be removed by
> > root, even forcefully. (I have not had time to look at the intervening
> > versions, so I might be missing something.)
> > 
> > Is there a way for root to forcefully override this?  
> 
> You recall correctly; however, after many internal crypto team
> discussions, it was decided that this feature was important
> and should be kept.

That's the problem with internal discussions: they're internal :( Would
have been nice to have some summary of this in the changelog.

> 
> Allow me to first address your fear that a bad actor can hog
> resources that can't be removed by root. With this enhancement,
> there is nothing preventing a root user from taking resources
> from a matrix mdev, it simply forces him/her to follow the
> proper procedure. The resources to be removed must first be
> unassigned from the matrix mdev to which they are assigned.
> The AP bus's /sys/bus/ap/apmask and /sys/bus/ap/aqmask
> sysfs attributes can then be edited to transfer ownership
> of the resources to zcrypt.

What is the suggested procedure when root wants to unbind a queue
device? Find the mdev using the queue (is that easy enough?), unassign
it, then unbind? Failing to unbind is a bit unexpected; can we point
the admin to the correct mdev from which the queue has to be removed
first?

> 
> The rationale for keeping this feature is:
> 
> * It is a bad idea to steal an adapter in use from a guest. In the worst
>    case, the guest could end up without access to any crypto adapters
>    without knowing why. This could lead to performance issues on guests
>    that rely heavily on crypto such as guests used for blockchain
>    transactions.

Yanking adapters out from a running guest is not a good idea, yes; but
I see it more as a problem of the management layer. Performance issues
etc. are not something we want, obviously; but is removing access to
the adapter deadly, or can the guest keep limping along? (Does the
guest have any chance to find out that the adapter is gone? What
happens on an LPAR if an adapter is gone, maybe due to a hardware
failure?)

> 
> * There are plenty of examples in linux of the kernel preventing a root
>    user from performing a task. For example, a module can't be removed
>    if references are still held for it. 

In this case, removing the module would actively break/crash anything
relying on it; I'm not sure how analogous the situation is here (i.e.
can we limp on without the device?)

> Another example would be trying
>    to bind a CEX4 adapter to a device driver not registered for CEX4;
>    this action will also be rejected.

I don't think this one is analogous at all: The device driver can't
drive the device, so why should it be able to bind to it?

> 
> * The semantics are much cleaner and the logic is far less complicated.

This is actually the most convincing of the arguments, I think :) If we
need some really byzantine logic to allow unbinding, it's probably not
worth it.

> 
> * It forces the use of the proper procedure to change ownership of AP
>    queues.

This needs to be properly documented, and the admin needs to have a
chance to find out why unbinding didn't work and what needs to be done
(see my comments above).

Anthony Krowiak July 8, 2019, 2:27 p.m. UTC | #9

On 7/1/19 3:26 PM, Cornelia Huck wrote:
> On Wed, 19 Jun 2019 09:04:18 -0400
> Tony Krowiak <[e-mail]> wrote:
> 
>> On 6/18/19 12:25 PM, Cornelia Huck wrote:
>>> On Thu, 13 Jun 2019 15:39:36 -0400
>>> Tony Krowiak <[e-mail]> wrote:
>>>    
>>>> Introduces a new driver callback to prevent a root user from unbinding
>>>> an AP queue from its device driver if the queue is in use. This prevents
>>>> a root user from inadvertently taking a queue away from a guest and
>>>> giving it to the host, or vice versa. The callback will be invoked
>>>> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
>>>> result in one or more AP queues being removed from its driver. If the
>>>> callback responds in the affirmative for any driver queried, the change
>>>> to the apmask or aqmask will be rejected with a device in use error.
>>>>
>>>> For this patch, only non-default drivers will be queried. Currently,
>>>> there is only one non-default driver, the vfio_ap device driver. The
>>>> vfio_ap device driver manages AP queues passed through to one or more
>>>> guests and we don't want to unexpectedly take AP resources away from
>>>> guests which are most likely independently administered.
>>>>
>>>> Signed-off-by: Tony Krowiak <[e-mail]>
>>>> ---
>>>>    drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
>>>>    drivers/s390/crypto/ap_bus.h |   3 +
>>>>    2 files changed, 135 insertions(+), 6 deletions(-)
>>>
>>> Hm... I recall objecting to this patch before, fearing that it makes it
>>> possible for a bad actor to hog resources that can't be removed by
>>> root, even forcefully. (I have not had time to look at the intervening
>>> versions, so I might be missing something.)
>>>
>>> Is there a way for root to forcefully override this?
>>
>> You recall correctly; however, after many internal crypto team
>> discussions, it was decided that this feature was important
>> and should be kept.
> 
> That's the problem with internal discussions: they're internal :( Would
> have been nice to have some summary of this in the changelog.

I tried to summarize everything here.

> 
>>
>> Allow me to first address your fear that a bad actor can hog
>> resources that can't be removed by root. With this enhancement,
>> there is nothing preventing a root user from taking resources
>> from a matrix mdev, it simply forces him/her to follow the
>> proper procedure. The resources to be removed must first be
>> unassigned from the matrix mdev to which they are assigned.
>> The AP bus's /sys/bus/ap/apmask and /sys/bus/ap/aqmask
>> sysfs attributes can then be edited to transfer ownership
>> of the resources to zcrypt.
> 
> What is the suggested procedure when root wants to unbind a queue
> device? Find the mdev using the queue (is that easy enough?), unassign
> it, then unbind? Failing to unbind is a bit unexpected; can we point
> the admin to the correct mdev from which the queue has to be removed
> first?

The proper procedure is to first unassign the adapter, domain, or both
from the mdev to which the APQN is assigned. The difficulty in finding
the queue depends upon how many mdevs have been created. I would expect
that an admin would keep records of who owns what, but in the case he or
she doesn't, it would be a matter of printing out the matrix attribute
of each mdev until you find the mdev to which the APQN is assigned.
The only means I know of for informing the admin to which mdev a given
APQN is assigned is to log the error when it occurs. I think Matt is
also looking to provide query functions in the management tool on which
he is currently working.

> 
>>
>> The rationale for keeping this feature is:
>>
>> * It is a bad idea to steal an adapter in use from a guest. In the worst
>>     case, the guest could end up without access to any crypto adapters
>>     without knowing why. This could lead to performance issues on guests
>>     that rely heavily on crypto such as guests used for blockchain
>>     transactions.
> 
> Yanking adapters out from a running guest is not a good idea, yes; but
> I see it more as a problem of the management layer. Performance issues
> etc. are not something we want, obviously; but is removing access to
> the adapter deadly, or can the guest keep limping along? (Does the
> guest have any chance to find out that the adapter is gone? What
> happens on an LPAR if an adapter is gone, maybe due to a hardware
> failure?)

I don't think anybody is going to die if an adapter is yanked out;), but
if the guest has only one adapter, then other avenues for crypto
services would have to be used.

> 
>>
>> * There are plenty of examples in linux of the kernel preventing a root
>>     user from performing a task. For example, a module can't be removed
>>     if references are still held for it.
> 
> In this case, removing the module would actively break/crash anything
> relying on it; I'm not sure how analogous the situation is here (i.e.
> can we limp on without the device?)

I believe crypto libraries like libica revert to using other means -
such as crypto software or CPACF functions? - if no adapters are
available, so I don't think the guest is dead in the water as far as
crypto goes, but I'm certainly no expert in what happens above the AP
layer.

> 
>> Another example would be trying
>>     to bind a CEX4 adapter to a device driver not registered for CEX4;
>>     this action will also be rejected.
> 
> I don't think this one is analogous at all: The device driver can't
> drive the device, so why should it be able to bind to it?

Yes, probably a bad example

> 
>>
>> * The semantics are much cleaner and the logic is far less complicated.
> 
> This is actually the most convincing of the arguments, I think :) If we
> need some really byzantine logic to allow unbinding, it's probably not
> worth it.
> 
>>
>> * It forces the use of the proper procedure to change ownership of AP
>>     queues.
> 
> This needs to be properly documented, and the admin needs to have a
> chance to find out why unbinding didn't work and what needs to be done
> (see my comments above).

I will create a section in the vfio-ap.txt document that comes with this
patch set describing the proper procedure for unbinding queues. Of
course, we'll make sure the official IBM doc also more thoroughly
describes this.

>

Cornelia Huck July 9, 2019, 10:49 a.m. UTC | #10

On Mon, 8 Jul 2019 10:27:11 -0400
Tony Krowiak <[e-mail]> wrote:

> On 7/1/19 3:26 PM, Cornelia Huck wrote:
> > On Wed, 19 Jun 2019 09:04:18 -0400
> > Tony Krowiak <[e-mail]> wrote:

> >> Allow me to first address your fear that a bad actor can hog
> >> resources that can't be removed by root. With this enhancement,
> >> there is nothing preventing a root user from taking resources
> >> from a matrix mdev, it simply forces him/her to follow the
> >> proper procedure. The resources to be removed must first be
> >> unassigned from the matrix mdev to which they are assigned.
> >> The AP bus's /sys/bus/ap/apmask and /sys/bus/ap/aqmask
> >> sysfs attributes can then be edited to transfer ownership
> >> of the resources to zcrypt.  
> > 
> > What is the suggested procedure when root wants to unbind a queue
> > device? Find the mdev using the queue (is that easy enough?), unassign
> > it, then unbind? Failing to unbind is a bit unexpected; can we point
> > the admin to the correct mdev from which the queue has to be removed
> > first?  
> 
> The proper procedure is to first unassign the adapter, domain, or both
> from the mdev to which the APQN is assigned. The difficulty in finding
> the queue depends upon how many mdevs have been created. I would expect
> that an admin would keep records of who owns what, but in the case he or
> she doesn't, it would be a matter of printing out the matrix attribute
> of each mdev until you find the mdev to which the APQN is assigned.

Ok, so the information is basically available, if needed.

> The only means I know of for informing the admin to which mdev a given
> APQN is assigned is to log the error when it occurs. 

That might be helpful, if it's easy to do.

> I think Matt is
> also looking to provide query functions in the management tool on which
> he is currently working.

That also sounds helpful.

(...)

> >> * It forces the use of the proper procedure to change ownership of AP
> >>     queues.  
> > 
> > This needs to be properly documented, and the admin needs to have a
> > chance to find out why unbinding didn't work and what needs to be done
> > (see my comments above).  
> 
> I will create a section in the vfio-ap.txt document that comes with this
> patch set describing the proper procedure for unbinding queues. Of
> course, we'll make sure the official IBM doc also more thoroughly
> describes this.

+1 for good documentation.

With that, I don't really object to this change.

Anthony Krowiak July 9, 2019, 9:11 p.m. UTC | #11

On 7/9/19 6:49 AM, Cornelia Huck wrote:
> On Mon, 8 Jul 2019 10:27:11 -0400
> Tony Krowiak <[e-mail]> wrote:
> 
>> On 7/1/19 3:26 PM, Cornelia Huck wrote:
>>> On Wed, 19 Jun 2019 09:04:18 -0400
>>> Tony Krowiak <[e-mail]> wrote:
> 
>>>> Allow me to first address your fear that a bad actor can hog
>>>> resources that can't be removed by root. With this enhancement,
>>>> there is nothing preventing a root user from taking resources
>>>> from a matrix mdev, it simply forces him/her to follow the
>>>> proper procedure. The resources to be removed must first be
>>>> unassigned from the matrix mdev to which they are assigned.
>>>> The AP bus's /sys/bus/ap/apmask and /sys/bus/ap/aqmask
>>>> sysfs attributes can then be edited to transfer ownership
>>>> of the resources to zcrypt.
>>>
>>> What is the suggested procedure when root wants to unbind a queue
>>> device? Find the mdev using the queue (is that easy enough?), unassign
>>> it, then unbind? Failing to unbind is a bit unexpected; can we point
>>> the admin to the correct mdev from which the queue has to be removed
>>> first?
>>
>> The proper procedure is to first unassign the adapter, domain, or both
>> from the mdev to which the APQN is assigned. The difficulty in finding
>> the queue depends upon how many mdevs have been created. I would expect
>> that an admin would keep records of who owns what, but in the case he or
>> she doesn't, it would be a matter of printing out the matrix attribute
>> of each mdev until you find the mdev to which the APQN is assigned.
> 
> Ok, so the information is basically available, if needed.
> 
>> The only means I know of for informing the admin to which mdev a given
>> APQN is assigned is to log the error when it occurs.
> 
> That might be helpful, if it's easy to do.
> 
>> I think Matt is
>> also looking to provide query functions in the management tool on which
>> he is currently working.
> 
> That also sounds helpful.
> 
> (...)
> 
>>>> * It forces the use of the proper procedure to change ownership of AP
>>>>      queues.
>>>
>>> This needs to be properly documented, and the admin needs to have a
>>> chance to find out why unbinding didn't work and what needs to be done
>>> (see my comments above).
>>
>> I will create a section in the vfio-ap.txt document that comes with this
>> patch set describing the proper procedure for unbinding queues. Of
>> course, we'll make sure the official IBM doc also more thoroughly
>> describes this.
> 
> +1 for good documentation.
> 
> With that, I don't really object to this change.

Then I will make the suggested changes and post v5 to the list.

>

Patch

diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index b9fc502c58c2..1b06f47d0d1c 100644
--- a/drivers/s390/crypto/ap_bus.c
+++ b/drivers/s390/crypto/ap_bus.c
@@ -35,6 +35,7 @@ 
 #include <linux/mod_devicetable.h>
 #include <linux/debugfs.h>
 #include <linux/ctype.h>
+#include <linux/module.h>
 
 #include "ap_bus.h"
 #include "ap_debug.h"
@@ -998,9 +999,11 @@  int ap_parse_mask_str(const char *str,
 	newmap = kmalloc(size, GFP_KERNEL);
 	if (!newmap)
 		return -ENOMEM;
-	if (mutex_lock_interruptible(lock)) {
-		kfree(newmap);
-		return -ERESTARTSYS;
+	if (lock) {
+		if (mutex_lock_interruptible(lock)) {
+			kfree(newmap);
+			return -ERESTARTSYS;
+		}
 	}
 
 	if (*str == '+' || *str == '-') {
@@ -1012,7 +1015,10 @@  int ap_parse_mask_str(const char *str,
 	}
 	if (rc == 0)
 		memcpy(bitmap, newmap, size);
-	mutex_unlock(lock);
+
+	if (lock)
+		mutex_unlock(lock);
+
 	kfree(newmap);
 	return rc;
 }
@@ -1199,12 +1205,72 @@  static ssize_t apmask_show(struct bus_type *bus, char *buf)
 	return rc;
 }
 
+int __verify_card_reservations(struct device_driver *drv, void *data)
+{
+	int rc = 0;
+	struct ap_driver *ap_drv = to_ap_drv(drv);
+	unsigned long *newapm = (unsigned long *)data;
+
+	/*
+	 * If the reserved bits do not identify cards reserved for use by the
+	 * non-default driver, there is no need to verify the driver is using
+	 * the queues.
+	 */
+	if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
+		return 0;
+
+	/* The non-default driver's module must be loaded */
+	if (!try_module_get(drv->owner))
+		return 0;
+
+	if (ap_drv->in_use)
+		if (ap_drv->in_use(newapm, ap_perms.aqm))
+			rc = -EADDRINUSE;
+
+	module_put(drv->owner);
+
+	return rc;
+}
+
+static int apmask_commit(unsigned long *newapm)
+{
+	int rc;
+	unsigned long reserved[BITS_TO_LONGS(AP_DEVICES)];
+
+	/*
+	 * Check if any bits in the apmask have been set which will
+	 * result in queues being removed from non-default drivers
+	 */
+	if (bitmap_andnot(reserved, newapm, ap_perms.apm, AP_DEVICES)) {
+		rc = (bus_for_each_drv(&ap_bus_type, NULL, reserved,
+				       __verify_card_reservations));
+		if (rc)
+			return rc;
+	}
+
+	memcpy(ap_perms.apm, newapm, APMASKSIZE);
+
+	return 0;
+}
+
 static ssize_t apmask_store(struct bus_type *bus, const char *buf,
 			    size_t count)
 {
 	int rc;
+	unsigned long newapm[BITS_TO_LONGS(AP_DEVICES)];
+
+	memcpy(newapm, ap_perms.apm, APMASKSIZE);
+
+	rc = ap_parse_mask_str(buf, newapm, AP_DEVICES, NULL);
+	if (rc)
+		return rc;
+
+	if (mutex_lock_interruptible(&ap_perms_mutex))
+		return -ERESTARTSYS;
+
+	rc = apmask_commit(newapm);
+	mutex_unlock(&ap_perms_mutex);
 
-	rc = ap_parse_mask_str(buf, ap_perms.apm, AP_DEVICES, &ap_perms_mutex);
 	if (rc)
 		return rc;
 
@@ -1230,12 +1296,72 @@  static ssize_t aqmask_show(struct bus_type *bus, char *buf)
 	return rc;
 }
 
+int __verify_queue_reservations(struct device_driver *drv, void *data)
+{
+	int rc = 0;
+	struct ap_driver *ap_drv = to_ap_drv(drv);
+	unsigned long *newaqm = (unsigned long *)data;
+
+	/*
+	 * If the reserved bits do not identify queues reserved for use by the
+	 * non-default driver, there is no need to verify the driver is using
+	 * the queues.
+	 */
+	if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
+		return 0;
+
+	/* The non-default driver's module must be loaded */
+	if (!try_module_get(drv->owner))
+		return 0;
+
+	if (ap_drv->in_use)
+		if (ap_drv->in_use(ap_perms.apm, newaqm))
+			rc = -EADDRINUSE;
+
+	module_put(drv->owner);
+
+	return rc;
+}
+
+static int aqmask_commit(unsigned long *newaqm)
+{
+	int rc;
+	unsigned long reserved[BITS_TO_LONGS(AP_DOMAINS)];
+
+	/*
+	 * Check if any bits in the aqmask have been set which will
+	 * result in queues being removed from non-default drivers
+	 */
+	if (bitmap_andnot(reserved, newaqm, ap_perms.aqm, AP_DOMAINS)) {
+		rc = (bus_for_each_drv(&ap_bus_type, NULL, reserved,
+				       __verify_queue_reservations));
+		if (rc)
+			return rc;
+	}
+
+	memcpy(ap_perms.aqm, newaqm, APMASKSIZE);
+
+	return 0;
+}
+
 static ssize_t aqmask_store(struct bus_type *bus, const char *buf,
 			    size_t count)
 {
 	int rc;
+	unsigned long newaqm[BITS_TO_LONGS(AP_DEVICES)];
+
+	memcpy(newaqm, ap_perms.aqm, AQMASKSIZE);
+
+	rc = ap_parse_mask_str(buf, newaqm, AP_DOMAINS, NULL);
+	if (rc)
+		return rc;
+
+	if (mutex_lock_interruptible(&ap_perms_mutex))
+		return -ERESTARTSYS;
+
+	rc = aqmask_commit(newaqm);
+	mutex_unlock(&ap_perms_mutex);
 
-	rc = ap_parse_mask_str(buf, ap_perms.aqm, AP_DOMAINS, &ap_perms_mutex);
 	if (rc)
 		return rc;
 
diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
index 6f3cf37776ca..0f865c5545f2 100644
--- a/drivers/s390/crypto/ap_bus.h
+++ b/drivers/s390/crypto/ap_bus.h
@@ -137,6 +137,7 @@  struct ap_driver {
 	void (*remove)(struct ap_device *);
 	void (*suspend)(struct ap_device *);
 	void (*resume)(struct ap_device *);
+	bool (*in_use)(unsigned long *apm, unsigned long *aqm);
 };
 
 #define to_ap_drv(x) container_of((x), struct ap_driver, driver)
@@ -265,6 +266,8 @@  void ap_queue_reinit_state(struct ap_queue *aq);
 struct ap_card *ap_card_create(int id, int queue_depth, int raw_device_type,
 			       int comp_device_type, unsigned int functions);
 
+#define APMASKSIZE (BITS_TO_LONGS(AP_DEVICES) * sizeof(unsigned long))
+#define AQMASKSIZE (BITS_TO_LONGS(AP_DOMAINS) * sizeof(unsigned long))
 struct ap_perms {
 	unsigned long ioctlm[BITS_TO_LONGS(AP_IOCTLS)];
 	unsigned long apm[BITS_TO_LONGS(AP_DEVICES)];