fix circular lockdep when staring SE guest

This patch fixes a circular locking dependency in the CI introduced by commit f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated"). The lockdep only occurs when starting a Secure Execution guest. Crypto virtualization (vfio_ap) is not yet supported for SE guests; however, in order to avoid CI errors, this fix is being provided.

The circular lockdep was introduced when the masks in the guest's APCB were taken under the matrix_dev->lock. While the lock is definitely needed to protect the setting/unsetting of the KVM pointer, it is not necessarily critical for setting the masks, so this will not be done under protection of the matrix_dev->lock.

Fixes f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated")

Cc [email protected]

Signed-off-by Tony Krowiak <[email protected]>

---
 drivers/s390/crypto/vfio_ap_ops.c | 75 ++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 30 deletions(-)

static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev) {

drivers/s390/crypto/vfio_ap_ops.c +8 -0

+		kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
+		mutex_lock(&matrix_dev->lock);
+		matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
+		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
+		kvm_put_kvm(matrix_mdev->kvm);
+		matrix_mdev->kvm = NULL;
+		mutex_unlock(&matrix_dev->lock);
+	}
 }

Quoted 23 lines · On Tue, 9 Feb 2021 14:48:30 -0500 Tony Krowiak <[email protected]> wrote:

This patch fixes a circular locking dependency in the CI introduced by commit f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated"). The lockdep only occurs when starting a Secure Execution guest. Crypto virtualization (vfio_ap) is not yet supported for SE guests; however, in order to avoid CI errors, this fix is being provided.

The circular lockdep was introduced when the masks in the guest's APCB were taken under the matrix_dev->lock. While the lock is definitely needed to protect the setting/unsetting of the KVM pointer, it is not necessarily critical for setting the masks, so this will not be done under protection of the matrix_dev->lock.

Fixes f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated")

Cc [email protected]

Signed-off-by Tony Krowiak <[email protected]>

---
 drivers/s390/crypto/vfio_ap_ops.c | 75 ++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 30 deletions(-)

static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev) {

drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb, · line 1114

Quoted 6 lines

drivers/s390/crypto/vfio_ap_ops.c +1 -5

-	kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
-	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
-	vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
-	kvm_put_kvm(matrix_mdev->kvm);
-	matrix_mdev->kvm = NULL;
+	if (matrix_mdev->kvm) {  

If you're doing setting/unsetting under matrix_dev->lock, is it possible that matrix_mdev->kvm gets unset between here and the next line, as you don't hold the lock?

Maybe you could
- grab a reference to kvm while holding the lock
- call the mask handling functions with that kvm reference
- lock again, drop the reference, and do the rest of the processing?

Quoted 9 lines

drivers/s390/crypto/vfio_ap_ops.c +8 -0

+		kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
+		mutex_lock(&matrix_dev->lock);
+		matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
+		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
+		kvm_put_kvm(matrix_mdev->kvm);
+		matrix_mdev->kvm = NULL;
+		mutex_unlock(&matrix_dev->lock);
+	}
 }  

Quoted 4 lines

Maybe you could
- grab a reference to kvm while holding the lock
- call the mask handling functions with that kvm reference
- lock again, drop the reference, and do the rest of the processing?

I agree, matrix_mdev->kvm can go NULL any time and we are risking a null pointer dereference here.

Another idea would be to do

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;                                         
                matrix_mdev->kvm = NULL;                                        
                mutex_unlock(&matrix_dev->lock);                                
                kvm_arch_crypto_clear_masks(kvm);                               
                mutex_lock(&matrix_dev->lock);                                  
                matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;

                vfio_ap_mdev_reset_queues(matrix_mdev->mdev);                   
                kvm_put_kvm(kvm);                                               
        }                                                                       
        mutex_unlock(&matrix_dev->lock);                                         
}

That way only one unset would actually do the unset and cleanup and every other invocation would bail out with only checking matrix_mdev->kvm.

