| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 25 Feb 2021 10:25:24 -0500
From: Tony Krowiak <akrowiak@...ux.ibm.com>
To: Halil Pasic <pasic@...ux.ibm.com>
Cc: linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org, stable@...r.kernel.org,
borntraeger@...ibm.com, cohuck@...hat.com, kwankhede@...dia.com,
pbonzini@...hat.com, alex.williamson@...hat.com,
pasic@...ux.vnet.ibm.com
Subject: Re: [PATCH v2 1/1] s390/vfio-ap: fix circular lockdep when
setting/clearing crypto masks
On 2/25/21 8:53 AM, Tony Krowiak wrote:
>
>
> On 2/25/21 6:28 AM, Halil Pasic wrote:
>> On Wed, 24 Feb 2021 22:28:50 -0500
>> Tony Krowiak<akrowiak@...ux.ibm.com> wrote:
>>
>>>>> 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;
>>>>> + struct kvm *kvm;
>>>>> +
>>>>> + if (matrix_mdev->kvm) {
>>>>> + kvm = matrix_mdev->kvm;
>>>>> + kvm_get_kvm(kvm);
>>>>> + matrix_mdev->kvm = NULL;
>>>> I think if there were two threads dong the unset in parallel, one
>>>> of them could bail out and carry on before the cleanup is done. But
>>>> since nothing much happens in release after that, I don't see an
>>>> immediate problem.
>>>>
>>>> Another thing to consider is, that setting ->kvm to NULL arms
>>>> vfio_ap_mdev_remove()...
>>> I'm not entirely sure what you mean by this, but my
>>> assumption is that you are talking about the check
>>> for matrix_mdev->kvm != NULL at the start of
>>> that function.
>> Yes I was talking about the check
>>
>> static int vfio_ap_mdev_remove(struct mdev_device *mdev)
>> {
>> struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>>
>> if (matrix_mdev->kvm)
>> return -EBUSY;
>> ...
>> kfree(matrix_mdev);
>> ...
>> }
>>
>> As you see, we bail out if kvm is still set, otherwise we clean up the
>> matrix_mdev which includes kfree-ing it. And vfio_ap_mdev_remove() is
>> initiated via the sysfs, i.e. can be initiated at any time. If we were
>> to free matrix_mdev in mdev_remove() and then carry on with kvm_unset()
>> with mutex_lock(&matrix_dev->lock); that would be bad.
>
> I agree.
>
>>
>>> The reason
>>> matrix_mdev->kvm is set to NULL before giving up
>>> the matrix_dev->lock is so that functions that check
>>> for the presence of the matrix_mdev->kvm pointer,
>>> such as assign_adapter_store() - will exit if they get
>>> control while the masks are being cleared.
>> I disagree!
>>
>> static ssize_t assign_adapter_store(struct device *dev,
>> struct device_attribute *attr,
>> const char *buf, size_t count)
>> {
>> int ret;
>> unsigned long apid;
>> struct mdev_device *mdev = mdev_from_dev(dev);
>> struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>>
>> /* If the guest is running, disallow assignment of adapter */
>> if (matrix_mdev->kvm)
>> return -EBUSY;
>>
>> We bail out when kvm != NULL, so having it set to NULL while the
>> mask are being cleared will make these not bail out.
>
> You are correct, I am an idiot.
>
>>> So what we have
>>> here is a catch-22; in other words, we have the case
>>> you pointed out above and the cases related to
>>> assigning/unassigning adapters, domains and
>>> control domains which should exit when a guest
>>> is running.
>> See above.
>
> Ditto.
>
>>> I may have an idea to resolve this. Suppose we add:
>>>
>>> struct ap_matrix_mdev {
>>> ...
>>> bool kvm_busy;
>>> ...
>>> }
>>>
>>> This flag will be set to true at the start of both the
>>> vfio_ap_mdev_set_kvm() and vfio_ap_mdev_unset_kvm()
>>> and set to false at the end. The assignment/unassignment
>>> and remove callback functions can test this flag and
>>> return -EBUSY if the flag is true. That will preclude assigning
>>> or unassigning adapters, domains and control domains when
>>> the KVM pointer is being set/unset. Likewise, removal of the
>>> mediated device will also be prevented while the KVM pointer
>>> is being set/unset.
>>>
>>> In the case of the PQAP handler function, it can wait for the
>>> set/unset of the KVM pointer as follows:
>>>
>>> /while (matrix_mdev->kvm_busy) {//
>>> // mutex_unlock(&matrix_dev->lock);//
>>> // msleep(100);//
>>> // mutex_lock(&matrix_dev->lock);//
>>> //}//
>>> //
>>> //if (!matrix_mdev->kvm)//
>>> // goto out_unlock;
>>>
>>> /What say you?
>>> //
>> I'm not sure. Since I disagree with your analysis above it is difficult
>> to deal with the conclusion. I'm not against decoupling the tracking of
>> the state of the mdev_matrix device from the value of the kvm pointer. I
>> think we should first get a common understanding of the problem, before
>> we proceed to the solution.
>
> Regardless of my brain fog regarding the testing of the
> matrix_mdev->kvm pointer, I stand by what I stated
> in the paragraphs just before the code snippet.
>
> The problem is there are 10 functions that depend upon
> the value of the matrix_mdev->kvm pointer that can get
> control while the pointer is being set/unset and the
> matrix_dev->lock is given up to set/clear the masks:
* vfio_ap_irq_enable: called by handle_pqap() when AQIC is intercepted
* vfio_ap_irq_disable: called by handle_pqap() when AQIC is intercepted
* assign_adapter_store: sysfs
* unassign_adapter_store: sysfs
* assign_domain_store: sysfs
* unassign_domain_store: sysfs
* assign__control_domain_store: sysfs
* unassign_control_domain_store: sysfs
* vfio_ap_mdev_remove: sysfs
* vfio_ap_mdev_release: mdev fd closed by userspace (i.e., qemu)If we
add the proposed flag to indicate when the matrix_mdev->kvm
> pointer is in flux, then we can check that before allowing the functions
> in the list above to proceed.
>
>> Regards,
>> Halil
>
Powered by blists - more mailing lists