lkml.org 
[lkml]   [2021]   [Mar]   [4]   [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 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

\
 
 \ /
  Last update: 2021-03-04 18:58    [W:0.574 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site