Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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

[..]




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux