diff mbox series

[1/7] s390: zcrypt: driver callback to indicate resource in use

Message ID 1555016604-2008-2-git-send-email-akrowiak@linux.ibm.com
State New, archived
Headers show
Series s390: vfio-ap: dynamic configuration support | expand

Commit Message

Tony Krowiak April 11, 2019, 9:03 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 <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
 drivers/s390/crypto/ap_bus.h |   3 +
 2 files changed, 135 insertions(+), 6 deletions(-)

Comments

Harald Freudenberger April 12, 2019, 6:54 a.m. UTC | #1
On 11.04.19 23:03, 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 <akrowiak@linux.ibm.com>

Hello Tony

you are going out with your patches but ... what is the result of the discussions
we had in the past ? Do we have a common understanding that we want to have
it this way ? A driver which is able to claim resources and the bus code
has lower precedence ?

> ---
>  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 1546389d71db..66a5a9d9fae6 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"
> @@ -980,9 +981,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 == '-') {
> @@ -994,7 +997,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;
>  }
> @@ -1181,12 +1187,72 @@ static ssize_t apmask_show(struct bus_type *bus, char *buf)
>  	return rc;
>  }
>  
> +static 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;
> +
> +	/* Pin the driver's module code */
> +	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;
>  
> @@ -1212,12 +1278,72 @@ static ssize_t aqmask_show(struct bus_type *bus, char *buf)
>  	return rc;
>  }
>  
> +static 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;
> +
> +	/* Pin the driver's module code */
> +	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_DOMAINS)];
> +
> +	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 15a98a673c5c..b00952d5fdbb 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)
> @@ -262,6 +263,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)];
Cornelia Huck April 12, 2019, 9:43 a.m. UTC | #2
On Fri, 12 Apr 2019 08:54:54 +0200
Harald Freudenberger <freude@linux.ibm.com> wrote:

> On 11.04.19 23:03, 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 <akrowiak@linux.ibm.com>  
> 
> Hello Tony
> 
> you are going out with your patches but ... what is the result of the discussions
> we had in the past ? Do we have a common understanding that we want to have
> it this way ? A driver which is able to claim resources and the bus code
> has lower precedence ?

I don't know about previous discussions, but I'm curious how you
arrived at this design. A driver being able to override the bus code
seems odd. Restricting this to 'non-default' drivers looks even more
odd.

What is this trying to solve? Traditionally, root has been able to
shoot any appendages of their choice. I would rather expect that in a
supported setup you'd have some management software keeping track of
device usage and making sure that only unused queues can be unbound. Do
we need to export more information to user space so that management
software can make better choices?

> 
> > ---
> >  drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
> >  drivers/s390/crypto/ap_bus.h |   3 +
> >  2 files changed, 135 insertions(+), 6 deletions(-)
Tony Krowiak April 12, 2019, 7:38 p.m. UTC | #3
On 4/12/19 5:43 AM, Cornelia Huck wrote:
> On Fri, 12 Apr 2019 08:54:54 +0200
> Harald Freudenberger <freude@linux.ibm.com> wrote:
> 
>> On 11.04.19 23:03, 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 <akrowiak@linux.ibm.com>
>>
>> Hello Tony
>>
>> you are going out with your patches but ... what is the result of the discussions
>> we had in the past ? Do we have a common understanding that we want to have
>> it this way ? A driver which is able to claim resources and the bus code
>> has lower precedence ?

This is what Reinhard suggested and what we agreed to as a team quite
some time ago. I submitted three versions of this patch to our
internal mailing list, all of which you reviewed, so I'm not sure
why you are surprised by this now.

> 
> I don't know about previous discussions, but I'm curious how you
> arrived at this design. A driver being able to override the bus code
> seems odd. Restricting this to 'non-default' drivers looks even more
> odd.
> 
> What is this trying to solve? Traditionally, root has been able to
> shoot any appendages of their choice. I would rather expect that in a
> supported setup you'd have some management software keeping track of
> device usage and making sure that only unused queues can be unbound. Do
> we need to export more information to user space so that management
> software can make better choices?

Is there a reason other than tradition to prevent root from accidentally
shooting himself in the foot or any other appendage? At present,
sysfs is the only supported setup, so IMHO it makes sense to prevent as
much accidentally caused damage as possible in the kernel.

