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
