LinuxLists.cc - [PATCH v8 04/16] s390/zcrypt: driver callback to indicate resource in use

2020-06-05 21:44:20

Subject: [PATCH v8 04/16] s390/zcrypt: driver callback to indicate resource in use

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. The intent of
this callback is to provide a driver with the means to prevent a root user
from inadvertently taking a queue away from a matrix mdev and giving it to
the host while it is assigned to the matrix mdev. The callback will
be invoked whenever a change to the AP bus's sysfs apmask or aqmask
attributes would 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 facilitates pass-through of an AP queue to a
guest. The idea here is that a guest may be administered by a different
sysadmin than the host and we don't want AP resources to unexpectedly
disappear from a guest's AP configuration (i.e., adapters, domains and
control domains assigned to the matrix mdev). This will enforce the proper
procedure for removing AP resources intended for guest usage which is to
first unassign them from the matrix mdev, then unbind them from the
vfio_ap device driver.

Signed-off-by: Tony Krowiak <[email protected]>
---
drivers/s390/crypto/ap_bus.c | 148 ++++++++++++++++++++++++++++++++---
drivers/s390/crypto/ap_bus.h | 4 +
2 files changed, 142 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index e71ca4a719a5..40cb5861dad3 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"
@@ -876,6 +877,23 @@ static int modify_bitmap(const char *str, unsigned long *bitmap, int bits)
return 0;
}

+static int ap_parse_bitmap_str(const char *str, unsigned long *bitmap, int bits,
+ unsigned long *newmap)
+{
+ unsigned long size;
+ int rc;
+
+ size = BITS_TO_LONGS(bits)*sizeof(unsigned long);
+ if (*str == '+' || *str == '-') {
+ memcpy(newmap, bitmap, size);
+ rc = modify_bitmap(str, newmap, bits);
+ } else {
+ memset(newmap, 0, size);
+ rc = hex2bitmap(str, newmap, bits);
+ }
+ return rc;
+}
+
int ap_parse_mask_str(const char *str,
unsigned long *bitmap, int bits,
struct mutex *lock)
@@ -895,14 +913,7 @@ int ap_parse_mask_str(const char *str,
kfree(newmap);
return -ERESTARTSYS;
}
-
- if (*str == '+' || *str == '-') {
- memcpy(newmap, bitmap, size);
- rc = modify_bitmap(str, newmap, bits);
- } else {
- memset(newmap, 0, size);
- rc = hex2bitmap(str, newmap, bits);
- }
+ rc = ap_parse_bitmap_str(str, bitmap, bits, newmap);
if (rc == 0)
memcpy(bitmap, newmap, size);
mutex_unlock(lock);
@@ -1092,12 +1103,70 @@ 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;
+
+ /*
+ * No need to verify whether the driver is using the queues if it is the
+ * default driver.
+ */
+ 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;
+ DECLARE_BITMAP(newapm, AP_DEVICES);
+
+ if (mutex_lock_interruptible(&ap_perms_mutex))
+ return -ERESTARTSYS;
+
+ rc = ap_parse_bitmap_str(buf, ap_perms.apm, AP_DEVICES, newapm);
+ if (rc)
+ goto done;

- rc = ap_parse_mask_str(buf, ap_perms.apm, AP_DEVICES, &ap_perms_mutex);
+ rc = apmask_commit(newapm);
+
+done:
+ mutex_unlock(&ap_perms_mutex);
if (rc)
return rc;

