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 | Wed, 3 Mar 2021 11:41:22 -0500 |
| |
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().
Okay
> >> + 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?
The VFIO_DEVICE_RESET ioctl expects a return code. The vfio_ap_mdev_reset_queues() function can return -EIO or -EBUSY, so I would expect userspace to handle -ENODEV similarly to -EIO or any other non-zero return code. I also looked at all of the VFIO_DEVICE_RESET calls from QEMU to see how the return from the ioctl call is handled:
* ap: reports the reset failed along with the rc * ccw: doesn't check the rc * pci: kind of hard to follow without digging deep, but definitely handles non-zero rc.
I think the caller should be notified whether the queues were successfully reset or not, and why; in this case, the answer is there are no devices to reset.
> > Otherwise looks good to me! > > I've tested your branch from yesterday (which looks to me like this patch > without the above check on ->kvm and reset) for the lockdep splat, but I > didn't do any comprehensive testing -- which would ensure that we didn't > break something else in the process. With the two issues fixed, and your > word that the patch was properly tested (except for the lockdep splat > which I tested myself), I feel comfortable with moving forward with this. > > Regards, >
| |