> 
>>
>>> ---
>>>   drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
>>>   drivers/s390/crypto/ap_bus.h |   3 +
>>>   2 files changed, 135 insertions(+), 6 deletions(-)
>
Cornelia Huck April 15, 2019, 9:50 a.m. UTC | #4
On Fri, 12 Apr 2019 15:38:21 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 4/12/19 5:43 AM, Cornelia Huck wrote:
> > On Fri, 12 Apr 2019 08:54:54 +0200
> > Harald Freudenberger <freude@linux.ibm.com> wrote:
> >   
> >> On 11.04.19 23:03, 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 <akrowiak@linux.ibm.com>  
> >>
> >> Hello Tony
> >>
> >> you are going out with your patches but ... what is the result of the discussions
> >> we had in the past ? Do we have a common understanding that we want to have
> >> it this way ? A driver which is able to claim resources and the bus code
> >> has lower precedence ?  
> 
> This is what Reinhard suggested and what we agreed to as a team quite
> some time ago. I submitted three versions of this patch to our
> internal mailing list, all of which you reviewed, so I'm not sure
> why you are surprised by this now.
> 
> > 
> > I don't know about previous discussions, but I'm curious how you
> > arrived at this design. A driver being able to override the bus code
> > seems odd. Restricting this to 'non-default' drivers looks even more
> > odd.
> > 
> > What is this trying to solve? Traditionally, root has been able to
> > shoot any appendages of their choice. I would rather expect that in a
> > supported setup you'd have some management software keeping track of
> > device usage and making sure that only unused queues can be unbound. Do
> > we need to export more information to user space so that management
> > software can make better choices?  
> 
> Is there a reason other than tradition to prevent root from accidentally
> shooting himself in the foot or any other appendage? At present,
> sysfs is the only supported setup, so IMHO it makes sense to prevent as
> much accidentally caused damage as possible in the kernel.

I don't think anybody wants an interface where it is easy for root to
accidentally cause damage... but from the patch description, it sounds
like you're creating an interface which makes it easy for a
badly-acting driver to hog resources without any way for root to remove
them forcefully.

Therefore, again my question: What is this trying to solve? I see a
management layer as a better place to make sure that queues are not
accidentally yanked from guests that are using them. Does more
information about queue usage need to be made available to userspace
for this to be feasible? Is anything else missing?

> 
> >   
> >>  
> >>> ---
> >>>   drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
> >>>   drivers/s390/crypto/ap_bus.h |   3 +
> >>>   2 files changed, 135 insertions(+), 6 deletions(-)  
> >   
>
Tony Krowiak April 15, 2019, 4:51 p.m. UTC | #5
On 4/15/19 5:50 AM, Cornelia Huck wrote:
> On Fri, 12 Apr 2019 15:38:21 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> On 4/12/19 5:43 AM, Cornelia Huck wrote:
>>> On Fri, 12 Apr 2019 08:54:54 +0200
>>> Harald Freudenberger <freude@linux.ibm.com> wrote:
>>>    
>>>> On 11.04.19 23:03, 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 <akrowiak@linux.ibm.com>
>>>>
>>>> Hello Tony
>>>>
>>>> you are going out with your patches but ... what is the result of the discussions
>>>> we had in the past ? Do we have a common understanding that we want to have
>>>> it this way ? A driver which is able to claim resources and the bus code
>>>> has lower precedence ?
>>
>> This is what Reinhard suggested and what we agreed to as a team quite
>> some time ago. I submitted three versions of this patch to our
>> internal mailing list, all of which you reviewed, so I'm not sure
>> why you are surprised by this now.
>>
>>>
>>> I don't know about previous discussions, but I'm curious how you
>>> arrived at this design. A driver being able to override the bus code
>>> seems odd. Restricting this to 'non-default' drivers looks even more
>>> odd.
>>>
>>> What is this trying to solve? Traditionally, root has been able to
>>> shoot any appendages of their choice. I would rather expect that in a
>>> supported setup you'd have some management software keeping track of
>>> device usage and making sure that only unused queues can be unbound. Do
>>> we need to export more information to user space so that management
>>> software can make better choices?
>>
>> Is there a reason other than tradition to prevent root from accidentally
>> shooting himself in the foot or any other appendage? At present,
>> sysfs is the only supported setup, so IMHO it makes sense to prevent as
>> much accidentally caused damage as possible in the kernel.
> 
> I don't think anybody wants an interface where it is easy for root to
> accidentally cause damage... but from the patch description, it sounds
> like you're creating an interface which makes it easy for a
> badly-acting driver to hog resources without any way for root to remove
> them forcefully.
> 
> Therefore, again my question: What is this trying to solve? I see a
> management layer as a better place to make sure that queues are not
> accidentally yanked from guests that are using them. Does more
> information about queue usage need to be made available to userspace
> for this to be feasible? Is anything else missing?

