From: Tony Krowiak <akrowiak@linux.ibm.com>
To: Halil Pasic <pasic@linux.ibm.com>
Cc: linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
kvm@vger.kernel.org, stable@vger.kernel.org,
borntraeger@de.ibm.com, cohuck@redhat.com, kwankhede@nvidia.com,
pbonzini@redhat.com, alex.williamson@redhat.com,
pasic@linux.vnet.ibm.com
Subject: Re: [PATCH v3 1/1] s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks
Date: Wed, 3 Mar 2021 11:41:22 -0500 [thread overview]
Message-ID: <e5cc2a81-7273-2b3e-0d4c-c6c17502bdae@linux.ibm.com> (raw)
In-Reply-To: <20210303162332.4d227dbe.pasic@linux.ibm.com>
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,
>
next prev parent reply other threads:[~2021-03-03 19:28 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-02 20:43 [PATCH v3 0/1] s390/vfio-ap: fix circular lockdep when starting SE guest Tony Krowiak
2021-03-02 20:43 ` [PATCH v3 1/1] s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks Tony Krowiak
2021-03-03 15:23 ` Halil Pasic
2021-03-03 16:41 ` Tony Krowiak [this message]
2021-03-03 19:42 ` Halil Pasic
2021-03-04 16:22 ` Tony Krowiak
2021-03-03 17:10 ` Tony Krowiak
2021-03-03 19:47 ` Halil Pasic
2021-03-04 17:43 ` Tony Krowiak
2021-03-09 10:23 ` Halil Pasic
2021-03-09 14:27 ` Tony Krowiak
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=e5cc2a81-7273-2b3e-0d4c-c6c17502bdae@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.ibm.com \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).