Quoted 11 lines · On Wed, 10 Feb 2021 16:24:29 +0100 Halil Pasic <[email protected]> wrote:

Quoted 4 lines

Maybe you could
- grab a reference to kvm while holding the lock
- call the mask handling functions with that kvm reference
- lock again, drop the reference, and do the rest of the processing?

I agree, matrix_mdev->kvm can go NULL any time and we are risking a null pointer dereference here.

Another idea would be to do

static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)

Quoted 11 lines

{
         struct kvm *kvm;
                                                         
         mutex_lock(&matrix_dev->lock);
         if (matrix_mdev->kvm) {
                 kvm = matrix_mdev->kvm;
                 matrix_mdev->kvm = NULL;
                 mutex_unlock(&matrix_dev->lock);
                 kvm_arch_crypto_clear_masks(kvm);
                 mutex_lock(&matrix_dev->lock);
                 matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;

s/matrix_mdev->kvm/kvm

Quoted 6 lines

                 vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
                 kvm_put_kvm(kvm);
         }
         mutex_unlock(&matrix_dev->lock);
}

Quoted 3 lines

That way only one unset would actually do the unset and cleanup and every other invocation would bail out with only checking matrix_mdev->kvm.

But the problem with that is that we enable the the assign/unassign prematurely, which could interfere wit reset_queues(). Forget about it.

Quoted 32 lines · Halil Pasic

Quoted 11 lines · On Wed, 10 Feb 2021 16:24:29 +0100 Halil Pasic <[email protected]> wrote:

Quoted 4 lines

Maybe you could
- grab a reference to kvm while holding the lock
- call the mask handling functions with that kvm reference
- lock again, drop the reference, and do the rest of the processing?

I agree, matrix_mdev->kvm can go NULL any time and we are risking a null pointer dereference here.

Another idea would be to do

static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)

Quoted 11 lines

{
         struct kvm *kvm;
                                                         
         mutex_lock(&matrix_dev->lock);
         if (matrix_mdev->kvm) {
                 kvm = matrix_mdev->kvm;
                 matrix_mdev->kvm = NULL;
                 mutex_unlock(&matrix_dev->lock);
                 kvm_arch_crypto_clear_masks(kvm);
                 mutex_lock(&matrix_dev->lock);
                 matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;  

s/matrix_mdev->kvm/kvm

Quoted 6 lines

                 vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
                 kvm_put_kvm(kvm);
         }
         mutex_unlock(&matrix_dev->lock);
}

Quoted 6 lines

Quoted 3 lines

That way only one unset would actually do the unset and cleanup and every other invocation would bail out with only checking matrix_mdev->kvm.

But the problem with that is that we enable the the assign/unassign prematurely, which could interfere wit reset_queues(). Forget about it.

Not sure what you mean by this.

Quoted 33 lines · On Wed, 10 Feb 2021 17:05:48 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 32 lines · Halil Pasic

Quoted 11 lines · On Wed, 10 Feb 2021 16:24:29 +0100 Halil Pasic <[email protected]> wrote:

Quoted 4 lines

Maybe you could
- grab a reference to kvm while holding the lock
- call the mask handling functions with that kvm reference
- lock again, drop the reference, and do the rest of the processing?

I agree, matrix_mdev->kvm can go NULL any time and we are risking a null pointer dereference here.

Another idea would be to do

static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)

Quoted 11 lines

{
          struct kvm *kvm;
                                                          
          mutex_lock(&matrix_dev->lock);
          if (matrix_mdev->kvm) {
                  kvm = matrix_mdev->kvm;
                  matrix_mdev->kvm = NULL;
                  mutex_unlock(&matrix_dev->lock);
                  kvm_arch_crypto_clear_masks(kvm);
                  mutex_lock(&matrix_dev->lock);
                  matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;

s/matrix_mdev->kvm/kvm

Quoted 6 lines

                  vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
                  kvm_put_kvm(kvm);
          }
          mutex_unlock(&matrix_dev->lock);
}

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

