KVM Archive on lore.kernel.org
 help / color / Atom feed
From: Tony Krowiak <akrowiak@linux.ibm.com>
To: Harald Freudenberger <freude@linux.ibm.com>,
	linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org
Cc: borntraeger@de.ibm.com, cohuck@redhat.com, frankja@linux.ibm.com,
	david@redhat.com, mjrosato@linux.ibm.com, schwidefsky@de.ibm.com,
	heiko.carstens@de.ibm.com, pmorel@linux.ibm.com,
	pasic@linux.ibm.com, alex.williamson@redhat.com,
	kwankhede@nvidia.com
Subject: Re: [PATCH v4 3/7] s390: zcrypt: driver callback to indicate resource in use
Date: Mon, 17 Jun 2019 10:37:07 -0400
Message-ID: <e96b1293-0086-b133-4a1a-ec363d3f0f0a@linux.ibm.com> (raw)
In-Reply-To: <c5e9bde9-4a0e-8ad8-d5d4-df852a33e232@linux.ibm.com>

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


  reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-13 19:39 [PATCH v4 0/7] s390: vfio-ap: dynamic configuration support Tony Krowiak
2019-06-13 19:39 ` [PATCH v4 1/7] s390: vfio-ap: Refactor vfio_ap driver probe and remove callbacks Tony Krowiak
2019-06-17  8:27   ` Harald Freudenberger
2019-06-17 14:24     ` Tony Krowiak
2019-06-18 16:14   ` Cornelia Huck
2019-06-19 12:31     ` Tony Krowiak
2019-06-13 19:39 ` [PATCH v4 2/7] s390: vfio-ap: wait for queue empty on queue reset Tony Krowiak
2019-06-17  8:47   ` Harald Freudenberger
2019-06-17 14:29     ` Tony Krowiak
2019-06-13 19:39 ` [PATCH v4 3/7] s390: zcrypt: driver callback to indicate resource in use Tony Krowiak
2019-06-17  9:28   ` Harald Freudenberger
2019-06-17 14:37     ` Tony Krowiak [this message]
2019-06-18 16:25   ` Cornelia Huck
2019-06-19 13:04     ` Tony Krowiak
2019-06-26 21:13       ` Tony Krowiak
2019-06-27  7:25         ` Cornelia Huck
2019-06-27 12:59           ` Tony Krowiak
2019-07-01 19:26       ` Cornelia Huck
2019-07-08 14:27         ` Tony Krowiak
2019-07-09 10:49           ` Cornelia Huck
2019-07-09 21:11             ` Tony Krowiak
2019-06-13 19:39 ` [PATCH v4 4/7] s390: vfio-ap: implement in-use callback for vfio_ap driver Tony Krowiak
2019-06-13 19:39 ` [PATCH v4 5/7] s390: vfio-ap: allow assignment of unavailable AP resources to mdev device Tony Krowiak
2019-06-17 10:05   ` Harald Freudenberger
2019-06-17 15:07     ` Tony Krowiak
2019-06-18  6:49       ` Harald Freudenberger
2019-06-19 13:39         ` Tony Krowiak
2019-06-13 19:39 ` [PATCH v4 6/7] s390: vfio-ap: allow hot plug/unplug of AP resources using " Tony Krowiak
2019-06-13 19:39 ` [PATCH v4 7/7] s390: vfio-ap: update documentation Tony Krowiak
2019-06-17 11:42   ` Harald Freudenberger
2019-06-17 15:21     ` Tony Krowiak
2019-07-09 15:30 ` [PATCH v4 0/7] s390: vfio-ap: dynamic configuration support Halil Pasic

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e96b1293-0086-b133-4a1a-ec363d3f0f0a@linux.ibm.com \
    --to=akrowiak@linux.ibm.com \
    --cc=alex.williamson@redhat.com \
    --cc=borntraeger@de.ibm.com \
    --cc=cohuck@redhat.com \
    --cc=david@redhat.com \
    --cc=frankja@linux.ibm.com \
    --cc=freude@linux.ibm.com \
    --cc=heiko.carstens@de.ibm.com \
    --cc=kvm@vger.kernel.org \
    --cc=kwankhede@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=mjrosato@linux.ibm.com \
    --cc=pasic@linux.ibm.com \
    --cc=pmorel@linux.ibm.com \
    --cc=schwidefsky@de.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org kvm@archiver.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox