All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tony Krowiak <akrowiak@linux.ibm.com>
To: Cornelia Huck <cohuck@redhat.com>
Cc: linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org, stable@vger.kernel.org,
	borntraeger@de.ibm.com, kwankhede@nvidia.com,
	pbonzini@redhat.com, alex.williamson@redhat.com,
	pasic@linux.vnet.ibm.com
Subject: Re: [PATCH 1/1] s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks
Date: Thu, 11 Feb 2021 09:38:35 -0500	[thread overview]
Message-ID: <357fe77e-eee3-9e83-d7bf-e59edf814045@linux.ibm.com> (raw)
In-Reply-To: <20210211132306.64249174.cohuck@redhat.com>



On 2/11/21 7:23 AM, Cornelia Huck wrote:
> On Wed, 10 Feb 2021 15:34:24 -0500
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> On 2/10/21 5:53 AM, Cornelia Huck wrote:
>>> On Tue,  9 Feb 2021 14:48:30 -0500
>>> Tony Krowiak <akrowiak@linux.ibm.com> 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: stable@vger.kernel.org
>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>>> ---
>>>>    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)
>>>>    {
>>>> -	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.
> 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.]

Of course it is possible to create such a test program, but off the
top of my head, I can't come up with an algorithm that would
result in the scenario you have laid out. I haven't dabbled in the QEMU
space in quite some time; so, there would also be a bit of a re-learning
curve. I'm not sure it would be worth the effort to take this on given
how unlikely it is this scenario can happen, but I will take it into
consideration as it is a good idea.

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


      reply	other threads:[~2021-02-11 14:49 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-09 19:48 [PATCH 0/1] fix circular lockdep when staring SE guest Tony Krowiak
2021-02-09 19:48 ` [PATCH 1/1] s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks Tony Krowiak
2021-02-10 10:53   ` Cornelia Huck
2021-02-10 15:24     ` Halil Pasic
2021-02-10 15:32       ` Halil Pasic
2021-02-10 22:05         ` Tony Krowiak
2021-02-10 22:46           ` Halil Pasic
2021-02-11 14:21             ` Tony Krowiak
2021-02-11 16:47               ` Halil Pasic
2021-02-11 19:18                 ` Tony Krowiak
2021-02-10 22:03       ` Tony Krowiak
2021-02-10 20:34     ` Tony Krowiak
2021-02-11 12:23       ` Cornelia Huck
2021-02-11 14:38         ` Tony Krowiak [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=357fe77e-eee3-9e83-d7bf-e59edf814045@linux.ibm.com \
    --to=akrowiak@linux.ibm.com \
    --cc=alex.williamson@redhat.com \
    --cc=borntraeger@de.ibm.com \
    --cc=cohuck@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=kwankhede@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=pasic@linux.vnet.ibm.com \
    --cc=pbonzini@redhat.com \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.