lkml.org 
[lkml]   [2021]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 1/1] s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks
    From
    Date


    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,
    >

    \
     
     \ /
      Last update: 2021-03-03 20:10    [W:4.952 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site