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

[Halil Pasic]

On Wed, 3 Mar 2021 12:10:11 -0500
Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:

> On 3/3/21 10:23 AM, Halil Pasic wrote:
> > On Tue,  2 Mar 2021 15:43:22 -0500
> > Tony Krowiak <akrowiak@xxxxxxxxxxxxx> 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().
> >  
> >> +				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?  
> 
> After reading your more detailed explanation, I have come to the
> conclusion that the test for matrix_mdev->kvm should not be
> performed here and the the vfio_ap_mdev_reset_queues() function
> should be called regardless. Each queue assigned to the mdev
> that is also bound to the vfio_ap driver will get reset and its
> IRQ resources cleaned up if they haven't already been and the
> other required conditions are met (i.e., see 
> vfio_ap_mdev_free_irq_resources()).

My point is if !->kvm the other required conditions are not met. But
yes we can go back to unconditional vfio_ap_mdev_reset_queues(mdev),
and think about the necessity of performing a
vfio_ap_mdev_reset_queues() if !->kvm later as I proposed in the other
mail.

Regards,
Halil