Messages in this thread | | | Subject | Re: [PATCH v3 1/1] s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks | From | Tony Krowiak <> | Date | Thu, 4 Mar 2021 12:43:44 -0500 |
| |
On 3/3/21 2:47 PM, Halil Pasic wrote: > On Wed, 3 Mar 2021 12:10:11 -0500 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> On 3/3/21 10:23 AM, Halil Pasic wrote: >>> On Tue, 2 Mar 2021 15:43:22 -0500 >>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >>> >>>> This patch fixes a lockdep splat introduced by commit f21916ec4826 >>>> ("s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated"). >>>> The lockdep splat only occurs when starting a Secure Execution guest. >>>> Crypto virtualization (vfio_ap) is not yet supported for SE guests; >>>> however, in order to avoid this problem when support becomes available, >>>> this fix is being provided. >>> [..] >>> >>>> @@ -1038,14 +1116,28 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev, >>>> { >>>> struct ap_matrix_mdev *m; >>>> >>>> - list_for_each_entry(m, &matrix_dev->mdev_list, node) { >>>> - if ((m != matrix_mdev) && (m->kvm == kvm)) >>>> - return -EPERM; >>>> - } >>>> + if (kvm->arch.crypto.crycbd) { >>>> + matrix_mdev->kvm_busy = true; >>>> >>>> - matrix_mdev->kvm = kvm; >>>> - kvm_get_kvm(kvm); >>>> - kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook; >>>> + list_for_each_entry(m, &matrix_dev->mdev_list, node) { >>>> + if ((m != matrix_mdev) && (m->kvm == kvm)) { >>>> + wake_up_all(&matrix_mdev->wait_for_kvm); >>> This ain't no good. kvm_busy will remain true if we take this exit. The >>> wake_up_all() is not needed, because we hold the lock, so nobody can >>> observe it if we don't forget kvm_busy set. >>> >>> I suggest moving matrix_mdev->kvm_busy = true; after this loop, maybe right >>> before the unlock, and removing the wake_up_all(). >>> >>>> + return -EPERM; >>>> + } >>>> + } >>>> + >>>> + kvm_get_kvm(kvm); >>>> + mutex_unlock(&matrix_dev->lock); >>>> + kvm_arch_crypto_set_masks(kvm, >>>> + matrix_mdev->matrix.apm, >>>> + matrix_mdev->matrix.aqm, >>>> + matrix_mdev->matrix.adm); >>>> + mutex_lock(&matrix_dev->lock); >>>> + kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook; >>>> + matrix_mdev->kvm = kvm; >>>> + matrix_mdev->kvm_busy = false; >>>> + wake_up_all(&matrix_mdev->wait_for_kvm); >>>> + } >>>> >>>> return 0; >>>> } >>> [..] >>> >>>> @@ -1300,7 +1406,21 @@ static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev, >>>> ret = vfio_ap_mdev_get_device_info(arg); >>>> break; >>>> case VFIO_DEVICE_RESET: >>>> - ret = vfio_ap_mdev_reset_queues(mdev); >>>> + matrix_mdev = mdev_get_drvdata(mdev); >>>> + >>>> + /* >>>> + * 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 == false, >>>> + mutex_unlock(&matrix_dev->lock), >>>> + mutex_lock(&matrix_dev->lock)); >>>> + >>>> + if (matrix_mdev->kvm) >>>> + ret = vfio_ap_mdev_reset_queues(mdev); >>>> + else >>>> + ret = -ENODEV; >>> I don't think rejecting the reset is a good idea. I have you a more detailed >>> explanation of the list, where we initially discussed this question. >>> >>> How do you exect userspace to react to this -ENODEV? >> After reading your more detailed explanation, I have come to the >> conclusion that the test for matrix_mdev->kvm should not be >> performed here and the the vfio_ap_mdev_reset_queues() function >> should be called regardless. Each queue assigned to the mdev >> that is also bound to the vfio_ap driver will get reset and its >> IRQ resources cleaned up if they haven't already been and the >> other required conditions are met (i.e., see >> vfio_ap_mdev_free_irq_resources()). > My point is if !->kvm the other required conditions are not met. But > yes we can go back to unconditional vfio_ap_mdev_reset_queues(mdev), > and think about the necessity of performing a > vfio_ap_mdev_reset_queues() if !->kvm later as I proposed in the other > mail.
The other conditions may or may not have been met depending upon whether ->kvm is NULL because the VFIO_DEVICE_RESET ioctl was invoked while the matrix_dev->lock was released in the vfio_ap_mdev_unset_kvm() function. If that was the case, then there is no need to clean up the IRQ resources because it will already have been done.
On the other hand, if we don't have ->kvm because something broke, then we may be out of luck anyway. There will certainly be no way to unregister the GISC; however, it may still be possible to unpin the pages if we still have q->saved_pfn.
The point is, if the queue is bound to vfio_ap, it can be reset. If we can't clean up the IRQ resources because something is broken, then there is nothing we can do about that.
> > Regards, > Halil
| |