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

[Jason Gunthorpe]

On Wed, May 26, 2021 at 10:28:29PM -0400, Tony Krowiak wrote:
> 
> 
> 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.

Well, it is not a 5 line patch, but it looks like 10 mins work and
some testing to me, tracking down all the uses of matrx_mdev->kvm
under the vfio_ap_mdev_unset_kvm() call does not seem difficult nor do
there seem to be so many.

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

Which is why I said unset_kvm needs to be reorganized to use the kvm
argument, not the matrixt_mdev->kvm

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

I checked and thought they already had a lock?
 
> > 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.

Usually when people start open coding locks it is often because
lockdep complained.

Open coding a lock makes lockdep stop because the lockdep code is
removed, but it doesn't fix anything.

> Clearly, I am lacking in understanding of how these locks
> interact. Any clues here?

I'd have to see the lockdep reports and look at it quite a lot
more. 

Jason