Re: [PATCH v3 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM

[Tony Krowiak <akrowiak@xxxxxxxxxxxxx>]

On 5/20/22 10:09 AM, Matthew Rosato wrote:
> On 5/20/22 9:56 AM, Tony Krowiak wrote:
> > On 5/19/22 2:33 PM, Matthew Rosato wrote:
> > > Rather than relying on a notifier for associating the KVM with
> > > the group, let's assume that the association has already been
> > > made prior to device_open.  The first time a device is opened
> > > associate the group KVM with the device.
> > >
> > > This fixes a user-triggerable oops in GVT.
> > >
> > > Reviewed-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
> > > Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> > > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/i915/gvt/gtt.c        |  4 +-
> > >   drivers/gpu/drm/i915/gvt/gvt.h        |  3 -
> > >   drivers/gpu/drm/i915/gvt/kvmgt.c      | 82 ++++++--------------------
> > >   drivers/s390/crypto/vfio_ap_ops.c     | 35 ++---------
> > >   drivers/s390/crypto/vfio_ap_private.h |  3 -
> > >   drivers/vfio/vfio.c                   | 83 
> > > ++++++++++-----------------
> > >   include/linux/vfio.h                  |  6 +-
> > >   7 files changed, 57 insertions(+), 159 deletions(-)
> > >
> > >
> > > diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
> > > b/drivers/s390/crypto/vfio_ap_ops.c
> > > index e8914024f5b1..a7d2a95796d3 100644
> > > --- a/drivers/s390/crypto/vfio_ap_ops.c
> > > +++ b/drivers/s390/crypto/vfio_ap_ops.c
> > > @@ -1284,25 +1284,6 @@ static void vfio_ap_mdev_unset_kvm(struct 
> > > ap_matrix_mdev *matrix_mdev)
> > >       }
> > >   }
> > > -static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
> > > -                       unsigned long action, void *data)
> > > -{
> > > -    int notify_rc = NOTIFY_OK;
> > > -    struct ap_matrix_mdev *matrix_mdev;
> > > -
> > > -    if (action != VFIO_GROUP_NOTIFY_SET_KVM)
> > > -        return NOTIFY_OK;
> > > -
> > > -    matrix_mdev = container_of(nb, struct ap_matrix_mdev, 
> > > group_notifier);
> > > -
> > > -    if (!data)
> > > -        vfio_ap_mdev_unset_kvm(matrix_mdev);
> > > -    else if (vfio_ap_mdev_set_kvm(matrix_mdev, data))
> > > -        notify_rc = NOTIFY_DONE;
> > > -
> > > -    return notify_rc;
> > > -}
> > > -
> > >   static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
> > >   {
> > >       struct device *dev;
> > > @@ -1402,11 +1383,10 @@ static int vfio_ap_mdev_open_device(struct 
> > > vfio_device *vdev)
> > >       unsigned long events;
> > >       int ret;
> > > -    matrix_mdev->group_notifier.notifier_call = 
> > > vfio_ap_mdev_group_notifier;
> > > -    events = VFIO_GROUP_NOTIFY_SET_KVM;
> > > +    if (!vdev->kvm)
> > > +        return -EINVAL;
> > > -    ret = vfio_register_notifier(vdev, VFIO_GROUP_NOTIFY, &events,
> > > -                     &matrix_mdev->group_notifier);
> > > +    ret = vfio_ap_mdev_set_kvm(matrix_mdev, vdev->kvm);
> > >       if (ret)
> > >           return ret;
> >
> > I'm sorry I didn't see this with my last review, but maybe move the call
> > to vfio_ap_mdev_set_kvm(matrix_mdev, vdev->kvm) after the successful
> > registration of the IOMMU notifier? This way you won't be plugging AP 
> > queues
> > into the guest only to remove them if the registration fails.
>
> This is a pretty edge error case, and the 
> vfio_ap_mdev_unset_kvm(matrix_mdev) call at err_kvm should do the 
> proper cleanup, right?  I guess I'm wondering if it's really any 
> different than the prior code which would have registered the 
> VFIO_GROUP_NOTIFY_SET_KVM first, which would have immediately 
> triggered the notifier since the KVM was already registered to the 
> group, meaning it would haved called 
> vfio_ap_mdev_group_notifier->vfio_ap_mdev_set_kvm anyway (see 
> vfio_register_group_notifier, the "The attaching of kvm and vfio_group 
> might already happen..." comment)

You are correct, the VFIO_GROUP_NOTIFY_SET_KVM notifier will get 
triggered when it is registered; however, you may have pointed out a 
flaw in the previous version of the code. I'm guessing this notifier is 
not triggered when it is unregistered, so unless the guest is terminated 
due to a non-zero return code from the open_device callback, it will 
have access to the AP queues. In hindsight, we probably should have 
registered the IOMMU notifier first.

You make a valid point about this being an edge case and I don't think 
it's critical, so feel free to keep it as-is.

My r-b still stands.

> > > @@ -1415,12 +1395,11 @@ static int vfio_ap_mdev_open_device(struct 
> > > vfio_device *vdev)
> > >       ret = vfio_register_notifier(vdev, VFIO_IOMMU_NOTIFY, &events,
> > >                        &matrix_mdev->iommu_notifier);
> > >       if (ret)
> > > -        goto out_unregister_group;
> > > +        goto err_kvm;
> > >       return 0;
> > > -out_unregister_group:
> > > -    vfio_unregister_notifier(vdev, VFIO_GROUP_NOTIFY,
> > > -                 &matrix_mdev->group_notifier);
> > > +err_kvm:
> > > +    vfio_ap_mdev_unset_kvm(matrix_mdev);
> > >       return ret;
> > >   }