On Mon, 19 Jul 2021 15:35:03 -0400
Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
> It was pointed out during an unrelated patch review that locks should not
[..]
> -static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
> +static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev,
> + struct kvm *kvm)
> {
> - /*
> - * 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 (matrix_mdev->kvm) {
We used to check if matrix_mdev->kvm is null, but ...
> - matrix_mdev->kvm_busy = true;
> - mutex_unlock(&matrix_dev->lock);
> -
> - 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);
> - }
> + if (kvm->arch.crypto.crycbd) {
... now we just try to dereference it. And ..
> + down_write(&kvm->arch.crypto.pqap_hook_rwsem);
> + kvm->arch.crypto.pqap_hook = NULL;
> + up_write(&kvm->arch.crypto.pqap_hook_rwsem);
>
> + mutex_lock(&kvm->lock);
> mutex_lock(&matrix_dev->lock);
> +
> + kvm_arch_crypto_clear_masks(kvm);
> vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
> - kvm_put_kvm(matrix_mdev->kvm);
> + kvm_put_kvm(kvm);
> matrix_mdev->kvm = NULL;
> - matrix_mdev->kvm_busy = false;
> - wake_up_all(&matrix_mdev->wait_for_kvm);
> +
> + mutex_unlock(&kvm->lock);
> + mutex_unlock(&matrix_dev->lock);
> }
> }
>
[..]
> @@ -1363,14 +1323,11 @@ 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);
> -
.. before access to the matrix_mdev->kvm used to be protected by
the 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, matrix_mdev->kvm);
... but it is not any more. BTW I don't think the code is guaranteed
to fetch ->kvm just once.
Can you please explain why can we get away with being more
lax when dealing with matrix_mdev->kvm?
Regards,
Halil
[..]