Accidentally yanking queues from guests is only part of the equation.
One has to consider the case where queues go away without root user
intervention. Take, for example, the case where an AP card temporarily
goes offline for some reason; possibly, due to glich or temporary
hardware malfunction of some sort. When the AP bus scan subsequently
runs, the card device and all associated queue devices will be unbound
from their respective device drivers. If the card then comes back
online, the next bus scan will recreate the card and queue devices and
bind them to their respective device drivers. These patches ensure the
queues are given back to the guest from which they were taken when
unbound.

Having said that, I understand your concern about a driver hogging
resources. I think I can provide a solution that serves both the
purpose of preventing problems associated with accidental removal
of AP resources as well as allowing root to remove them
forcefully. I'll work on that for v2.

> 
>>
>>>    
>>>>   
>>>>> ---
>>>>>    drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
>>>>>    drivers/s390/crypto/ap_bus.h |   3 +
>>>>>    2 files changed, 135 insertions(+), 6 deletions(-)
>>>    
>>
>
Cornelia Huck April 15, 2019, 5:02 p.m. UTC | #6
On Mon, 15 Apr 2019 12:51:23 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 4/15/19 5:50 AM, Cornelia Huck wrote:
> > On Fri, 12 Apr 2019 15:38:21 -0400
> > Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >   
> >> On 4/12/19 5:43 AM, Cornelia Huck wrote:  
> >>> On Fri, 12 Apr 2019 08:54:54 +0200
> >>> Harald Freudenberger <freude@linux.ibm.com> wrote:
> >>>      
> >>>> On 11.04.19 23:03, 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 <akrowiak@linux.ibm.com>  
> >>>>
> >>>> Hello Tony
> >>>>
> >>>> you are going out with your patches but ... what is the result of the discussions
> >>>> we had in the past ? Do we have a common understanding that we want to have
> >>>> it this way ? A driver which is able to claim resources and the bus code
> >>>> has lower precedence ?  
> >>
> >> This is what Reinhard suggested and what we agreed to as a team quite
> >> some time ago. I submitted three versions of this patch to our
> >> internal mailing list, all of which you reviewed, so I'm not sure
> >> why you are surprised by this now.
> >>  
> >>>
> >>> I don't know about previous discussions, but I'm curious how you
> >>> arrived at this design. A driver being able to override the bus code
> >>> seems odd. Restricting this to 'non-default' drivers looks even more
> >>> odd.
> >>>
> >>> What is this trying to solve? Traditionally, root has been able to
> >>> shoot any appendages of their choice. I would rather expect that in a
> >>> supported setup you'd have some management software keeping track of
> >>> device usage and making sure that only unused queues can be unbound. Do
> >>> we need to export more information to user space so that management
> >>> software can make better choices?  
> >>
> >> Is there a reason other than tradition to prevent root from accidentally
> >> shooting himself in the foot or any other appendage? At present,
> >> sysfs is the only supported setup, so IMHO it makes sense to prevent as
> >> much accidentally caused damage as possible in the kernel.  
> > 
> > I don't think anybody wants an interface where it is easy for root to
> > accidentally cause damage... but from the patch description, it sounds
> > like you're creating an interface which makes it easy for a
> > badly-acting driver to hog resources without any way for root to remove
> > them forcefully.
> > 
> > Therefore, again my question: What is this trying to solve? I see a
> > management layer as a better place to make sure that queues are not
> > accidentally yanked from guests that are using them. Does more
> > information about queue usage need to be made available to userspace
> > for this to be feasible? Is anything else missing?  
> 
> Accidentally yanking queues from guests is only part of the equation.
> One has to consider the case where queues go away without root user
> intervention. Take, for example, the case where an AP card temporarily
> goes offline for some reason; possibly, due to glich or temporary
> hardware malfunction of some sort. When the AP bus scan subsequently
> runs, the card device and all associated queue devices will be unbound
> from their respective device drivers. If the card then comes back
> online, the next bus scan will recreate the card and queue devices and
> bind them to their respective device drivers. These patches ensure the
> queues are given back to the guest from which they were taken when
> unbound.

Ok, that sounds a bit like the 'disconnected' state for ccw devices,
and this sounds more useful to me than trying to prevent root from
removing devices. (Put that explanation in the patch description,
maybe?)

> 
> Having said that, I understand your concern about a driver hogging
> resources. I think I can provide a solution that serves both the
> purpose of preventing problems associated with accidental removal
> of AP resources as well as allowing root to remove them
> forcefully. I'll work on that for v2.

