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

[Tony Krowiak]

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

Okay

> > +				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?

The VFIO_DEVICE_RESET ioctl expects a return code.
The vfio_ap_mdev_reset_queues() function can return -EIO or
-EBUSY, so I would expect userspace to handle -ENODEV
similarly to -EIO or any other non-zero return code. I also
looked at all of the VFIO_DEVICE_RESET calls from QEMU to see
how the return from the ioctl call is handled:

* ap: reports the reset failed along with the rc
* ccw: doesn't check the rc
* pci: kind of hard to follow without digging deep, but definitely
         handles non-zero rc.

I think the caller should be notified whether the queues were
successfully reset or not, and why; in this case, the answer is
there are no devices to reset.

> Otherwise looks good to me!
>
> I've tested your branch from yesterday (which looks to me like this patch
> without the above check on ->kvm and reset) for the lockdep splat, but I
> didn't do any comprehensive testing -- which would ensure that we didn't
> break something else in the process. With the two issues fixed, and your
> word that the patch was properly tested (except for the lockdep splat
> which I tested myself), I feel comfortable with moving forward with this.
>
> Regards,