| Message ID | 20200821195616.13554-5-akrowiak@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390/vfio-ap: dynamic configuration support | expand |
On Fri, 21 Aug 2020 15:56:04 -0400 Tony Krowiak <akrowiak@linux.ibm.com> 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. > > 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 <akrowiak@linux.ibm.com> > Reported-by: kernel test robot <lkp@intel.com> This looks a bit odd... > --- > drivers/s390/crypto/ap_bus.c | 148 ++++++++++++++++++++++++++++++++--- > drivers/s390/crypto/ap_bus.h | 4 + > 2 files changed, 142 insertions(+), 10 deletions(-) > (...) > @@ -1107,12 +1118,70 @@ 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; > + > + /* > + * 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; ISTR that Christian suggested -EBUSY in a past revision of this series? I think that would be more appropriate. Also, I know we have discussed this before, but it is very hard to figure out the offending device(s) if the sysfs manipulation failed. Can we at least drop something into the syslog? That would be far from perfect, but it gives an admin at least a chance to figure out why they got an error. Some more structured way that would be usable from tools can still be added later. > + > + module_put(drv->owner); > + > + return rc; > +}
On 9/14/20 11:29 AM, Cornelia Huck wrote: > On Fri, 21 Aug 2020 15:56:04 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> 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. >> >> 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 <akrowiak@linux.ibm.com> >> Reported-by: kernel test robot <lkp@intel.com> > This looks a bit odd... I've removed all of those. These kernel test robot errors were flagged in the last series. The review comments from the robot suggested the reported-by, but I assume that was for patches intended to fix those errors, so I am removing these as per Christian's comments. > >> --- >> drivers/s390/crypto/ap_bus.c | 148 ++++++++++++++++++++++++++++++++--- >> drivers/s390/crypto/ap_bus.h | 4 + >> 2 files changed, 142 insertions(+), 10 deletions(-) >> > (...) > >> @@ -1107,12 +1118,70 @@ 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; >> + >> + /* >> + * 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; > ISTR that Christian suggested -EBUSY in a past revision of this series? > I think that would be more appropriate. I went back and looked and sure enough, he did recommend that. You have a great memory! I didn't respond to that comment, so I must have missed it at the time. I personally prefer EADDRINUSE because I think it is more indicative of the reason an AP resource can not be assigned back to the host drivers is because it is in use by a guest or, at the very least, reserved for use by a guest (i.e., assigned to an mdev). To say it is busy implies that the device is busy performing encryption services which may or may not be true at a given moment. Even if so, that is not the reason for refusing to allow reassignment of the device. > > Also, I know we have discussed this before, but it is very hard to > figure out the offending device(s) if the sysfs manipulation failed. Can > we at least drop something into the syslog? That would be far from > perfect, but it gives an admin at least a chance to figure out why they > got an error. Some more structured way that would be usable from tools > can still be added later. I see you found the patch that logged this:) > >> + >> + module_put(drv->owner); >> + >> + return rc; >> +}
On Tue, 15 Sep 2020 15:32:35 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > On 9/14/20 11:29 AM, Cornelia Huck wrote: > > On Fri, 21 Aug 2020 15:56:04 -0400 > > Tony Krowiak <akrowiak@linux.ibm.com> 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. > >> > >> 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 <akrowiak@linux.ibm.com> > >> Reported-by: kernel test robot <lkp@intel.com> > > This looks a bit odd... > > I've removed all of those. These kernel test robot errors were flagged > in the last series. The review comments from the robot suggested > the reported-by, but I assume that was for patches intended to > fix those errors, so I am removing these as per Christian's comments. Yes, I think the Reported-by: mostly makes sense if you include a patch to fix something on top. > > > > >> --- > >> drivers/s390/crypto/ap_bus.c | 148 ++++++++++++++++++++++++++++++++--- > >> drivers/s390/crypto/ap_bus.h | 4 + > >> 2 files changed, 142 insertions(+), 10 deletions(-) > >> > > (...) > > > >> @@ -1107,12 +1118,70 @@ 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; > >> + > >> + /* > >> + * 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; > > ISTR that Christian suggested -EBUSY in a past revision of this series? > > I think that would be more appropriate. > > I went back and looked and sure enough, he did recommend that. > You have a great memory! I didn't respond to that comment, so I > must have missed it at the time. > > I personally prefer EADDRINUSE because I think it is more indicative > of the reason an AP resource can not be assigned back to the host > drivers is because it is in use by a guest or, at the very least, reserved > for use by a guest (i.e., assigned to an mdev). To say it is busy implies > that the device is busy performing encryption services which may or > may not be true at a given moment. Even if so, that is not the reason > for refusing to allow reassignment of the device. I have a different understanding of these error codes: EADDRINUSE is something used in the networking context when an actual address is already used elsewhere. EBUSY is more of a generic error that indicates that a certain resource is not free to perform the requested operation; it does not necessarily mean that the resource is currently actively doing something. Kind of when you get EBUSY when trying to eject something another program holds a reference on: that other program might not actually be doing anything, but it potentially could.
On 9/17/20 8:14 AM, Cornelia Huck wrote: > On Tue, 15 Sep 2020 15:32:35 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> On 9/14/20 11:29 AM, Cornelia Huck wrote: >>> On Fri, 21 Aug 2020 15:56:04 -0400 >>> Tony Krowiak <akrowiak@linux.ibm.com> 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. >>>> >>>> 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 <akrowiak@linux.ibm.com> >>>> Reported-by: kernel test robot <lkp@intel.com> >>> This looks a bit odd... >> I've removed all of those. These kernel test robot errors were flagged >> in the last series. The review comments from the robot suggested >> the reported-by, but I assume that was for patches intended to >> fix those errors, so I am removing these as per Christian's comments. > Yes, I think the Reported-by: mostly makes sense if you include a patch > to fix something on top. > >>> >>>> --- >>>> drivers/s390/crypto/ap_bus.c | 148 ++++++++++++++++++++++++++++++++--- >>>> drivers/s390/crypto/ap_bus.h | 4 + >>>> 2 files changed, 142 insertions(+), 10 deletions(-) >>>> >>> (...) >>> >>>> @@ -1107,12 +1118,70 @@ 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; >>>> + >>>> + /* >>>> + * 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; >>> ISTR that Christian suggested -EBUSY in a past revision of this series? >>> I think that would be more appropriate. >> I went back and looked and sure enough, he did recommend that. >> You have a great memory! I didn't respond to that comment, so I >> must have missed it at the time. >> >> I personally prefer EADDRINUSE because I think it is more indicative >> of the reason an AP resource can not be assigned back to the host >> drivers is because it is in use by a guest or, at the very least, reserved >> for use by a guest (i.e., assigned to an mdev). To say it is busy implies >> that the device is busy performing encryption services which may or >> may not be true at a given moment. Even if so, that is not the reason >> for refusing to allow reassignment of the device. > I have a different understanding of these error codes: EADDRINUSE is > something used in the networking context when an actual address is > already used elsewhere. EBUSY is more of a generic error that indicates > that a certain resource is not free to perform the requested operation; > it does not necessarily mean that the resource is currently actively > doing something. Kind of when you get EBUSY when trying to eject > something another program holds a reference on: that other program > might not actually be doing anything, but it potentially could. I'll go ahead and change it to -EBUSY. >
On Fri, 21 Aug 2020 15:56:04 -0400 Tony Krowiak <akrowiak@linux.ibm.com> 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. > > 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 <akrowiak@linux.ibm.com> > Reported-by: kernel test robot <lkp@intel.com> > --- > 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 24a1940b829e..db27bd931308 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" > @@ -889,6 +890,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) > @@ -908,14 +926,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); > @@ -1107,12 +1118,70 @@ 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; > + > + /* > + * 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; > + } I understand the above asks all the non-default drivers if some of the queues are 'used'. But AFAIU this reflects the truth ap_drv->in_use() is only telling us something about a given moment... > + > + memcpy(ap_perms.apm, newapm, APMASKSIZE); ... So I fail to understand what will prevent us from performing a successful commit if some of the resources become 'used' between the call to the in_use() callback and the memcpy. Of course I might be wrong. BTW I was never a fan of this mechanism, so I don't mind if it does not work perfectly, and this should catch most of the cases. Just want to make sure we don't introduce more confusion than necessary. > + > + 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; > > @@ -1138,12 +1207,71 @@ static ssize_t aqmask_show(struct bus_type *bus, char *buf) > return rc; > } > > +static 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); > + Same here. Regards, Halil > + 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 1ea046324e8f..48c57b3d53a0 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) > @@ -255,6 +256,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)];
On 9/25/20 5:24 AM, Halil Pasic wrote: > On Fri, 21 Aug 2020 15:56:04 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> 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. >> >> 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 <akrowiak@linux.ibm.com> >> Reported-by: kernel test robot <lkp@intel.com> >> --- >> 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 24a1940b829e..db27bd931308 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" >> @@ -889,6 +890,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) >> @@ -908,14 +926,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); >> @@ -1107,12 +1118,70 @@ 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; >> + >> + /* >> + * 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; >> + } > I understand the above asks all the non-default drivers if some of the > queues are 'used'. But AFAIU this reflects the truth ap_drv->in_use() > is only telling us something about a given moment... > >> + >> + memcpy(ap_perms.apm, newapm, APMASKSIZE); > ... So I fail to understand what will prevent us from performing a > successful commit if some of the resources become 'used' between > the call to the in_use() callback and the memcpy. So, the scenario you describe would go something like this: 1. User changes apmask or aqmask attempting to take queue xx.yyyy away from the vfio_ap driver. 2. The in_use callback does not detect the affected queues to be in use (i.e., it is not assigned to an mdev). 3. Another user assigns queue xx.yyyy to an mdev 4. The memcpy is performed and ownership of xx.yyyy is transferred to the host. 5. Afterward, the queues are reprobed which results in the remove callback on the vfio_ap driver for xx.yyyy and the probe callback on the cex4queue driver for xx.yyyy. You are correct, there is nothing preventing a resource from becoming 'used' between the in_use callback and the memcpy. While the above scenario could be considered a circumvention of the intent of this design, the result would be no different than if the in_use callback was not implemented at all. When the remove callback is invoked for xx.yyyy on the vfio_ap driver due to the reprobe, the queue will be released. The chances of this scenario occurring are probably quite tiny given the timing of two root users almost simultaneously taking the required actions in the time it takes the verification loop to complete and the mask to be copied. I suppose this could happen if there are a great number of mdevs or a very large number of queues bound to the vfio_ap driver, but this scenario seems very unlikely. > > Of course I might be wrong. > > BTW I was never a fan of this mechanism, so I don't mind if it > does not work perfectly, and this should catch most of the cases. Just > want to make sure we don't introduce more confusion than necessary. > >> + >> + 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; >> >> @@ -1138,12 +1207,71 @@ static ssize_t aqmask_show(struct bus_type *bus, char *buf) >> return rc; >> } >> >> +static 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); >> + > Same here. > > Regards, > Halil > >> + 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 1ea046324e8f..48c57b3d53a0 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) >> @@ -255,6 +256,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)];
diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c index 24a1940b829e..db27bd931308 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" @@ -889,6 +890,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) @@ -908,14 +926,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); @@ -1107,12 +1118,70 @@ 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; + + /* + * 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; @@ -1138,12 +1207,71 @@ static ssize_t aqmask_show(struct bus_type *bus, char *buf) return rc; } +static 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 1ea046324e8f..48c57b3d53a0 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) @@ -255,6 +256,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)];
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 <akrowiak@linux.ibm.com> Reported-by: kernel test robot <lkp@intel.com> --- drivers/s390/crypto/ap_bus.c | 148 ++++++++++++++++++++++++++++++++--- drivers/s390/crypto/ap_bus.h | 4 + 2 files changed, 142 insertions(+), 10 deletions(-)