@@ -1123,12 +1192,71 @@ 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, AQMASKSIZE);
+
+ return 0;
+}
+
static ssize_t aqmask_store(struct bus_type *bus, const char *buf,
size_t count)
{
int rc;
+ DECLARE_BITMAP(newaqm, AP_DOMAINS);

- rc = ap_parse_mask_str(buf, ap_perms.aqm, AP_DOMAINS, &ap_perms_mutex);
+ if (mutex_lock_interruptible(&ap_perms_mutex))
+ return -ERESTARTSYS;
+
+ rc = ap_parse_bitmap_str(buf, ap_perms.aqm, AP_DOMAINS, newaqm);
+ if (rc)
+ goto done;
+
+ rc = aqmask_commit(newaqm);
+
+done:
+ mutex_unlock(&ap_perms_mutex);
if (rc)
return rc;

diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
index 053cc34d2ca2..7d9646251bfd 100644
--- a/drivers/s390/crypto/ap_bus.h
+++ b/drivers/s390/crypto/ap_bus.h
@@ -136,6 +136,7 @@ struct ap_driver {

int (*probe)(struct ap_device *);
void (*remove)(struct ap_device *);
+ bool (*in_use)(unsigned long *apm, unsigned long *aqm);
};

#define to_ap_drv(x) container_of((x), struct ap_driver, driver)
@@ -254,6 +255,9 @@ void ap_queue_init_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)];
--
2.21.1

2020-06-06 01:40:49

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v8 04/16] s390/zcrypt: driver callback to indicate resource in use

Attachments:

(No filename) (5.50 kB)
.config.gz (57.88 kB)
Download all attachments

2020-06-06 01:43:20

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v8 04/16] s390/zcrypt: driver callback to indicate resource in use

Hi Tony,

I love your patch! Perhaps something to improve:

[auto build test WARNING on kvms390/next]
[also build test WARNING on linus/master v5.7]
[cannot apply to s390/features linux/master next-20200605]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/Tony-Krowiak/s390-vfio-ap-dynamic-configuration-support/20200606-054350
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git next
config: s390-randconfig-r025-20200605 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 6dd738e2f0609f7d3313b574a1d471263d2d3ba1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

