| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8b6db781-9d4e-4d64-04fa-94e45dbf8b22@linux.ibm.com>
Date: Fri, 20 May 2022 09:56:45 -0400
From: Tony Krowiak <akrowiak@...ux.ibm.com>
To: Matthew Rosato <mjrosato@...ux.ibm.com>, jgg@...dia.com,
alex.williamson@...hat.com
Cc: cohuck@...hat.com, borntraeger@...ux.ibm.com,
jjherne@...ux.ibm.com, pasic@...ux.ibm.com,
zhenyuw@...ux.intel.com, zhi.a.wang@...el.com, hch@...radead.org,
intel-gfx@...ts.freedesktop.org,
intel-gvt-dev@...ts.freedesktop.org, linux-s390@...r.kernel.org,
kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
Kevin Tian <kevin.tian@...el.com>,
Christoph Hellwig <hch@....de>
Subject: Re: [PATCH v3 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM
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@...ux.ibm.com>
> Reviewed-by: Kevin Tian <kevin.tian@...el.com>
> Reviewed-by: Christoph Hellwig <hch@....de>
> Signed-off-by: Jason Gunthorpe <jgg@...dia.com>
> Signed-off-by: Matthew Rosato <mjrosato@...ux.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.
>
> @@ -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;
> }
>
> @@ -1431,8 +1410,6 @@ static void vfio_ap_mdev_close_device(struct vfio_device *vdev)
>
> vfio_unregister_notifier(vdev, VFIO_IOMMU_NOTIFY,
> &matrix_mdev->iommu_notifier);
> - vfio_unregister_notifier(vdev, VFIO_GROUP_NOTIFY,
> - &matrix_mdev->group_notifier);
> vfio_ap_mdev_unset_kvm(matrix_mdev);
> }
>
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 648fcaf8104a..a26efd804d0d 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -81,8 +81,6 @@ struct ap_matrix {
> * @node: allows the ap_matrix_mdev struct to be added to a list
> * @matrix: the adapters, usage domains and control domains assigned to the
> * mediated matrix device.
> - * @group_notifier: notifier block used for specifying callback function for
> - * handling the VFIO_GROUP_NOTIFY_SET_KVM event
> * @iommu_notifier: notifier block used for specifying callback function for
> * handling the VFIO_IOMMU_NOTIFY_DMA_UNMAP even
> * @kvm: the struct holding guest's state
> @@ -94,7 +92,6 @@ struct ap_matrix_mdev {
> struct vfio_device vdev;
> struct list_head node;
> struct ap_matrix matrix;
> - struct notifier_block group_notifier;
> struct notifier_block iommu_notifier;
> struct kvm *kvm;
> crypto_hook pqap_hook;
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index cfcff7764403..831fc722e3f8 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1083,10 +1083,21 @@ static struct file *vfio_device_open(struct vfio_device *device)
>
> mutex_lock(&device->dev_set->lock);
> device->open_count++;
> - if (device->open_count == 1 && device->ops->open_device) {
> - ret = device->ops->open_device(device);
> - if (ret)
> - goto err_undo_count;
> + if (device->open_count == 1) {
> + /*
> + * Here we pass the KVM pointer with the group under the read
> + * lock. If the device driver will use it, it must obtain a
> + * reference and release it during close_device.
> + */
> + down_read(&device->group->group_rwsem);
> + device->kvm = device->group->kvm;
> +
> + if (device->ops->open_device) {
> + ret = device->ops->open_device(device);
> + if (ret)
> + goto err_undo_count;
> + }
> + up_read(&device->group->group_rwsem);
> }
> mutex_unlock(&device->dev_set->lock);
>
> @@ -1119,10 +1130,14 @@ static struct file *vfio_device_open(struct vfio_device *device)
>
> err_close_device:
> mutex_lock(&device->dev_set->lock);
> + down_read(&device->group->group_rwsem);
> if (device->open_count == 1 && device->ops->close_device)
> device->ops->close_device(device);
> err_undo_count:
> device->open_count--;
> + if (device->open_count == 0 && device->kvm)
> + device->kvm = NULL;
> + up_read(&device->group->group_rwsem);
> mutex_unlock(&device->dev_set->lock);
> module_put(device->dev->driver->owner);
> err_unassign_container:
> @@ -1315,9 +1330,13 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>
> mutex_lock(&device->dev_set->lock);
> vfio_assert_device_open(device);
> + down_read(&device->group->group_rwsem);
> if (device->open_count == 1 && device->ops->close_device)
> device->ops->close_device(device);
> + up_read(&device->group->group_rwsem);
> device->open_count--;
> + if (device->open_count == 0)
> + device->kvm = NULL;
> mutex_unlock(&device->dev_set->lock);
>
> module_put(device->dev->driver->owner);
> @@ -1726,8 +1745,8 @@ EXPORT_SYMBOL_GPL(vfio_file_enforced_coherent);
> * @file: VFIO group file
> * @kvm: KVM to link
> *
> - * The kvm pointer will be forwarded to all the vfio_device's attached to the
> - * VFIO file via the VFIO_GROUP_NOTIFY_SET_KVM notifier.
> + * When a VFIO device is first opened the KVM will be available in
> + * device->kvm if one was associated with the group.
> */
> void vfio_file_set_kvm(struct file *file, struct kvm *kvm)
> {
> @@ -1738,8 +1757,6 @@ void vfio_file_set_kvm(struct file *file, struct kvm *kvm)
>
> down_write(&group->group_rwsem);
> group->kvm = kvm;
> - blocking_notifier_call_chain(&group->notifier,
> - VFIO_GROUP_NOTIFY_SET_KVM, kvm);
> up_write(&group->group_rwsem);
> }
> EXPORT_SYMBOL_GPL(vfio_file_set_kvm);
> @@ -2006,7 +2023,8 @@ static int vfio_register_iommu_notifier(struct vfio_group *group,
> struct vfio_iommu_driver *driver;
> int ret;
>
> - down_read(&group->group_rwsem);
> + lockdep_assert_held_read(&group->group_rwsem);
> +
> container = group->container;
> driver = container->iommu_driver;
> if (likely(driver && driver->ops->register_notifier))
> @@ -2014,7 +2032,6 @@ static int vfio_register_iommu_notifier(struct vfio_group *group,
> events, nb);
> else
> ret = -ENOTTY;
> - up_read(&group->group_rwsem);
>
> return ret;
> }
> @@ -2026,7 +2043,8 @@ static int vfio_unregister_iommu_notifier(struct vfio_group *group,
> struct vfio_iommu_driver *driver;
> int ret;
>
> - down_read(&group->group_rwsem);
> + lockdep_assert_held_read(&group->group_rwsem);
> +
> container = group->container;
> driver = container->iommu_driver;
> if (likely(driver && driver->ops->unregister_notifier))
> @@ -2034,47 +2052,10 @@ static int vfio_unregister_iommu_notifier(struct vfio_group *group,
> nb);
> else
> ret = -ENOTTY;
> - up_read(&group->group_rwsem);
>
> return ret;
> }
>
> -static int vfio_register_group_notifier(struct vfio_group *group,
> - unsigned long *events,
> - struct notifier_block *nb)
> -{
> - int ret;
> - bool set_kvm = false;
> -
> - if (*events & VFIO_GROUP_NOTIFY_SET_KVM)
> - set_kvm = true;
> -
> - /* clear known events */
> - *events &= ~VFIO_GROUP_NOTIFY_SET_KVM;
> -
> - /* refuse to continue if still events remaining */
> - if (*events)
> - return -EINVAL;
> -
> - ret = blocking_notifier_chain_register(&group->notifier, nb);
> - if (ret)
> - return ret;
> -
> - /*
> - * The attaching of kvm and vfio_group might already happen, so
> - * here we replay once upon registration.
> - */
> - if (set_kvm) {
> - down_read(&group->group_rwsem);
> - if (group->kvm)
> - blocking_notifier_call_chain(&group->notifier,
> - VFIO_GROUP_NOTIFY_SET_KVM,
> - group->kvm);
> - up_read(&group->group_rwsem);
> - }
> - return 0;
> -}
> -
> int vfio_register_notifier(struct vfio_device *device,
> enum vfio_notify_type type, unsigned long *events,
> struct notifier_block *nb)
> @@ -2090,9 +2071,6 @@ int vfio_register_notifier(struct vfio_device *device,
> case VFIO_IOMMU_NOTIFY:
> ret = vfio_register_iommu_notifier(group, events, nb);
> break;
> - case VFIO_GROUP_NOTIFY:
> - ret = vfio_register_group_notifier(group, events, nb);
> - break;
> default:
> ret = -EINVAL;
> }
> @@ -2114,9 +2092,6 @@ int vfio_unregister_notifier(struct vfio_device *device,
> case VFIO_IOMMU_NOTIFY:
> ret = vfio_unregister_iommu_notifier(group, nb);
> break;
> - case VFIO_GROUP_NOTIFY:
> - ret = blocking_notifier_chain_unregister(&group->notifier, nb);
> - break;
> default:
> ret = -EINVAL;
> }
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 45b287826ce6..aa888cc51757 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -36,6 +36,8 @@ struct vfio_device {
> struct vfio_device_set *dev_set;
> struct list_head dev_set_list;
> unsigned int migration_flags;
> + /* Driver must reference the kvm during open_device or never touch it */
> + struct kvm *kvm;
>
> /* Members below here are private, not for driver use */
> refcount_t refcount;
> @@ -155,15 +157,11 @@ extern int vfio_dma_rw(struct vfio_device *device, dma_addr_t user_iova,
> /* each type has independent events */
> enum vfio_notify_type {
> VFIO_IOMMU_NOTIFY = 0,
> - VFIO_GROUP_NOTIFY = 1,
> };
>
> /* events for VFIO_IOMMU_NOTIFY */
> #define VFIO_IOMMU_NOTIFY_DMA_UNMAP BIT(0)
>
> -/* events for VFIO_GROUP_NOTIFY */
> -#define VFIO_GROUP_NOTIFY_SET_KVM BIT(0)
> -
> extern int vfio_register_notifier(struct vfio_device *device,
> enum vfio_notify_type type,
> unsigned long *required_events,
Powered by blists - more mailing lists