| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <99ddfadd-7aac-8f3c-9f07-ae2b66cf45cc@linux.ibm.com>
Date: Tue, 21 Jun 2022 16:02:23 -0400
From: "Jason J. Herne" <jjherne@...ux.ibm.com>
To: Tony Krowiak <akrowiak@...ux.ibm.com>, linux-s390@...r.kernel.org,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Cc: freude@...ux.ibm.com, borntraeger@...ibm.com, cohuck@...hat.com,
mjrosato@...ux.ibm.com, pasic@...ux.ibm.com,
alex.williamson@...hat.com, kwankhede@...dia.com,
fiuczy@...ux.ibm.com
Subject: Re: [PATCH v20 10/20] s390/vfio-ap: prepare for dynamic update of
guest's APCB on assign/unassign
On 6/21/22 11:51, Tony Krowiak wrote:
> The functions backing the matrix mdev's sysfs attribute interfaces to
> assign/unassign adapters, domains and control domains must take and
> release the locks required to perform a dynamic update of a guest's APCB
> in the proper order.
>
> The proper order for taking the locks is:
>
> matrix_dev->guests_lock => kvm->lock => matrix_dev->mdevs_lock
>
> The proper order for releasing the locks is:
>
> matrix_dev->mdevs_lock => kvm->lock => matrix_dev->guests_lock
>
> Two new macros are introduced for this purpose: One to take the locks and
> the other to release the locks. These macros will be used by the
> assignment/unassignment functions to prepare for dynamic update of
> the KVM guest's APCB.
>
> Signed-off-by: Tony Krowiak <akrowiak@...ux.ibm.com>
> ---
> drivers/s390/crypto/vfio_ap_ops.c | 69 +++++++++++++++++++++++++------
> 1 file changed, 57 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index f31db1248740..b8f901e6b580 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -75,6 +75,51 @@ static const struct vfio_device_ops vfio_ap_matrix_dev_ops;
> mutex_unlock(&matrix_dev->guests_lock); \
> })
>
> +/**
> + * get_update_locks_for_mdev: Acquire the locks required to dynamically update a
> + * KVM guest's APCB in the proper order.
> + *
> + * @matrix_mdev: a pointer to a struct ap_matrix_mdev object containing the AP
> + * configuration data to use to update a KVM guest's APCB.
> + *
> + * The proper locking order is:
> + * 1. matrix_dev->guests_lock: required to use the KVM pointer to update a KVM
> + * guest's APCB.
> + * 2. matrix_mdev->kvm->lock: required to update a guest's APCB
> + * 3. matrix_dev->mdevs_lock: required to access data stored in a matrix_mdev
> + *
> + * Note: If @matrix_mdev is NULL or is not attached to a KVM guest, the KVM
> + * lock will not be taken.
> + */
> +#define get_update_locks_for_mdev(matrix_mdev) ({ \
> + mutex_lock(&matrix_dev->guests_lock); \
> + if (matrix_mdev && matrix_mdev->kvm) \
> + mutex_lock(&matrix_mdev->kvm->lock); \
> + mutex_lock(&matrix_dev->mdevs_lock); \
> +})
> +
> +/**
> + * release_update_locks_for_mdev: Release the locks used to dynamically update a
> + * KVM guest's APCB in the proper order.
> + *
> + * @matrix_mdev: a pointer to a struct ap_matrix_mdev object containing the AP
> + * configuration data to use to update a KVM guest's APCB.
> + *
> + * The proper unlocking order is:
> + * 1. matrix_dev->mdevs_lock
> + * 2. matrix_mdev->kvm->lock
> + * 3. matrix_dev->guests_lock
> + *
> + * Note: If @matrix_mdev is NULL or is not attached to a KVM guest, the KVM
> + * lock will not be released.
> + */
> +#define release_update_locks_for_mdev(matrix_mdev) ({ \
> + mutex_unlock(&matrix_dev->mdevs_lock); \
> + if (matrix_mdev && matrix_mdev->kvm) \
> + mutex_unlock(&matrix_mdev->kvm->lock); \
> + mutex_unlock(&matrix_dev->guests_lock); \
> +})
> +
> /**
> * vfio_ap_mdev_get_queue - retrieve a queue with a specific APQN from a
> * hash table of queues assigned to a matrix mdev
> @@ -830,7 +875,7 @@ static ssize_t assign_adapter_store(struct device *dev,
>
> struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
>
> - mutex_lock(&matrix_dev->mdevs_lock);
> + get_update_locks_for_mdev(matrix_mdev);
>
> /* If the KVM guest is running, disallow assignment of adapter */
> if (matrix_mdev->kvm) {
> @@ -862,7 +907,7 @@ static ssize_t assign_adapter_store(struct device *dev,
> matrix_mdev->matrix.aqm, matrix_mdev);
> ret = count;
> done:
> - mutex_unlock(&matrix_dev->mdevs_lock);
> + release_update_locks_for_mdev(matrix_mdev);
>
> return ret;
> }
> @@ -905,7 +950,7 @@ static ssize_t unassign_adapter_store(struct device *dev,
> unsigned long apid;
> struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
>
> - mutex_lock(&matrix_dev->mdevs_lock);
> + get_update_locks_for_mdev(matrix_mdev);
>
> /* If the KVM guest is running, disallow unassignment of adapter */
> if (matrix_mdev->kvm) {
> @@ -930,7 +975,7 @@ static ssize_t unassign_adapter_store(struct device *dev,
>
> ret = count;
> done:
> - mutex_unlock(&matrix_dev->mdevs_lock);
> + release_update_locks_for_mdev(matrix_mdev);
> return ret;
> }
> static DEVICE_ATTR_WO(unassign_adapter);
> @@ -985,7 +1030,7 @@ static ssize_t assign_domain_store(struct device *dev,
> struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
> unsigned long max_apqi = matrix_mdev->matrix.aqm_max;
>
> - mutex_lock(&matrix_dev->mdevs_lock);
> + get_update_locks_for_mdev(matrix_mdev);
>
> /* If the KVM guest is running, disallow assignment of domain */
> if (matrix_mdev->kvm) {
> @@ -1016,7 +1061,7 @@ static ssize_t assign_domain_store(struct device *dev,
> matrix_mdev);
> ret = count;
> done:
> - mutex_unlock(&matrix_dev->mdevs_lock);
> + release_update_locks_for_mdev(matrix_mdev);
>
> return ret;
> }
> @@ -1059,7 +1104,7 @@ static ssize_t unassign_domain_store(struct device *dev,
> unsigned long apqi;
> struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
>
> - mutex_lock(&matrix_dev->mdevs_lock);
> + get_update_locks_for_mdev(matrix_mdev);
>
> /* If the KVM guest is running, disallow unassignment of domain */
> if (matrix_mdev->kvm) {
> @@ -1085,7 +1130,7 @@ static ssize_t unassign_domain_store(struct device *dev,
> ret = count;
>
> done:
> - mutex_unlock(&matrix_dev->mdevs_lock);
> + release_update_locks_for_mdev(matrix_mdev);
> return ret;
> }
> static DEVICE_ATTR_WO(unassign_domain);
> @@ -1112,7 +1157,7 @@ static ssize_t assign_control_domain_store(struct device *dev,
> unsigned long id;
> struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
>
> - mutex_lock(&matrix_dev->mdevs_lock);
> + get_update_locks_for_mdev(matrix_mdev);
>
> /* If the KVM guest is running, disallow assignment of control domain */
> if (matrix_mdev->kvm) {
> @@ -1138,7 +1183,7 @@ static ssize_t assign_control_domain_store(struct device *dev,
> vfio_ap_mdev_filter_cdoms(matrix_mdev);
> ret = count;
> done:
> - mutex_unlock(&matrix_dev->mdevs_lock);
> + release_update_locks_for_mdev(matrix_mdev);
> return ret;
> }
> static DEVICE_ATTR_WO(assign_control_domain);
> @@ -1166,7 +1211,7 @@ static ssize_t unassign_control_domain_store(struct device *dev,
> struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(dev);
> unsigned long max_domid = matrix_mdev->matrix.adm_max;
>
> - mutex_lock(&matrix_dev->mdevs_lock);
> + get_update_locks_for_mdev(matrix_mdev);
>
> /* If a KVM guest is running, disallow unassignment of control domain */
> if (matrix_mdev->kvm) {
> @@ -1189,7 +1234,7 @@ static ssize_t unassign_control_domain_store(struct device *dev,
>
> ret = count;
> done:
> - mutex_unlock(&matrix_dev->mdevs_lock);
> + release_update_locks_for_mdev(matrix_mdev);
> return ret;
> }
> static DEVICE_ATTR_WO(unassign_control_domain);
Reviewed-by: Jason J. Herne <jjherne@...ux.ibm.com>
--
-- Jason J. Herne (jjherne@...ux.ibm.com)
Powered by blists - more mailing lists