lkml.org 
[lkml]   [2021]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH] s390/vfio-ap: do not use open locks during VFIO_GROUP_NOTIFY_SET_KVM notification
    Date
    The fix to resolve a lockdep splat while handling the
    VFIO_GROUP_NOTIFY_SET_KVM event introduced a kvm_busy flag indicating that
    the vfio_ap device driver is busy setting or unsetting the KVM pointer.
    A wait queue was employed to allow functions requiring access to the KVM
    pointer to wait for the kvm_busy flag to be cleared. For the duration of
    the wait period, the mdev lock was unlocked then acquired again after the
    kvm_busy flag was cleared. This got rid of the lockdep report, but didn't
    really resolve the problem.

    This patch removes the the kvm_busy flag and wait queue as they are not
    necessary to resolve the lockdep splat problem. The wait queue was
    introduced to prevent changes to the matrix used to update the guest's
    AP configuration. The idea was that whenever an adapter, domain or control
    domain was being assigned to or unassigned from the matrix, the function
    would wait until the group notifier function was no longer busy with the
    KVM pointer.

    The thing is, the KVM pointer value (matrix_mdev->kvm) is always set and
    cleared while holding the matrix_dev->lock mutex. The assignment and
    unassignment interfaces also lock the matrix_dev->lock mutex prior to
    checking whether the matrix_mdev->kvm pointer is set and if so, returns
    the -EBUSY error from the function. Consequently, there is no chance for
    an update to the matrix to occur while the guest's AP configuration is
    being updated.

    Fixes: 0cc00c8d4050 ("s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks")
    Cc: stable@vger.kernel.org
    Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
    ---
    drivers/s390/crypto/vfio_ap_ops.c | 77 +++++++--------------------
    drivers/s390/crypto/vfio_ap_private.h | 2 -
    2 files changed, 20 insertions(+), 59 deletions(-)

    diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
    index 742277bc8d1c..99a58f54c076 100644
    --- a/drivers/s390/crypto/vfio_ap_ops.c
    +++ b/drivers/s390/crypto/vfio_ap_ops.c
    @@ -294,15 +294,6 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
    matrix_mdev = container_of(vcpu->kvm->arch.crypto.pqap_hook,
    struct ap_matrix_mdev, pqap_hook);

    - /*
    - * If the KVM pointer is in the process of being set, wait until the
    - * process has completed.
    - */
    - wait_event_cmd(matrix_mdev->wait_for_kvm,
    - !matrix_mdev->kvm_busy,
    - mutex_unlock(&matrix_dev->lock),
    - mutex_lock(&matrix_dev->lock));
    -
    /* If the there is no guest using the mdev, there is nothing to do */
    if (!matrix_mdev->kvm)
    goto out_unlock;
    @@ -350,7 +341,6 @@ static int vfio_ap_mdev_create(struct mdev_device *mdev)

    matrix_mdev->mdev = mdev;
    vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
    - init_waitqueue_head(&matrix_mdev->wait_for_kvm);
    mdev_set_drvdata(mdev, matrix_mdev);
    matrix_mdev->pqap_hook = handle_pqap;
    mutex_lock(&matrix_dev->lock);
    @@ -623,7 +613,7 @@ static ssize_t assign_adapter_store(struct device *dev,
    * If the KVM pointer is in flux or the guest is running, disallow
    * un-assignment of adapter
    */
    - if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
    + if (matrix_mdev->kvm) {
    ret = -EBUSY;
    goto done;
    }
    @@ -696,7 +686,7 @@ static ssize_t unassign_adapter_store(struct device *dev,
    * If the KVM pointer is in flux or the guest is running, disallow
    * un-assignment of adapter
    */
    - if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
    + if (matrix_mdev->kvm) {
    ret = -EBUSY;
    goto done;
    }
    @@ -786,7 +776,7 @@ static ssize_t assign_domain_store(struct device *dev,
    * If the KVM pointer is in flux or the guest is running, disallow
    * assignment of domain
    */
    - if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
    + if (matrix_mdev->kvm) {
    ret = -EBUSY;
    goto done;
    }
    @@ -854,7 +844,7 @@ static ssize_t unassign_domain_store(struct device *dev,
    * If the KVM pointer is in flux or the guest is running, disallow
    * un-assignment of domain
    */
    - if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
    + if (matrix_mdev->kvm) {
    ret = -EBUSY;
    goto done;
    }
    @@ -908,7 +898,7 @@ static ssize_t assign_control_domain_store(struct device *dev,
    * If the KVM pointer is in flux or the guest is running, disallow
    * assignment of control domain.
    */
    - if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
    + if (matrix_mdev->kvm) {
    ret = -EBUSY;
    goto done;
    }
    @@ -967,7 +957,7 @@ static ssize_t unassign_control_domain_store(struct device *dev,
    * If the KVM pointer is in flux or the guest is running, disallow
    * un-assignment of control domain.
    */
    - if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
    + if (matrix_mdev->kvm) {
    ret = -EBUSY;
    goto done;
    }
    @@ -1108,14 +1098,17 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
    struct ap_matrix_mdev *m;

    if (kvm->arch.crypto.crycbd) {
    + mutex_lock(&matrix_dev->lock);
    +
    list_for_each_entry(m, &matrix_dev->mdev_list, node) {
    - if (m != matrix_mdev && m->kvm == kvm)
    + if (m != matrix_mdev && m->kvm == kvm) {
    + mutex_unlock(&matrix_dev->lock);
    return -EPERM;
    + }
    }

    kvm_get_kvm(kvm);
    matrix_mdev->kvm = kvm;
    - matrix_mdev->kvm_busy = true;
    mutex_unlock(&matrix_dev->lock);

    down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
    @@ -1126,10 +1119,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
    matrix_mdev->matrix.apm,
    matrix_mdev->matrix.aqm,
    matrix_mdev->matrix.adm);
    -
    - mutex_lock(&matrix_dev->lock);
    - matrix_mdev->kvm_busy = false;
    - wake_up_all(&matrix_mdev->wait_for_kvm);
    }

    return 0;
    @@ -1181,33 +1170,21 @@ static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb,
    */
    static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
    {
    - /*
    - * If the KVM pointer is in the process of being set, wait until the
    - * process has completed.
    - */
    - wait_event_cmd(matrix_mdev->wait_for_kvm,
    - !matrix_mdev->kvm_busy,
    - mutex_unlock(&matrix_dev->lock),
    - mutex_lock(&matrix_dev->lock));
    + mutex_lock(&matrix_dev->lock);

    - if (matrix_mdev->kvm) {
    - matrix_mdev->kvm_busy = true;
    + if ((matrix_mdev->kvm) && (matrix_mdev->kvm->arch.crypto.crycbd)) {
    mutex_unlock(&matrix_dev->lock);
    + down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
    + matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
    + up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);

    - if (matrix_mdev->kvm->arch.crypto.crycbd) {
    - down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
    - matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
    - up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
    -
    - kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
    - }
    + kvm_arch_crypto_clear_masks(matrix_mdev->kvm);

    mutex_lock(&matrix_dev->lock);
    vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
    kvm_put_kvm(matrix_mdev->kvm);
    matrix_mdev->kvm = NULL;
    - matrix_mdev->kvm_busy = false;
    - wake_up_all(&matrix_mdev->wait_for_kvm);
    + mutex_unlock(&matrix_dev->lock);
    }
    }

    @@ -1220,7 +1197,6 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
    if (action != VFIO_GROUP_NOTIFY_SET_KVM)
    return NOTIFY_OK;

    - mutex_lock(&matrix_dev->lock);
    matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);

    if (!data)
    @@ -1228,8 +1204,6 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
    else if (vfio_ap_mdev_set_kvm(matrix_mdev, data))
    notify_rc = NOTIFY_DONE;

    - mutex_unlock(&matrix_dev->lock);
    -
    return notify_rc;
    }

    @@ -1363,14 +1337,12 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
    {
    struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);

    - mutex_lock(&matrix_dev->lock);
    - vfio_ap_mdev_unset_kvm(matrix_mdev);
    - mutex_unlock(&matrix_dev->lock);
    -
    vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
    &matrix_mdev->iommu_notifier);
    vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
    &matrix_mdev->group_notifier);
    +
    + vfio_ap_mdev_unset_kvm(matrix_mdev);
    module_put(THIS_MODULE);
    }

    @@ -1412,15 +1384,6 @@ static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
    break;
    }

    - /*
    - * If the KVM pointer is in the process of being set, wait until
    - * the process has completed.
    - */
    - wait_event_cmd(matrix_mdev->wait_for_kvm,
    - !matrix_mdev->kvm_busy,
    - mutex_unlock(&matrix_dev->lock),
    - mutex_lock(&matrix_dev->lock));
    -
    ret = vfio_ap_mdev_reset_queues(mdev);
    break;
    default:
    diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
    index e12218e5a629..22d2e0ca3ae5 100644
    --- a/drivers/s390/crypto/vfio_ap_private.h
    +++ b/drivers/s390/crypto/vfio_ap_private.h
    @@ -83,8 +83,6 @@ struct ap_matrix_mdev {
    struct ap_matrix matrix;
    struct notifier_block group_notifier;
    struct notifier_block iommu_notifier;
    - bool kvm_busy;
    - wait_queue_head_t wait_for_kvm;
    struct kvm *kvm;
    crypto_hook pqap_hook;
    struct mdev_device *mdev;
    --
    2.30.2
    \
     
     \ /
      Last update: 2021-06-26 00:09    [W:4.435 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site