Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 1/1] s390/vfio-ap: fix circular lockdep when
 setting/clearing crypto masks
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
References: <20210216011547.22277-1-akrowiak@linux.ibm.com>
 <20210216011547.22277-2-akrowiak@linux.ibm.com>
 <20210223104805.6a8d1872.pasic@linux.ibm.com>
 <63bb0d61-efcd-315b-5a1a-0ef4d99600f4@linux.ibm.com>
 <20210225122824.467b8ed9.pasic@linux.ibm.com>
 <f5d5cbab-2181-2a95-8a87-b21d05405936@linux.ibm.com>
 <0cebaf32-776c-62c5-b7a7-d0e8afb02ceb@linux.ibm.com>
 <20210225163507.45c3391f.pasic@linux.ibm.com>
From:   Tony Krowiak <akrowiak@linux.ibm.com>
Message-ID: <12db0317-810c-419f-275f-0cd4e5516cf7@linux.ibm.com>
Date:   Thu, 25 Feb 2021 15:02:42 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <20210225163507.45c3391f.pasic@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 2/25/21 10:35 AM, Halil Pasic wrote:
> On Thu, 25 Feb 2021 10:25:24 -0500
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> 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@linux.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
> Something is strange with this email. It is basically the same email
> as the previous one, just broken, or?

the previous email was rejected for the kernel addresses because
I used bulleted lists which aren't acceptable. The kernel email addresses
accept text-only, so I replaced the bulleted list with the above.

>
>>> pointer is in flux, then we can check that before allowing the functions
>>> in the list above to proceed.
>>>   
>>>> Regards,
>>>> Halil
>>>