include/uapi/linux/swab.h:19:12: note: expanded from macro '___constant_swab32'
(((__u32)(x) & (__u32)0x000000ffUL) << 24) | ^
In file included from drivers/s390/crypto/ap_bus.c:28:
In file included from arch/s390/include/asm/airq.h:14:
In file included from include/linux/dma-mapping.h:11:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:72:
include/asm-generic/io.h:492:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
___constant_swab32(x) : ^
include/uapi/linux/swab.h:20:12: note: expanded from macro '___constant_swab32'
(((__u32)(x) & (__u32)0x0000ff00UL) << 8) | ^
In file included from drivers/s390/crypto/ap_bus.c:28:
In file included from arch/s390/include/asm/airq.h:14:
In file included from include/linux/dma-mapping.h:11:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:72:
include/asm-generic/io.h:492:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
___constant_swab32(x) : ^
include/uapi/linux/swab.h:21:12: note: expanded from macro '___constant_swab32'
(((__u32)(x) & (__u32)0x00ff0000UL) >> 8) | ^
In file included from drivers/s390/crypto/ap_bus.c:28:
In file included from arch/s390/include/asm/airq.h:14:
In file included from include/linux/dma-mapping.h:11:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:72:
include/asm-generic/io.h:492:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
___constant_swab32(x) : ^
include/uapi/linux/swab.h:22:12: note: expanded from macro '___constant_swab32'
(((__u32)(x) & (__u32)0xff000000UL) >> 24)))
^
In file included from drivers/s390/crypto/ap_bus.c:28:
In file included from arch/s390/include/asm/airq.h:14:
In file included from include/linux/dma-mapping.h:11:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:72:
include/asm-generic/io.h:492:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:120:12: note: expanded from macro '__swab32'
__fswab32(x))
^
In file included from drivers/s390/crypto/ap_bus.c:28:
In file included from arch/s390/include/asm/airq.h:14:
In file included from include/linux/dma-mapping.h:11:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:72:
include/asm-generic/io.h:503:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:513:46: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew(cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:523:46: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel(cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:585:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:593:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:601:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:610:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:619:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:628:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
>> drivers/s390/crypto/ap_bus.c:1106:5: warning: no previous prototype for function '__verify_card_reservations' [-Wmissing-prototypes]
int __verify_card_reservations(struct device_driver *drv, void *data)
^
drivers/s390/crypto/ap_bus.c:1106:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int __verify_card_reservations(struct device_driver *drv, void *data)
^
static
>> drivers/s390/crypto/ap_bus.c:1195:5: warning: no previous prototype for function '__verify_queue_reservations' [-Wmissing-prototypes]
int __verify_queue_reservations(struct device_driver *drv, void *data)
^
drivers/s390/crypto/ap_bus.c:1195:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int __verify_queue_reservations(struct device_driver *drv, void *data)
^
static
22 warnings generated.

vim +/__verify_card_reservations +1106 drivers/s390/crypto/ap_bus.c

1105
> 1106 int __verify_card_reservations(struct device_driver *drv, void *data)
1107 {
1108 int rc = 0;
1109 struct ap_driver *ap_drv = to_ap_drv(drv);
1110 unsigned long *newapm = (unsigned long *)data;
1111
1112 /*
1113 * No need to verify whether the driver is using the queues if it is the
1114 * default driver.
1115 */
1116 if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
1117 return 0;
1118
1119 /* The non-default driver's module must be loaded */
1120 if (!try_module_get(drv->owner))
1121 return 0;
1122
1123 if (ap_drv->in_use)
1124 if (ap_drv->in_use(newapm, ap_perms.aqm))
1125 rc = -EADDRINUSE;
1126
1127 module_put(drv->owner);
1128
1129 return rc;
1130 }
1131
1132 static int apmask_commit(unsigned long *newapm)
1133 {
1134 int rc;
1135 unsigned long reserved[BITS_TO_LONGS(AP_DEVICES)];
1136
1137 /*
1138 * Check if any bits in the apmask have been set which will
1139 * result in queues being removed from non-default drivers
1140 */
1141 if (bitmap_andnot(reserved, newapm, ap_perms.apm, AP_DEVICES)) {
1142 rc = bus_for_each_drv(&ap_bus_type, NULL, reserved,
1143 __verify_card_reservations);
1144 if (rc)
1145 return rc;
1146 }
1147
1148 memcpy(ap_perms.apm, newapm, APMASKSIZE);
1149
1150 return 0;
1151 }
1152
1153 static ssize_t apmask_store(struct bus_type *bus, const char *buf,
1154 size_t count)
1155 {
1156 int rc;
1157 DECLARE_BITMAP(newapm, AP_DEVICES);
1158
1159 if (mutex_lock_interruptible(&ap_perms_mutex))
1160 return -ERESTARTSYS;
1161
1162 rc = ap_parse_bitmap_str(buf, ap_perms.apm, AP_DEVICES, newapm);
1163 if (rc)
1164 goto done;
1165
1166 rc = apmask_commit(newapm);
1167
1168 done:
1169 mutex_unlock(&ap_perms_mutex);
1170 if (rc)
1171 return rc;
1172
1173 ap_bus_revise_bindings();
1174
1175 return count;
1176 }
1177
1178 static BUS_ATTR_RW(apmask);
1179
1180 static ssize_t aqmask_show(struct bus_type *bus, char *buf)
1181 {
1182 int rc;
1183
1184 if (mutex_lock_interruptible(&ap_perms_mutex))
1185 return -ERESTARTSYS;
1186 rc = scnprintf(buf, PAGE_SIZE,
1187 "0x%016lx%016lx%016lx%016lx\n",
1188 ap_perms.aqm[0], ap_perms.aqm[1],
1189 ap_perms.aqm[2], ap_perms.aqm[3]);
1190 mutex_unlock(&ap_perms_mutex);
1191
1192 return rc;
1193 }
1194
> 1195 int __verify_queue_reservations(struct device_driver *drv, void *data)
1196 {
1197 int rc = 0;
1198 struct ap_driver *ap_drv = to_ap_drv(drv);
1199 unsigned long *newaqm = (unsigned long *)data;
1200
1201 /*
1202 * If the reserved bits do not identify queues reserved for use by the
1203 * non-default driver, there is no need to verify the driver is using
1204 * the queues.
1205 */
1206 if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
1207 return 0;
1208
1209 /* The non-default driver's module must be loaded */
1210 if (!try_module_get(drv->owner))
1211 return 0;
1212
1213 if (ap_drv->in_use)
1214 if (ap_drv->in_use(ap_perms.apm, newaqm))
1215 rc = -EADDRINUSE;
1216
1217 module_put(drv->owner);
1218
1219 return rc;
1220 }
1221

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (11.48 kB)
.config.gz (26.21 kB)
Download all attachments

2020-07-08 12:30:26

by Christian Borntraeger

[permalink] [raw]

Subject: Re: [PATCH v8 04/16] s390/zcrypt: driver callback to indicate resource in use

On 05.06.20 23: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. The intent of
> this callback is to provide a driver with the means to prevent a root user
> from inadvertently taking a queue away from a matrix mdev and giving it to
> the host while it is assigned to the matrix mdev. The callback will
> be invoked whenever a change to the AP bus's sysfs apmask or aqmask
> attributes would 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.

The alternative would be to tear down the connection in the matrix mdev in this
callback (so that the guest will see a hot unplug), but actually making this
a more conscious decision (requiring 2 steps from the host admin) is certainly
also fine.

>
> 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 facilitates pass-through of an AP queue to a
> guest. The idea here is that a guest may be administered by a different
> sysadmin than the host and we don't want AP resources to unexpectedly
> disappear from a guest's AP configuration (i.e., adapters, domains and
> control domains assigned to the matrix mdev). This will enforce the proper
> procedure for removing AP resources intended for guest usage which is to
> first unassign them from the matrix mdev, then unbind them from the
> vfio_ap device driver.

What I said above, we can force a hot unplug to the guest, but we require
to do 2 steps. I think this is fine.

>
> Signed-off-by: Tony Krowiak <[email protected]>
> ---
> drivers/s390/crypto/ap_bus.c | 148 ++++++++++++++++++++++++++++++++---
> drivers/s390/crypto/ap_bus.h | 4 +
> 2 files changed, 142 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
> index e71ca4a719a5..40cb5861dad3 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"
> @@ -876,6 +877,23 @@ static int modify_bitmap(const char *str, unsigned long *bitmap, int bits)
> return 0;
> }
>
> +static int ap_parse_bitmap_str(const char *str, unsigned long *bitmap, int bits,
> + unsigned long *newmap)
> +{
> + unsigned long size;
> + int rc;
> +
> + size = BITS_TO_LONGS(bits)*sizeof(unsigned long); ^ ^ spaces around the *
> + if (*str == '+' || *str == '-') {
> + memcpy(newmap, bitmap, size);
> + rc = modify_bitmap(str, newmap, bits);
> + } else {
> + memset(newmap, 0, size);
> + rc = hex2bitmap(str, newmap, bits);
> + }
> + return rc;
> +}
> +
> int ap_parse_mask_str(const char *str,
> unsigned long *bitmap, int bits,
> struct mutex *lock)
> @@ -895,14 +913,7 @@ int ap_parse_mask_str(const char *str,
> kfree(newmap);
> return -ERESTARTSYS;
> }
> -
> - if (*str == '+' || *str == '-') {
> - memcpy(newmap, bitmap, size);

Do we still need the size variable in here?

> - rc = modify_bitmap(str, newmap, bits);
> - } else {
> - memset(newmap, 0, size);
> - rc = hex2bitmap(str, newmap, bits);
> - }
> + rc = ap_parse_bitmap_str(str, bitmap, bits, newmap);
> if (rc == 0)
> memcpy(bitmap, newmap, size);
> mutex_unlock(lock);
> @@ -1092,12 +1103,70 @@ 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;
> +
> + /*
> + * No need to verify whether the driver is using the queues if it is the
> + * default driver.
> + */
> + 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;