That way only one unset would actually do the unset and cleanup and every other invocation would bail out with only checking matrix_mdev->kvm.

But the problem with that is that we enable the the assign/unassign prematurely, which could interfere wit reset_queues(). Forget about it.

Not sure what you mean by this.

I mean because above I first do
(1) matrix_mdev->kvm = NULL;
and then do
(2) vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
another thread could do
static ssize_t unassign_adapter_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t count)

{
         int ret;
         unsigned long apid;
         struct mdev_device *mdev = mdev_from_dev(dev);
         struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
                                                                                 
         /* If the guest is running, disallow un-assignment of adapter */
         if (matrix_mdev->kvm)
                 return -EBUSY;

...
}
between (1) and (2), and we would not bail out with -EBUSY because !!kvm because of (1). That means we would change matrix_mdev->matrix and we would not reset the queues that correspond to the apid that was just removed, because by the time we do the reset_queues, the queues are not in the matrix_mdev->matrix any more.

Does that make sense?

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);

        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);
}

Quoted 9 lines · On Thu, 11 Feb 2021 09:21:26 -0500 Tony Krowiak <[email protected]> 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;

Quoted 5 lines

      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

Quoted 10 lines

          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);
}

Quoted 35 lines · On Wed, 10 Feb 2021 11:53:34 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 22 lines · On Tue, 9 Feb 2021 14:48:30 -0500 Tony Krowiak <[email protected]> wrote:

This patch fixes a circular locking dependency in the CI introduced by commit f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated"). The lockdep only occurs when starting a Secure Execution guest. Crypto virtualization (vfio_ap) is not yet supported for SE guests; however, in order to avoid CI errors, this fix is being provided.

The circular lockdep was introduced when the masks in the guest's APCB were taken under the matrix_dev->lock. While the lock is definitely needed to protect the setting/unsetting of the KVM pointer, it is not necessarily critical for setting the masks, so this will not be done under protection of the matrix_dev->lock.

Fixes f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated")

Cc [email protected]

Signed-off-by Tony Krowiak <[email protected]>

---
  drivers/s390/crypto/vfio_ap_ops.c | 75 ++++++++++++++++++-------------
  1 file changed, 45 insertions(+), 30 deletions(-)

  static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
  {

drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb, · line 1114

Quoted 6 lines

drivers/s390/crypto/vfio_ap_ops.c +1 -5

-	kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
-	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
-	vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
-	kvm_put_kvm(matrix_mdev->kvm);
-	matrix_mdev->kvm = NULL;
+	if (matrix_mdev->kvm) {

If you're doing setting/unsetting under matrix_dev->lock, is it possible that matrix_mdev->kvm gets unset between here and the next line, as you don't hold the lock?

Quoted 4 lines

Maybe you could
- grab a reference to kvm while holding the lock
- call the mask handling functions with that kvm reference
- lock again, drop the reference, and do the rest of the processing?

I agree, matrix_mdev->kvm can go NULL any time and we are risking a null pointer dereference here.

Another idea would be to do

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;
                 matrix_mdev->kvm = NULL;
                 mutex_unlock(&matrix_dev->lock);
                 kvm_arch_crypto_clear_masks(kvm);
                 mutex_lock(&matrix_dev->lock);
                 matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
                 vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
                 kvm_put_kvm(kvm);
         }
         mutex_unlock(&matrix_dev->lock);
}

That way only one unset would actually do the unset and cleanup and every other invocation would bail out with only checking matrix_mdev->kvm.

Quoted 11 lines

Quoted 11 lines

drivers/s390/crypto/vfio_ap_ops.c +8 -0

+		kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
+		mutex_lock(&matrix_dev->lock);
+		matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
+		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
+		kvm_put_kvm(matrix_mdev->kvm);
+		matrix_mdev->kvm = NULL;
+		mutex_unlock(&matrix_dev->lock);
+	}
  }

