On Thu, 11 Feb 2021 09:21:26 -0500
Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
> Yes, it makes sense. I guess I didn't look closely at your
> suggestion when I said it was exactly what I implemented
> after agreeing with Connie. I had a slight difference in
> my implementation:
>
> static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
> {
> struct kvm *kvm;
>
> mutex_lock(&matrix_dev->lock);
>
> if (matrix_mdev->kvm) {
> kvm = matrix_mdev->kvm;
> mutex_unlock(&matrix_dev->lock);
The problem with this one is that as soon as we drop
the lock here, another thread can in theory execute
the critical section below, which drops our reference
to kvm via kvm_put_kvm(kvm). Thus when we enter
kvm_arch_crypto_clear_mask(), even if we are guaranteed
to have a non-null pointer, the pointee is not guaranteed
to be around. So like Connie suggested, you better take
another reference to kvm in the first critical section.
Regards,
Halil
> kvm_arch_crypto_clear_masks(kvm);
> mutex_lock(&matrix_dev->lock);
> kvm->arch.crypto.pqap_hook = NULL;
> vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
> matrix_mdev->kvm = NULL;
> kvm_put_kvm(kvm);
> }
>
> mutex_unlock(&matrix_dev->lock);
> }