I think -EBUSY is more appropriate. (also in the other places)

2020-07-08 14:06:51

by Anthony Krowiak

[permalink] [raw]

Subject: Re: [PATCH v8 04/16] s390/zcrypt: driver callback to indicate resource in use

On 7/8/20 8:27 AM, Christian Borntraeger wrote:
> On 05.06.20 23: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. The intent of
>> this callback is to provide a driver with the means to prevent a root user
>> from inadvertently taking a queue away from a matrix mdev and giving it to
>> the host while it is assigned to the matrix mdev. The callback will
>> be invoked whenever a change to the AP bus's sysfs apmask or aqmask
>> attributes would 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.
> The alternative would be to tear down the connection in the matrix mdev in this
> callback (so that the guest will see a hot unplug), but actually making this
> a more conscious decision (requiring 2 steps from the host admin) is certainly
> also fine.

That alternative was considered. Keep in mind that unassigning
an adapter (apmask) or domain (aqmask) may result in multiple APQNs
being removed from one or more matrix mdevs, which could affect
multiple guests. The choice was made to enforce the proper procedure
for taking AP resources away from a guest to prevent accidental or
indiscriminate maladministration.

>
>
>> 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 facilitates pass-through of an AP queue to a
>> guest. The idea here is that a guest may be administered by a different
>> sysadmin than the host and we don't want AP resources to unexpectedly
>> disappear from a guest's AP configuration (i.e., adapters, domains and
>> control domains assigned to the matrix mdev). This will enforce the proper
>> procedure for removing AP resources intended for guest usage which is to
>> first unassign them from the matrix mdev, then unbind them from the
>> vfio_ap device driver.
> What I said above, we can force a hot unplug to the guest, but we require
> to do 2 steps. I think this is fine.
>
>
>> Signed-off-by: Tony Krowiak <[email protected]>
>> ---
>> drivers/s390/crypto/ap_bus.c | 148 ++++++++++++++++++++++++++++++++---
>> drivers/s390/crypto/ap_bus.h | 4 +
>> 2 files changed, 142 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
>> index e71ca4a719a5..40cb5861dad3 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"
>> @@ -876,6 +877,23 @@ static int modify_bitmap(const char *str, unsigned long *bitmap, int bits)
>> return 0;
>> }
>>
>> +static int ap_parse_bitmap_str(const char *str, unsigned long *bitmap, int bits,
>> + unsigned long *newmap)
>> +{
>> + unsigned long size;
>> + int rc;
>> +
>> + size = BITS_TO_LONGS(bits)*sizeof(unsigned long); ^ ^ spaces around the *
>> + if (*str == '+' || *str == '-') {
>> + memcpy(newmap, bitmap, size);
>> + rc = modify_bitmap(str, newmap, bits);
>> + } else {
>> + memset(newmap, 0, size);
>> + rc = hex2bitmap(str, newmap, bits);
>> + }
>> + return rc;
>> +}
>> +
>> int ap_parse_mask_str(const char *str,
>> unsigned long *bitmap, int bits,
>> struct mutex *lock)
>> @@ -895,14 +913,7 @@ int ap_parse_mask_str(const char *str,
>> kfree(newmap);
>> return -ERESTARTSYS;
>> }
>> -
>> - if (*str == '+' || *str == '-') {
>> - memcpy(newmap, bitmap, size);
> Do we still need the size variable in here?
>
>> - rc = modify_bitmap(str, newmap, bits);
>> - } else {
>> - memset(newmap, 0, size);
>> - rc = hex2bitmap(str, newmap, bits);
>> - }
>> + rc = ap_parse_bitmap_str(str, bitmap, bits, newmap);
>> if (rc == 0)
>> memcpy(bitmap, newmap, size);
>> mutex_unlock(lock);
>> @@ -1092,12 +1103,70 @@ 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;
>> +
>> + /*
>> + * No need to verify whether the driver is using the queues if it is the
>> + * default driver.
>> + */
>> + 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;
> I think -EBUSY is more appropriate. (also in the other places)
>