| 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
| ||
|
Date: Tue, 31 May 2022 08:02:41 -0400
From: Tony Krowiak <akrowiak@...ux.ibm.com>
To: jjherne@...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 v19 11/20] s390/vfio-ap: prepare for dynamic update of
guest's APCB on queue probe/remove
On 5/27/22 9:50 AM, Jason J. Herne wrote:
> On 4/4/22 18:10, Tony Krowiak wrote:
>> The callback functions for probing and removing a queue device 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
>>
>> A new helper function is introduced to be used by the probe callback to
>> acquire the required locks. Since the probe callback only has
>> access to a queue device when it is called, the helper function will
>> find
>> the ap_matrix_mdev object to which the queue device's APQN is
>> assigned and
>> return it so the KVM guest to which the mdev is attached can be
>> dynamically
>> updated.
>>
>> Note that in order to find the ap_matrix_mdev (matrix_mdev) object,
>> it is
>> necessary to search the matrix_dev->mdev_list. This presents a
>> locking order dilemma because the matrix_dev->mdevs_lock can't be
>> taken to
>> protect against changes to the list while searching for the
>> matrix_mdev to
>> which a queue device's APQN is assigned. This is due to the fact that
>> the
>> proper locking order requires that the matrix_dev->mdevs_lock be taken
>> after both the matrix_mdev->kvm->lock and the matrix_dev->mdevs_lock.
>> Consequently, the matrix_dev->guests_lock will be used to protect
>> against
>> removal of a matrix_mdev object from the list while a queue device is
>> being probed. This necessitates changes to the mdev probe/remove
>> callback functions to take the matrix_dev->guests_lock prior to removing
>> a matrix_mdev object from the list.
>>
>> A new macro is also introduced to acquire the locks required to
>> dynamically
>> update the guest's APCB in the proper order when a queue device is
>> removed.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@...ux.ibm.com>
>> ---
>> drivers/s390/crypto/vfio_ap_ops.c | 126 +++++++++++++++++++++---------
>> 1 file changed, 88 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 2219b1069ceb..080a733f7cd2 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -116,6 +116,74 @@ static const struct vfio_device_ops
>> vfio_ap_matrix_dev_ops;
>> mutex_unlock(&matrix_dev->guests_lock); \
>> })
>> +/**
>> + * vfio_ap_mdev_get_update_locks_for_apqn: retrieve the matrix mdev
>> to which an
>> + * APQN is assigned and acquire the
>> + * locks required to update the APCB of
>> + * the KVM guest to which the mdev is
>> + * attached.
>> + *
>> + * @apqn: the APQN of a queue device.
>> + *
>> + * 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 @apqn is not assigned to a matrix_mdev, the
>> matrix_mdev->kvm->lock
>> + * will not be taken.
>> + *
>> + * Return: the ap_matrix_mdev object to which @apqn is assigned or
>> NULL if @apqn
>> + * is not assigned to an ap_matrix_mdev.
>> + */
>> +static struct ap_matrix_mdev
>> *vfio_ap_mdev_get_update_locks_for_apqn(int apqn)
>> +{
>> + struct ap_matrix_mdev *matrix_mdev;
>> +
>> + mutex_lock(&matrix_dev->guests_lock);
>> +
>> + list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
>> + if (test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm) &&
>> + test_bit_inv(AP_QID_QUEUE(apqn),
>> matrix_mdev->matrix.aqm)) {
>> + if (matrix_mdev->kvm)
>> + mutex_lock(&matrix_mdev->kvm->lock);
>> +
>> + mutex_lock(&matrix_dev->mdevs_lock);
>> +
>> + return matrix_mdev;
>> + }
>> + }
>> +
>> + mutex_lock(&matrix_dev->mdevs_lock);
>> +
>> + return NULL;
>> +}
>> +
>> +/**
>> + * get_update_locks_for_queue: get the locks required to update the
>> APCB of the
>> + * KVM guest to which the matrix mdev linked to a
>> + * vfio_ap_queue object is attached.
>> + *
>> + * @queue: a pointer to a vfio_ap_queue object.
>> + *
>> + * The proper locking order is:
>> + * 1. matrix_dev->guests_lock: required to use the KVM pointer to
>> update a KVM
>> + * guest's APCB.
>> + * 2. queue->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 @queue is not linked to an ap_matrix_mdev object, the
>> KVM lock
>> + * will not be taken.
>> + */
>> +#define get_update_locks_for_queue(queue) ({ \
>> + struct ap_matrix_mdev *matrix_mdev = q->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); \
>> +})
>> +
>
>
> One more comment I forgot to include before:
> This macro is far too similar to existing macro,
> get_update_locks_for_mdev. And it is only called in one place. Let's
> remove this and replace the single invocation with:
>
> get_update_locks_for_mdev(q->matrix_mdev);
Yikes, I see another flaw in this macro! Either the input parameter
needs to be renamed to 'q' or the q->matrix_mdev needs to be changed to
queue->matrix_mdev. I think I'll go with the former since vfio_ap_queue
is referred to as 'q' everywhere else.
>
>
>> /**
>> * vfio_ap_mdev_get_queue - retrieve a queue with a specific APQN
>> from a
>> * hash table of queues assigned to a matrix mdev
>> @@ -615,21 +683,18 @@ static int vfio_ap_mdev_probe(struct
>> mdev_device *mdev)
>> matrix_mdev->pqap_hook = handle_pqap;
>> vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->shadow_apcb);
>> hash_init(matrix_mdev->qtable.queues);
>> - mdev_set_drvdata(mdev, matrix_mdev);
>> - mutex_lock(&matrix_dev->mdevs_lock);
>> - list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
>> - mutex_unlock(&matrix_dev->mdevs_lock);
>> ret = vfio_register_emulated_iommu_dev(&matrix_mdev->vdev);
>> if (ret)
>> goto err_list;
>> + mdev_set_drvdata(mdev, matrix_mdev);
>> + mutex_lock(&matrix_dev->mdevs_lock);
>> + list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
>> + mutex_unlock(&matrix_dev->mdevs_lock);
>> dev_set_drvdata(&mdev->dev, matrix_mdev);
>> return 0;
>> err_list:
>> - mutex_lock(&matrix_dev->mdevs_lock);
>> - list_del(&matrix_mdev->node);
>> - mutex_unlock(&matrix_dev->mdevs_lock);
>> vfio_uninit_group_dev(&matrix_mdev->vdev);
>> kfree(matrix_mdev);
>> err_dec_available:
>> @@ -692,11 +757,13 @@ static void vfio_ap_mdev_remove(struct
>> mdev_device *mdev)
>> vfio_unregister_group_dev(&matrix_mdev->vdev);
>> + mutex_lock(&matrix_dev->guests_lock);
>> mutex_lock(&matrix_dev->mdevs_lock);
>> vfio_ap_mdev_reset_queues(matrix_mdev);
>> vfio_ap_mdev_unlink_fr_queues(matrix_mdev);
>> list_del(&matrix_mdev->node);
>> mutex_unlock(&matrix_dev->mdevs_lock);
>> + mutex_unlock(&matrix_dev->guests_lock);
>> vfio_uninit_group_dev(&matrix_mdev->vdev);
>> kfree(matrix_mdev);
>> atomic_inc(&matrix_dev->available_instances);
>> @@ -1665,49 +1732,30 @@ void vfio_ap_mdev_unregister(void)
>> mdev_unregister_driver(&vfio_ap_matrix_driver);
>> }
>> -/*
>> - * vfio_ap_queue_link_mdev
>> - *
>> - * @q: The queue to link with the matrix mdev.
>> - *
>> - * Links @q with the matrix mdev to which the queue's APQN is assigned.
>> - */
>> -static void vfio_ap_queue_link_mdev(struct vfio_ap_queue *q)
>> -{
>> - unsigned long apid = AP_QID_CARD(q->apqn);
>> - unsigned long apqi = AP_QID_QUEUE(q->apqn);
>> - struct ap_matrix_mdev *matrix_mdev;
>> -
>> - list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
>> - if (test_bit_inv(apid, matrix_mdev->matrix.apm) &&
>> - test_bit_inv(apqi, matrix_mdev->matrix.aqm)) {
>> - vfio_ap_mdev_link_queue(matrix_mdev, q);
>> - break;
>> - }
>> - }
>> -}
>> -
>> int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>> {
>> struct vfio_ap_queue *q;
>> + struct ap_matrix_mdev *matrix_mdev;
>> DECLARE_BITMAP(apm_delta, AP_DEVICES);
>> q = kzalloc(sizeof(*q), GFP_KERNEL);
>> if (!q)
>> return -ENOMEM;
>> - mutex_lock(&matrix_dev->mdevs_lock);
>> q->apqn = to_ap_queue(&apdev->device)->qid;
>> q->saved_isc = VFIO_AP_ISC_INVALID;
>> - vfio_ap_queue_link_mdev(q);
>> - if (q->matrix_mdev) {
>> +
>> + matrix_mdev = vfio_ap_mdev_get_update_locks_for_apqn(q->apqn);
>> +
>> + if (matrix_mdev) {
>> + vfio_ap_mdev_link_queue(matrix_mdev, q);
>> memset(apm_delta, 0, sizeof(apm_delta));
>> set_bit_inv(AP_QID_CARD(q->apqn), apm_delta);
>> vfio_ap_mdev_filter_matrix(apm_delta,
>> - q->matrix_mdev->matrix.aqm,
>> - q->matrix_mdev);
>> + matrix_mdev->matrix.aqm,
>> + matrix_mdev);
>> }
>> dev_set_drvdata(&apdev->device, q);
>> - mutex_unlock(&matrix_dev->mdevs_lock);
>> + release_update_locks_for_mdev(matrix_mdev);
>> return 0;
>> }
>> @@ -1716,11 +1764,13 @@ void vfio_ap_mdev_remove_queue(struct
>> ap_device *apdev)
>> {
>> unsigned long apid;
>> struct vfio_ap_queue *q;
>> + struct ap_matrix_mdev *matrix_mdev;
>> - mutex_lock(&matrix_dev->mdevs_lock);
>> q = dev_get_drvdata(&apdev->device);
>> + get_update_locks_for_queue(q);
>> + matrix_mdev = q->matrix_mdev;
>> - if (q->matrix_mdev) {
>> + if (matrix_mdev) {
>> vfio_ap_unlink_queue_fr_mdev(q);
>> apid = AP_QID_CARD(q->apqn);
>> @@ -1731,5 +1781,5 @@ void vfio_ap_mdev_remove_queue(struct ap_device
>> *apdev)
>> vfio_ap_mdev_reset_queue(q, 1);
>> dev_set_drvdata(&apdev->device, NULL);
>> kfree(q);
>> - mutex_unlock(&matrix_dev->mdevs_lock);
>> + release_update_locks_for_mdev(matrix_mdev);
>> }
>
>
Powered by blists - more mailing lists