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

[Tony Krowiak <akrowiak@linux.ibm.com>]

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@linux.ibm.com>
>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>> ---
>>>   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;
>>>   }
>