Quoted 20 lines · On Tue, 9 Feb 2021 14:48:30 -0500 Tony Krowiak <[email protected]> wrote:

This patch fixes a circular locking dependency in the CI introduced by commit f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated"). The lockdep only occurs when starting a Secure Execution guest. Crypto virtualization (vfio_ap) is not yet supported for SE guests; however, in order to avoid CI errors, this fix is being provided.

The circular lockdep was introduced when the masks in the guest's APCB were taken under the matrix_dev->lock. While the lock is definitely needed to protect the setting/unsetting of the KVM pointer, it is not necessarily critical for setting the masks, so this will not be done under protection of the matrix_dev->lock.

Fixes f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated")

Cc [email protected]

Signed-off-by Tony Krowiak <[email protected]>

---
  drivers/s390/crypto/vfio_ap_ops.c | 75 ++++++++++++++++++-------------
  1 file changed, 45 insertions(+), 30 deletions(-)

drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb, · line 1114

Quoted 8 lines

  static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
  {

drivers/s390/crypto/vfio_ap_ops.c +1 -5

-	kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
-	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
-	vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
-	kvm_put_kvm(matrix_mdev->kvm);
-	matrix_mdev->kvm = NULL;
+	if (matrix_mdev->kvm) {

If you're doing setting/unsetting under matrix_dev->lock, is it possible that matrix_mdev->kvm gets unset between here and the next line, as you don't hold the lock?

Maybe you could
- grab a reference to kvm while holding the lock
- call the mask handling functions with that kvm reference
- lock again, drop the reference, and do the rest of the processing?

Quoted 9 lines

drivers/s390/crypto/vfio_ap_ops.c +8 -0

+		kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
+		mutex_lock(&matrix_dev->lock);
+		matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
+		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
+		kvm_put_kvm(matrix_mdev->kvm);
+		matrix_mdev->kvm = NULL;
+		mutex_unlock(&matrix_dev->lock);
+	}
  }

Quoted 23 lines · Cornelia Huck

Quoted 20 lines · On Tue, 9 Feb 2021 14:48:30 -0500 Tony Krowiak <[email protected]> wrote:

This patch fixes a circular locking dependency in the CI introduced by commit f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated"). The lockdep only occurs when starting a Secure Execution guest. Crypto virtualization (vfio_ap) is not yet supported for SE guests; however, in order to avoid CI errors, this fix is being provided.

The circular lockdep was introduced when the masks in the guest's APCB were taken under the matrix_dev->lock. While the lock is definitely needed to protect the setting/unsetting of the KVM pointer, it is not necessarily critical for setting the masks, so this will not be done under protection of the matrix_dev->lock.

Fixes f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated")

Cc [email protected]

Signed-off-by Tony Krowiak <[email protected]>

---
  drivers/s390/crypto/vfio_ap_ops.c | 75 ++++++++++++++++++-------------
  1 file changed, 45 insertions(+), 30 deletions(-)

Quoted 11 lines

drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb, · line 1114

Quoted 8 lines

  static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
  {

drivers/s390/crypto/vfio_ap_ops.c +1 -5

-	kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
-	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
-	vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
-	kvm_put_kvm(matrix_mdev->kvm);
-	matrix_mdev->kvm = NULL;
+	if (matrix_mdev->kvm) {  

If you're doing setting/unsetting under matrix_dev->lock, is it possible that matrix_mdev->kvm gets unset between here and the next line, as you don't hold the lock?

That is highly unlikely because the only place the matrix_mdev->kvm pointer is cleared is in this function which is called from only two
places: the notifier that handles the VFIO_GROUP_NOTIFY_SET_KVM notification when the KVM pointer is cleared; the vfio_ap_mdev_release() function which is called when the mdev fd is closed (i.e., when the guest is shut down). The fact is, with the only end-to-end implementation currently available, the notifier callback is never invoked to clear the KVM pointer because the vfio_ap_mdev_release callback is invoked first and it unregisters the notifier callback.

Having said that, I suppose there is no guarantee that there will not be different userspace clients in the future that do things in a different order. At the very least, it wouldn't hurt to protect against that as you suggest below.

Quoted 16 lines

Maybe you could
- grab a reference to kvm while holding the lock
- call the mask handling functions with that kvm reference
- lock again, drop the reference, and do the rest of the processing?

Quoted 9 lines

drivers/s390/crypto/vfio_ap_ops.c +8 -0

+		kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
+		mutex_lock(&matrix_dev->lock);
+		matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
+		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
+		kvm_put_kvm(matrix_mdev->kvm);
+		matrix_mdev->kvm = NULL;
+		mutex_unlock(&matrix_dev->lock);
+	}
  }  