Ok, that and the explanation above makes this approach a lot more
reasonable.

> 
> >   
> >>  
> >>>      
> >>>>     
> >>>>> ---
> >>>>>    drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
> >>>>>    drivers/s390/crypto/ap_bus.h |   3 +
> >>>>>    2 files changed, 135 insertions(+), 6 deletions(-)  
> >>>      
> >>  
> >   
>
Halil Pasic April 15, 2019, 6:59 p.m. UTC | #7
On Mon, 15 Apr 2019 12:51:23 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> Having said that, I understand your concern about a driver hogging
> resources. I think I can provide a solution that serves both the
> purpose of preventing problems associated with accidental removal
> of AP resources as well as allowing root to remove them
> forcefully. I'll work on that for v2.

Can you tell us some more about this solution? Should we stop reviewing
v1 because v2 is going to be different anyway?

Regards,
Halil
Tony Krowiak April 15, 2019, 10:43 p.m. UTC | #8
On 4/15/19 2:59 PM, Halil Pasic wrote:
> On Mon, 15 Apr 2019 12:51:23 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> Having said that, I understand your concern about a driver hogging
>> resources. I think I can provide a solution that serves both the
>> purpose of preventing problems associated with accidental removal
>> of AP resources as well as allowing root to remove them
>> forcefully. I'll work on that for v2.
> 
> Can you tell us some more about this solution? Should we stop reviewing
> v1 because v2 is going to be different anyway?

Patch 1 and 2 will be removed. There will not be a major design change
between these patches and v2. In order to avoid a long explanation of
my proposed changes, I'd prefer to state that the patch set will 
establish and enforce the following rules:

     1. An APQN can be assigned to an mdev device iff it is NOT
        reserved for use by a zcrypt driver and is not assigned to
        another mdev device.

     2. Once an APQN is assigned to an mdev device, it will remain
        assigned until it is explicitly unassigned.

     3. A queue's APQN can be set in the guest's CRYCB iff the APQN is
        assigned to the mdev device used by the guest; however, if the
        queue is also in the host configuration (i.e., online), it MUST
        also be bound to the vfio_ap device driver.

     4. When a queue is bound to the vfio_ap driver and its APQN
        is assigned to an mdev device in use by a guest, the guest will
        be given access to the queue.

     5. When a queue is unbound from the vfio_ap driver and its APQN
        is assigned to an mdev device in use by the guest, access to
        the card containing the queue will be removed from the guest.
        Keep in mind that we can not deny access to a specific queue
        due to the architecture (i.e., clearing a bit in the AQM
        removes access to the queue for all adapters)

     6. When an adapter is assigned to an mdev device that is in use
        by a guest, the guest will be given access to the adapter.

     7. When an adapter is unassigned from an mdev device that is in use
        by a guest, access to the adapter will removed from the guest.

     8. When a domain is assigned to an mdev device that is in use
        by a guest, the guest will be given access to the domain.

     9. When a domain is unassigned from an mdev device that is in use
        by a guest, access to the domain will removed from the guest.

> 
> Regards,
> Halil
>
Pierre Morel April 16, 2019, 7:52 a.m. UTC | #9
On 11/04/2019 23:03, 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 <akrowiak@linux.ibm.com>
> ---
>   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 1546389d71db..66a5a9d9fae6 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"
> @@ -980,9 +981,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 == '-') {
> @@ -994,7 +997,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;
>   }
> @@ -1181,12 +1187,72 @@ static ssize_t apmask_show(struct bus_type *bus, char *buf)
>   	return rc;
>   }
>   
> +static 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;

I prefer you suppress this asymmetry with the assumption that the 
"default driver" will not register a "in_use" callback.


> +
> +	/* Pin the driver's module code */
> +	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;
> +}
> +
Tony Krowiak April 16, 2019, 1:11 p.m. UTC | #10
On 4/16/19 3:52 AM, Pierre Morel wrote:
> On 11/04/2019 23:03, 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 <akrowiak@linux.ibm.com>
>> ---
>>   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 1546389d71db..66a5a9d9fae6 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"
>> @@ -980,9 +981,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 == '-') {
>> @@ -994,7 +997,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;
>>   }
>> @@ -1181,12 +1187,72 @@ static ssize_t apmask_show(struct bus_type 
>> *bus, char *buf)
>>       return rc;
>>   }
>> +static 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;
> 
> I prefer you suppress this asymmetry with the assumption that the 
> "default driver" will not register a "in_use" callback.

Based on comments by Connie, I plan on removing this patch from the
next version.

