Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Tony Krowiak]

On 5/25/21 12:29 PM, Jason Gunthorpe wrote:
> On Tue, May 25, 2021 at 11:56:50AM -0400, Tony Krowiak wrote:
>
> > The vfio_ap_mdev_unset_kvm() function, however, is called both by
> > the group notifier when the KVM pointer has been cleared or when the
> > mdev is being removed. In both cases, the only way to get the KVM
> > pointer - which is needed to unplug the AP resources from the guest
> > - is from the matrix_mdev which contains it.
> Okay, but that isn't a problem, the matrix dev holds a ref on the kvm
> pointer so we can just copy it outside the lock after we prevent it
> from changing by unregistering the notifier:
>
> @@ -1362,14 +1365,19 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
>   {
>          struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>   
> -       mutex_lock(&matrix_dev->lock);
> -       vfio_ap_mdev_unset_kvm(matrix_mdev);
> -       mutex_unlock(&matrix_dev->lock);
> -
>          vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>                                   &matrix_mdev->iommu_notifier);
>          vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>                                   &matrix_mdev->group_notifier);
> +
> +       mutex_lock(&matrix_dev->lock);
> +       /* matrix_dev->kvm cannot be changed now since we removed the notifiers */
> +       kvm = matrix_mdev->kvm;
> +       matrix_mdev->kvm = NULL;
> +       mutex_unlock(&matrix_dev->lock);
> +
> +       vfio_ap_mdev_unset_kvm(matrix_mdev, kvm);
> +
>          module_put(THIS_MODULE);
>
> Note the above misordering is an existing bug too
>
> And reoganize unset_kvm so it uses internal locking and gets the kvm
> from the argument.

As I told you in a previous email, this is not a trivial exercise.
If you take a look at the vfio_ap_mdev_unset_kvm()
function, you will notice that it invokes the vfio_ap_mdev_reset_queues()
function to reset the queues after taking them away from the
guest. As each queue is reset, the resources required for
processing interrupts for the queue are freed in the
vfio_ap_free_aqic_resources() function. In order to unregister the
the guest's ISC, the matrix_mdev->kvm pointer must still
be set, however, you cleared it above.

Another thing you're overlooking is the fact that all of the
assignment/unassignment functions associated with the
corresponding syfs attributes of the mdev change the
content of the matrix_mdev->matrix and
matrix_mdev->shadow_apcb structures. In particular,
the matrix_mdev->matrix contains the APQNs of the
queues that must be reset. These sysfs attributes can
be accessed at any time including when the
vfio_ap_mdev_unset_kvm() function is executing,
so that is something that must also be taken into
consideration.


> Also the kvm_busy should be replaced by a proper rwsem, don't try to
> open code locks like that - it just defeats lockdep analysis.

I've had no luck trying to refactor this using rwsem. I always
run into lockdep problems between the matrix_dev->lock
and matrix_mdev->rwsem, even if the locking order is maintained.
Clearly, I am lacking in understanding of how these locks
interact. Any clues here?

> Finally, since the only way the ->kvm can be become non-NULL is if the
> notifier is registered, release above removes the notifier, and remove
> can't be called unless release has been completed, it looks to me like
> this the remove check is just dead code, delete it, or leave it as a
> WARN_ON:
>
> @@ -366,16 +366,6 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev)
>          struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>
>          mutex_lock(&matrix_dev->lock);
> -
> -       /*
> -        * If the KVM pointer is in flux or the guest is running, disallow
> -        * un-assignment of control domain.
> -        */
> -       if (matrix_mdev->kvm_busy || matrix_mdev->kvm) {
> -               mutex_unlock(&matrix_dev->lock);
> -               return -EBUSY;
> -       }
>
> Jason