Messages in this thread | | | Subject | Re: [PATCH v4 3/7] s390: zcrypt: driver callback to indicate resource in use | From | Tony Krowiak <> | Date | Mon, 17 Jun 2019 10:37:07 -0400 |
| |
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)]; >
| |