Re: [PATCH v2] virtio-mmio: fix memory leak of vm_dev
On Thu, 7 Sep 2023 14:17:16 +, Maximilian Heyne wrote: > With the recent removal of vm_dev from devres its memory is only freed > via the callback virtio_mmio_release_dev. However, this only takes > effect after device_add is called by register_virtio_device. Until then > it's an unmanaged resource and must be explicitly freed on error exit. > > This bug was discovered and resolved using Coverity Static Analysis > Security Testing (SAST) by Synopsys, Inc. > > Cc: > Fixes: 55c91fedd03d ("virtio-mmio: don't break lifecycle of vm_dev") > Signed-off-by: Maximilian Heyne Reviewed-by: Xuan Zhuo > --- > drivers/virtio/virtio_mmio.c | 19 ++- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > index 97760f611295..59892a31cf76 100644 > --- a/drivers/virtio/virtio_mmio.c > +++ b/drivers/virtio/virtio_mmio.c > @@ -631,14 +631,17 @@ static int virtio_mmio_probe(struct platform_device > *pdev) > spin_lock_init(_dev->lock); > > vm_dev->base = devm_platform_ioremap_resource(pdev, 0); > - if (IS_ERR(vm_dev->base)) > - return PTR_ERR(vm_dev->base); > + if (IS_ERR(vm_dev->base)) { > + rc = PTR_ERR(vm_dev->base); > + goto free_vm_dev; > + } > > /* Check magic value */ > magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE); > if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) { > dev_warn(>dev, "Wrong magic value 0x%08lx!\n", magic); > - return -ENODEV; > + rc = -ENODEV; > + goto free_vm_dev; > } > > /* Check device version */ > @@ -646,7 +649,8 @@ static int virtio_mmio_probe(struct platform_device *pdev) > if (vm_dev->version < 1 || vm_dev->version > 2) { > dev_err(>dev, "Version %ld not supported!\n", > vm_dev->version); > - return -ENXIO; > + rc = -ENXIO; > + goto free_vm_dev; > } > > vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID); > @@ -655,7 +659,8 @@ static int virtio_mmio_probe(struct platform_device *pdev) >* virtio-mmio device with an ID 0 is a (dummy) placeholder >* with no function. End probing now with no error reported. >*/ > - return -ENODEV; > + rc = -ENODEV; > + goto free_vm_dev; > } > vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID); > > @@ -685,6 +690,10 @@ static int virtio_mmio_probe(struct platform_device > *pdev) > put_device(_dev->vdev.dev); > > return rc; > + > +free_vm_dev: > + kfree(vm_dev); > + return rc; > } > > static int virtio_mmio_remove(struct platform_device *pdev) > -- > 2.40.1 > > > > > Amazon Development Center Germany GmbH > Krausenstr. 38 > 10117 Berlin > Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss > Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B > Sitz: Berlin > Ust-ID: DE 289 237 879 > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa: consume device_features parameter
On 9/7/2023 1:41 PM, Si-Wei Liu wrote: Hi David, Why this patch doesn't get picked in the last 4 months? Maybe the subject is not clear, but this is an iproute2 patch. Would it be possible to merge at your earliest convenience? PS, adding my R-b to the patch. Maybe I aimed this at the wrong person? I see that Stephen just announced the latest iproute2 https://lore.kernel.org/netdev/20230906093918.394a1b1d@hermes.local/ I probably also should have made sure that "iproute2" was in the subject prefix. Hi Stephen, perhaps you can help with this? Thanks, sln Thanks, -Siwei On Sat, May 13, 2023 at 12:42 AM Shannon Nelson wrote: > > From: Allen Hubbe > > Consume the parameter to device_features when parsing command line > options. Otherwise the parameter may be used again as an option name. > > # vdpa dev add ... device_features 0xdeadbeef mac 00:11:22:33:44:55 > Unknown option "0xdeadbeef" > > Fixes: a4442ce58ebb ("vdpa: allow provisioning device features") > Signed-off-by: Allen Hubbe > Reviewed-by: Shannon Nelson Reviewed-by: Si-Wei Liu > --- > vdpa/vdpa.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c > index 27647d73d498..8a2fca8647b6 100644 > --- a/vdpa/vdpa.c > +++ b/vdpa/vdpa.c > @@ -353,6 +353,8 @@ static int vdpa_argv_parse(struct vdpa *vdpa, int argc, char **argv, > >device_features); > if (err) > return err; > + > + NEXT_ARG_FWD(); > o_found |= VDPA_OPT_VDEV_FEATURES; > } else { > fprintf(stderr, "Unknown option \"%s\"\n", *argv); > -- > 2.17.1 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa: consume device_features parameter
Hi David, Why this patch doesn't get picked in the last 4 months? Maybe the subject is not clear, but this is an iproute2 patch. Would it be possible to merge at your earliest convenience? PS, adding my R-b to the patch. Thanks, -Siwei On Sat, May 13, 2023 at 12:42 AM Shannon Nelson wrote: > > From: Allen Hubbe > > Consume the parameter to device_features when parsing command line > options. Otherwise the parameter may be used again as an option name. > > # vdpa dev add ... device_features 0xdeadbeef mac 00:11:22:33:44:55 > Unknown option "0xdeadbeef" > > Fixes: a4442ce58ebb ("vdpa: allow provisioning device features") > Signed-off-by: Allen Hubbe > Reviewed-by: Shannon Nelson Reviewed-by: Si-Wei Liu > --- > vdpa/vdpa.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c > index 27647d73d498..8a2fca8647b6 100644 > --- a/vdpa/vdpa.c > +++ b/vdpa/vdpa.c > @@ -353,6 +353,8 @@ static int vdpa_argv_parse(struct vdpa *vdpa, int argc, char **argv, > >device_features); > if (err) > return err; > + > + NEXT_ARG_FWD(); > o_found |= VDPA_OPT_VDEV_FEATURES; > } else { > fprintf(stderr, "Unknown option \"%s\"\n", *argv); > -- > 2.17.1 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio-mmio: fix memory leak of vm_dev
On Thu, Sep 07, 2023 at 02:17:16PM +, Maximilian Heyne wrote: > With the recent removal of vm_dev from devres its memory is only freed > via the callback virtio_mmio_release_dev. However, this only takes > effect after device_add is called by register_virtio_device. Until then > it's an unmanaged resource and must be explicitly freed on error exit. > > This bug was discovered and resolved using Coverity Static Analysis > Security Testing (SAST) by Synopsys, Inc. > > Cc: > Fixes: 55c91fedd03d ("virtio-mmio: don't break lifecycle of vm_dev") > Signed-off-by: Maximilian Heyne Reviewed-by: Catalin Marinas Tested-by: Catalin Marinas Thanks. -- Catalin ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] iommu/virtio: Add ops->flush_iotlb_all and enable deferred flush
Hi, On 9/6/23 15:20, Jean-Philippe Brucker wrote: > On Wed, Sep 06, 2023 at 09:55:49AM +0200, Niklas Schnelle wrote: >> On Mon, 2023-09-04 at 17:33 +0100, Robin Murphy wrote: >>> On 2023-09-04 16:34, Jean-Philippe Brucker wrote: On Fri, Aug 25, 2023 at 05:21:26PM +0200, Niklas Schnelle wrote: > Add ops->flush_iotlb_all operation to enable virtio-iommu for the > dma-iommu deferred flush scheme. This results inn a significant increase in > in performance in exchange for a window in which devices can still > access previously IOMMU mapped memory. To get back to the prior behavior > iommu.strict=1 may be set on the kernel command line. Maybe add that it depends on CONFIG_IOMMU_DEFAULT_DMA_{LAZY,STRICT} as well, because I've seen kernel configs that enable either. >>> Indeed, I'd be inclined phrase it in terms of the driver now actually >>> being able to honour lazy mode when requested (which happens to be the >>> default on x86), rather than as if it might be some >>> potentially-unexpected change in behaviour. >>> >>> Thanks, >>> Robin. >> I kept running this series on a KVM guest on my private workstation >> (QEMU v8.0.4) and while running iperf3 on a passed-through Intel 82599 >> VF. I got a bunch of IOMMU events similar to the following as well as >> card resets in the host. >> >> .. >> [ 5959.338214] vfio-pci :04:10.0: AMD-Vi: Event logged [IO_PAGE_FAULT >> domain=0x0037 address=0x7b657064 flags=0x] >> [ 5963.353429] ixgbe :03:00.0 enp3s0: Detected Tx Unit Hang >> Tx Queue <0> >> TDH, TDT <93>, <9d> >> next_to_use <9d> >> next_to_clean<93> >>tx_buffer_info[next_to_clean] >> time_stamp <10019e800> >> jiffies <10019ec80> >> ... >> >> I retested on v6.5 vanilla (guest & host) and still get the above >> errors so luckily for me it doesn't seem to be caused by the new code >> but I can't reproduce it without virtio-iommu. Any idea what could >> cause this? > Adding Eric in case this looks familiar. Unfortunately no idea of what could cause those page faults. On ther other hand I mostly test on ARM and INTEL. Thanks Eric > > I don't have hardware to test this but I guess QEMU system emulation may > be able to reproduce the issue since it has an AMD IOMMU (unmaintained) > and igb, I can give that a try. > > Thanks, > Jean > > Link: https://lore.kernel.org/lkml/20230802123612.GA6142@myrica/ > Signed-off-by: Niklas Schnelle > --- > drivers/iommu/virtio-iommu.c | 12 > 1 file changed, 12 insertions(+) > > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c > index fb73dec5b953..1b7526494490 100644 > --- a/drivers/iommu/virtio-iommu.c > +++ b/drivers/iommu/virtio-iommu.c > @@ -924,6 +924,15 @@ static int viommu_iotlb_sync_map(struct iommu_domain > *domain, > return viommu_sync_req(vdomain->viommu); > } > > +static void viommu_flush_iotlb_all(struct iommu_domain *domain) > +{ > + struct viommu_domain *vdomain = to_viommu_domain(domain); > + > + if (!vdomain->nr_endpoints) > + return; As for patch 1, a NULL check in viommu_sync_req() would allow dropping this one Thanks, Jean >> Right, makes sense will move the check into viommu_sync_req() and add a >> coment that it is there fore the cases where viommu_iotlb_sync() et al >> get called before the IOMMU is set up. >> > + viommu_sync_req(vdomain->viommu); > +} > + > static void viommu_get_resv_regions(struct device *dev, struct > list_head *head) > { > struct iommu_resv_region *entry, *new_entry, *msi = NULL; > @@ -1049,6 +1058,8 @@ static bool viommu_capable(struct device *dev, enum > iommu_cap cap) > switch (cap) { > case IOMMU_CAP_CACHE_COHERENCY: > return true; > + case IOMMU_CAP_DEFERRED_FLUSH: > + return true; > default: > return false; > } > @@ -1069,6 +1080,7 @@ static struct iommu_ops viommu_ops = { > .map_pages = viommu_map_pages, > .unmap_pages= viommu_unmap_pages, > .iova_to_phys = viommu_iova_to_phys, > + .flush_iotlb_all= viommu_flush_iotlb_all, > .iotlb_sync = viommu_iotlb_sync, > .iotlb_sync_map = viommu_iotlb_sync_map, > .free = viommu_domain_free, > > -- > 2.39.2 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org