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

[Halil Pasic <pasic@linux.ibm.com>]

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?

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