Quoted 45 lines · On Wed, 10 Feb 2021 15:34:24 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 23 lines · Cornelia Huck

Quoted 20 lines · On Tue, 9 Feb 2021 14:48:30 -0500 Tony Krowiak <[email protected]> wrote:

This patch fixes a circular locking dependency in the CI introduced by commit f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated"). The lockdep only occurs when starting a Secure Execution guest. Crypto virtualization (vfio_ap) is not yet supported for SE guests; however, in order to avoid CI errors, this fix is being provided.

The circular lockdep was introduced when the masks in the guest's APCB were taken under the matrix_dev->lock. While the lock is definitely needed to protect the setting/unsetting of the KVM pointer, it is not necessarily critical for setting the masks, so this will not be done under protection of the matrix_dev->lock.

Fixes f21916ec4826 ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated")

Cc [email protected]

Signed-off-by Tony Krowiak <[email protected]>

---
   drivers/s390/crypto/vfio_ap_ops.c | 75 ++++++++++++++++++-------------
   1 file changed, 45 insertions(+), 30 deletions(-)

Quoted 11 lines

drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb, · line 1114

Quoted 8 lines

   static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
   {

drivers/s390/crypto/vfio_ap_ops.c +1 -5

-	kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
-	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
-	vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
-	kvm_put_kvm(matrix_mdev->kvm);
-	matrix_mdev->kvm = NULL;
+	if (matrix_mdev->kvm) {

If you're doing setting/unsetting under matrix_dev->lock, is it possible that matrix_mdev->kvm gets unset between here and the next line, as you don't hold the lock?

That is highly unlikely because the only place the matrix_mdev->kvm pointer is cleared is in this function which is called from only two
places: the notifier that handles the VFIO_GROUP_NOTIFY_SET_KVM notification when the KVM pointer is cleared; the vfio_ap_mdev_release() function which is called when the mdev fd is closed (i.e., when the guest is shut down). The fact is, with the only end-to-end implementation currently available, the notifier callback is never invoked to clear the KVM pointer because the vfio_ap_mdev_release callback is invoked first and it unregisters the notifier callback.

Quoted 4 lines

Having said that, I suppose there is no guarantee that there will not be different userspace clients in the future that do things in a different order. At the very least, it wouldn't hurt to protect against that as you suggest below.

Yes, if userspace is able to use the interfaces in the certain way, we should always make sure that nothing bad happens if it does so, even if known userspace applications are well-behaved.

[Can we make an 'evil userspace' test program, maybe? The hardware dependency makes this hard to run, though.]

Quoted 15 lines

Quoted 15 lines

Maybe you could
- grab a reference to kvm while holding the lock
- call the mask handling functions with that kvm reference
- lock again, drop the reference, and do the rest of the processing?

Quoted 9 lines

drivers/s390/crypto/vfio_ap_ops.c +8 -0

+		kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
+		mutex_lock(&matrix_dev->lock);
+		matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
+		vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
+		kvm_put_kvm(matrix_mdev->kvm);
+		matrix_mdev->kvm = NULL;
+		mutex_unlock(&matrix_dev->lock);
+	}
   }