| 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: <99c92c99-cd60-4034-8729-a90ac9a80a7b@linux.ibm.com>
Date: Tue, 12 Jul 2022 10:21:14 -0400
From: Anthony Krowiak <akrowiak@...ux.ibm.com>
To: Nicolin Chen <nicolinc@...dia.com>, kwankhede@...dia.com,
corbet@....net, hca@...ux.ibm.com, gor@...ux.ibm.com,
agordeev@...ux.ibm.com, borntraeger@...ux.ibm.com,
svens@...ux.ibm.com, zhenyuw@...ux.intel.com, zhi.a.wang@...el.com,
jani.nikula@...ux.intel.com, joonas.lahtinen@...ux.intel.com,
rodrigo.vivi@...el.com, tvrtko.ursulin@...ux.intel.com,
airlied@...ux.ie, daniel@...ll.ch, farman@...ux.ibm.com,
mjrosato@...ux.ibm.com, pasic@...ux.ibm.com, vneethv@...ux.ibm.com,
oberpar@...ux.ibm.com, freude@...ux.ibm.com, jjherne@...ux.ibm.com,
alex.williamson@...hat.com, cohuck@...hat.com, jgg@...dia.com,
kevin.tian@...el.com, hch@...radead.org
Cc: jchrist@...ux.ibm.com, kvm@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-s390@...r.kernel.org, intel-gvt-dev@...ts.freedesktop.org,
intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
terrence.xu@...el.com
Subject: Re: [PATCH v3 01/10] vfio: Make vfio_unpin_pages() return void
On 7/8/22 6:44 PM, Nicolin Chen wrote:
> There's only one caller that checks its return value with a WARN_ON_ONCE,
> while all other callers don't check the return value at all. Above that,
> an undo function should not fail. So, simplify the API to return void by
> embedding similar WARN_ONs.
>
> Also for users to pinpoint which condition fails, separate WARN_ON lines,
> yet remove the "driver->ops->unpin_pages" check, since it's unreasonable
> for callers to unpin on something totally random that wasn't even pinned.
> And remove NULL pointer checks for they would trigger oops vs. warnings.
> Note that npage is already validated in the vfio core, thus drop the same
> check in the type1 code.
>
> Suggested-by: Christoph Hellwig <hch@...radead.org>
> Reviewed-by: Christoph Hellwig <hch@....de>
> Reviewed-by: Jason Gunthorpe <jgg@...dia.com>
> Reviewed-by: Kirti Wankhede <kwankhede@...dia.com>
> Tested-by: Terrence Xu <terrence.xu@...el.com>
> Signed-off-by: Nicolin Chen <nicolinc@...dia.com>
> ---
> .../driver-api/vfio-mediated-device.rst | 2 +-
> drivers/gpu/drm/i915/gvt/kvmgt.c | 5 +----
> drivers/vfio/vfio.c | 21 +++++++------------
> drivers/vfio/vfio.h | 2 +-
> drivers/vfio/vfio_iommu_type1.c | 15 ++++++-------
> include/linux/vfio.h | 4 ++--
> 6 files changed, 18 insertions(+), 31 deletions(-)
>
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
> index d7a512676853..4307421dcaa0 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -263,7 +263,7 @@ driver::
> int vfio_pin_pages(struct vfio_device *device, unsigned long *user_pfn,
> int npage, int prot, unsigned long *phys_pfn);
>
> - int vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> + void vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> int npage);
>
> These functions call back into the back-end IOMMU module by using the pin_pages
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index e2f6c56ab342..8c67c9aba82d 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -231,18 +231,15 @@ static void intel_gvt_cleanup_vgpu_type_groups(struct intel_gvt *gvt)
> static void gvt_unpin_guest_page(struct intel_vgpu *vgpu, unsigned long gfn,
> unsigned long size)
> {
> - struct drm_i915_private *i915 = vgpu->gvt->gt->i915;
> int total_pages;
> int npage;
> - int ret;
>
> total_pages = roundup(size, PAGE_SIZE) / PAGE_SIZE;
>
> for (npage = 0; npage < total_pages; npage++) {
> unsigned long cur_gfn = gfn + npage;
>
> - ret = vfio_unpin_pages(&vgpu->vfio_device, &cur_gfn, 1);
> - drm_WARN_ON(&i915->drm, ret != 1);
> + vfio_unpin_pages(&vgpu->vfio_device, &cur_gfn, 1);
> }
> }
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 100a3d84380c..ad90adbfddc8 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1964,31 +1964,24 @@ EXPORT_SYMBOL(vfio_pin_pages);
> * PFNs should not be greater than VFIO_PIN_PAGES_MAX_ENTRIES.
> * @npage [in] : count of elements in user_pfn array. This count should not
> * be greater than VFIO_PIN_PAGES_MAX_ENTRIES.
> - * Return error or number of pages unpinned.
> */
> -int vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> - int npage)
> +void vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> + int npage)
> {
> struct vfio_container *container;
> struct vfio_iommu_driver *driver;
> - int ret;
>
> - if (!user_pfn || !npage || !vfio_assert_device_open(device))
> - return -EINVAL;
You left out the check for !user_pfn?
> + if (WARN_ON(npage <= 0 || npage > VFIO_PIN_PAGES_MAX_ENTRIES))
> + return;
>
> - if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
> - return -E2BIG;
> + if (WARN_ON(!vfio_assert_device_open(device)))
> + return;
>
> /* group->container cannot change while a vfio device is open */
> container = device->group->container;
> driver = container->iommu_driver;
> - if (likely(driver && driver->ops->unpin_pages))
> - ret = driver->ops->unpin_pages(container->iommu_data, user_pfn,
> - npage);
> - else
> - ret = -ENOTTY;
>
> - return ret;
> + driver->ops->unpin_pages(container->iommu_data, user_pfn, npage);
> }
> EXPORT_SYMBOL(vfio_unpin_pages);
>
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index a67130221151..bef4edf58138 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -53,7 +53,7 @@ struct vfio_iommu_driver_ops {
> unsigned long *user_pfn,
> int npage, int prot,
> unsigned long *phys_pfn);
> - int (*unpin_pages)(void *iommu_data,
> + void (*unpin_pages)(void *iommu_data,
> unsigned long *user_pfn, int npage);
> int (*register_notifier)(void *iommu_data,
> unsigned long *events,
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index db24062fb343..cfeea4efd625 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -948,20 +948,16 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> return ret;
> }
>
> -static int vfio_iommu_type1_unpin_pages(void *iommu_data,
> - unsigned long *user_pfn,
> - int npage)
> +static void vfio_iommu_type1_unpin_pages(void *iommu_data,
> + unsigned long *user_pfn, int npage)
> {
> struct vfio_iommu *iommu = iommu_data;
> bool do_accounting;
> int i;
>
> - if (!iommu || !user_pfn || npage <= 0)
> - return -EINVAL;
Is there a reason the checks above were not checked for WARN_ON?
> -
> /* Supported for v2 version only */
> - if (!iommu->v2)
> - return -EACCES;
> + if (WARN_ON(!iommu->v2))
> + return;
>
> mutex_lock(&iommu->lock);
>
> @@ -979,7 +975,8 @@ static int vfio_iommu_type1_unpin_pages(void *iommu_data,
> }
>
> mutex_unlock(&iommu->lock);
> - return i > 0 ? i : -EINVAL;
> +
> + WARN_ON(i != npage);
> }
>
> static long vfio_sync_unpin(struct vfio_dma *dma, struct vfio_domain *domain,
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 4d26e149db81..5348ef353029 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -159,8 +159,8 @@ bool vfio_file_has_dev(struct file *file, struct vfio_device *device);
>
> int vfio_pin_pages(struct vfio_device *device, unsigned long *user_pfn,
> int npage, int prot, unsigned long *phys_pfn);
> -int vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> - int npage);
> +void vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> + int npage);
> int vfio_dma_rw(struct vfio_device *device, dma_addr_t user_iova,
> void *data, size_t len, bool write);
>
Powered by blists - more mailing lists