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

From: Pierre Morel
Date: Tue Apr 16 2019 - 09:14:11 EST


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@xxxxxxxxxxxxx>
---
 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;
+}
+




--
Pierre Morel
Linux/KVM/QEMU in BÃblingen - Germany