> 
> 
>> +
>> +    /* Pin the driver's module code */
>> +    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;
>> +}
>> +
>
Pierre Morel April 16, 2019, 1:13 p.m. UTC | #11
On 16/04/2019 15:11, Tony Krowiak wrote:
> On 4/16/19 3:52 AM, Pierre Morel wrote:
>> On 11/04/2019 23:03, 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 <akrowiak@linux.ibm.com>
>>> ---
>>>   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 1546389d71db..66a5a9d9fae6 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"
>>> @@ -980,9 +981,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 == '-') {
>>> @@ -994,7 +997,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;
>>>   }
>>> @@ -1181,12 +1187,72 @@ static ssize_t apmask_show(struct bus_type 
>>> *bus, char *buf)
>>>       return rc;
>>>   }
>>> +static 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;
>>
>> I prefer you suppress this asymmetry with the assumption that the 
>> "default driver" will not register a "in_use" callback.
> 
> Based on comments by Connie, I plan on removing this patch from the
> next version.

Yes it was the goal.

> 
>>
>>
>>> +
>>> +    /* Pin the driver's module code */
>>> +    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;
>>> +}
>>> +
>>
>
Halil Pasic April 17, 2019, 3:37 p.m. UTC | #12
On Mon, 15 Apr 2019 18:43:24 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 4/15/19 2:59 PM, Halil Pasic wrote:
> > On Mon, 15 Apr 2019 12:51:23 -0400
> > Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> > 
> >> Having said that, I understand your concern about a driver hogging
> >> resources. I think I can provide a solution that serves both the
> >> purpose of preventing problems associated with accidental removal
> >> of AP resources as well as allowing root to remove them
> >> forcefully. I'll work on that for v2.
> > 
> > Can you tell us some more about this solution? Should we stop reviewing
> > v1 because v2 is going to be different anyway?
> 
> Patch 1 and 2 will be removed. There will not be a major design change
> between these patches and v2. In order to avoid a long explanation of
> my proposed changes, I'd prefer to state that the patch set will 
> establish and enforce the following rules:
> 
>      1. An APQN can be assigned to an mdev device iff it is NOT
>         reserved for use by a zcrypt driver and is not assigned to
>         another mdev device.
> 
>      2. Once an APQN is assigned to an mdev device, it will remain
>         assigned until it is explicitly unassigned.
> 
>      3. A queue's APQN can be set in the guest's CRYCB iff the APQN is
>         assigned to the mdev device used by the guest; however, if the
>         queue is also in the host configuration (i.e., online), it MUST
>         also be bound to the vfio_ap device driver.
> 
>      4. When a queue is bound to the vfio_ap driver and its APQN
>         is assigned to an mdev device in use by a guest, the guest will
>         be given access to the queue.
> 
>      5. When a queue is unbound from the vfio_ap driver and its APQN
>         is assigned to an mdev device in use by the guest, access to
>         the card containing the queue will be removed from the guest.
>         Keep in mind that we can not deny access to a specific queue
>         due to the architecture (i.e., clearing a bit in the AQM
>         removes access to the queue for all adapters)
> 
>      6. When an adapter is assigned to an mdev device that is in use
>         by a guest, the guest will be given access to the adapter.
> 
>      7. When an adapter is unassigned from an mdev device that is in use
>         by a guest, access to the adapter will removed from the guest.
> 
>      8. When a domain is assigned to an mdev device that is in use
>         by a guest, the guest will be given access to the domain.
> 
>      9. When a domain is unassigned from an mdev device that is in use
>         by a guest, access to the domain will removed from the guest.
> 

Based on our off-the-list chat and this list I think I know
where are you heading :). I think it's actually the design that I
currently prefer the most. But in that case, it may be wise to touch
base with Reinhard -- AFAIR he was the strongest proponent of the 'do
not let a[pq]mask changes take away queues from guests' design.

Regards,
Halil
diff mbox series

Patch

diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index 1546389d71db..66a5a9d9fae6 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"
@@ -980,9 +981,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 == '-') {
@@ -994,7 +997,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;
 }
@@ -1181,12 +1187,72 @@  static ssize_t apmask_show(struct bus_type *bus, char *buf)
 	return rc;
 }
 
+static 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;
+
+	/* Pin the driver's module code */
+	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;
 
@@ -1212,12 +1278,72 @@  static ssize_t aqmask_show(struct bus_type *bus, char *buf)
 	return rc;
 }
 
+static 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;
+
+	/* Pin the driver's module code */
+	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_DOMAINS)];
+
+	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 15a98a673c5c..b00952d5fdbb 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)
@@ -262,6 +263,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)];