Re: [PATCH v2 1/1] s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks

[Halil Pasic]

On Wed, 24 Feb 2021 22:28:50 -0500
Tony Krowiak <akrowiak@xxxxxxxxxxxxx> 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.



> 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.

> 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.

> 
> 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.

Regards,
Halil