Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification
From: Tony Krowiak
Date: Thu Jul 22 2021 - 09:09:36 EST
On 7/21/21 10:45 AM, Halil Pasic wrote:
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 ..
We used to check matrix_mdev->kvm, now the kvm pointer is passed into
the function; however, having said that, the pointer passed in should be
checked before de-referencing it.
+ 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.
There are a couple of things to point out here:
1. The vfio_ap_mdev_unset_kvm function() is the only place where the
matrix_mdev->kvm pointer is cleared. That function is called here
as well as by the group notifier callback for VFIO_GROUP_NOTIFY_SET_KVM
events. If you notice in the code above, the group notifier is
unregistered
before calling the unset function, so either the notifier will have
already
been invoked and the pointer cleared (which is why you are correct
that the KVM pointer passed in needs to get checked in the unset
function),
or will get cleared here.
2. The release callback is invoked when the mdev fd is closed by userspace.
The remove callback is the only place where the matrix_mdev is
freed. The
remove callback is not called until the mdev fd is released, so it
is guaranteed
the matrix_mdev will exist when the release callback is invoked.
3. The matrix_dev->lock is then taken in the vfio_ap_mdev_unset_kvm function
before doing any operations that modify the matrix_mdev.
Can you please explain why can we get away with being more
lax when dealing with matrix_mdev->kvm?
See above.
Regards,
Halil
[..]