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

[Pierre Morel]

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