[PATCH] bus: fix leak for devices without driver

[PATCH] bus: fix leak for devices without driver

[Volodymyr Fialko]

For devices not assigned to any driver, we leak a pci device object since
it is never freed from the PCI bus device list, reported by ASAN.

Fixes: 1cab1a40ea9b ("bus: cleanup devices on shutdown")

Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
---
 drivers/bus/pci/pci_common.c | 3 ++-
 drivers/bus/vdev/vdev.c      | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index bc3a7f39fe..e32a9d517a 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -448,7 +448,7 @@ pci_cleanup(void)
 		int ret = 0;
 
 		if (drv == NULL || drv->remove == NULL)
-			continue;
+			goto free;
 
 		ret = drv->remove(dev);
 		if (ret < 0) {
@@ -458,6 +458,7 @@ pci_cleanup(void)
 		dev->driver = NULL;
 		dev->device.driver = NULL;
 
+free:
 		/* free interrupt handles */
 		rte_intr_instance_free(dev->intr_handle);
 		dev->intr_handle = NULL;
diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index 41bc07dde7..7974b27295 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -578,18 +578,19 @@ vdev_cleanup(void)
 		int ret = 0;
 
 		if (dev->device.driver == NULL)
-			continue;
+			goto free;
 
 		drv = container_of(dev->device.driver, const struct rte_vdev_driver, driver);
 
 		if (drv->remove == NULL)
-			continue;
+			goto free;
 
 		ret = drv->remove(dev);
 		if (ret < 0)
 			error = -1;
 
 		dev->device.driver = NULL;
+free:
 		free(dev);
 	}
 
-- 
2.34.1

Re: [PATCH] bus: fix leak for devices without driver

[David Marchand]

On Sat, Jan 7, 2023 at 4:12 PM Volodymyr Fialko <vfialko@marvell.com> wrote:
>
> For devices not assigned to any driver, we leak a pci device object since
> it is never freed from the PCI bus device list, reported by ASAN.
>
> Fixes: 1cab1a40ea9b ("bus: cleanup devices on shutdown")

Worth copying stable.

>
> Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>

At a quick glance, the fix lgtm.
Kevin, can you double check this fix thoroughly?

Thanks.


-- 
David Marchand

Re: [PATCH] bus: fix leak for devices without driver

[Kevin Laatz]

Hi Volodymyr,

On 07/01/2023 15:12, Volodymyr Fialko wrote:
> For devices not assigned to any driver, we leak a pci device object since
> it is never freed from the PCI bus device list, reported by ASAN.
>
> Fixes: 1cab1a40ea9b ("bus: cleanup devices on shutdown")
>
> Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
> ---
>   drivers/bus/pci/pci_common.c | 3 ++-
>   drivers/bus/vdev/vdev.c      | 5 +++--
>   2 files changed, 5 insertions(+), 3 deletions(-)
>
This fix LGTM, but its tricky to reproduce so I have not been able to 
test this.

Could you add some more text to the commit message describing the issue 
and fix, please?

/Kevin

[PATCH v2] bus: fix leak for devices without driver

[Volodymyr Fialko]

During the bus scan, memory for device configuration is allocated.
Currently, if a driver wasn't attached to the device during initialization,
memory for that device will not be released at bus cleanup.
This patch address this issue and releases the memory for all allocated
devices.

Fixes: 1cab1a40ea9b ("bus: cleanup devices on shutdown")
Cc: stable@dpdk.org

Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
---
V2:
- Updated commit message.

 drivers/bus/pci/pci_common.c | 3 ++-
 drivers/bus/vdev/vdev.c      | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index e0e15fd624..3b4196a43b 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -450,7 +450,7 @@ pci_cleanup(void)
 		int ret = 0;
 
 		if (drv == NULL || drv->remove == NULL)
-			continue;
+			goto free;
 
 		ret = drv->remove(dev);
 		if (ret < 0) {
@@ -460,6 +460,7 @@ pci_cleanup(void)
 		dev->driver = NULL;
 		dev->device.driver = NULL;
 
+free:
 		/* free interrupt handles */
 		rte_intr_instance_free(dev->intr_handle);
 		dev->intr_handle = NULL;
diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index 41bc07dde7..7974b27295 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -578,18 +578,19 @@ vdev_cleanup(void)
 		int ret = 0;
 
 		if (dev->device.driver == NULL)
-			continue;
+			goto free;
 
 		drv = container_of(dev->device.driver, const struct rte_vdev_driver, driver);
 
 		if (drv->remove == NULL)
-			continue;
+			goto free;
 
 		ret = drv->remove(dev);
 		if (ret < 0)
 			error = -1;
 
 		dev->device.driver = NULL;
+free:
 		free(dev);
 	}
 
-- 
2.34.1

Re: [PATCH v2] bus: fix leak for devices without driver

[Kevin Laatz]

On 09/02/2023 13:22, Volodymyr Fialko wrote:
> During the bus scan, memory for device configuration is allocated.
> Currently, if a driver wasn't attached to the device during initialization,
> memory for that device will not be released at bus cleanup.
> This patch address this issue and releases the memory for all allocated
> devices.
>
> Fixes: 1cab1a40ea9b ("bus: cleanup devices on shutdown")
> Cc: stable@dpdk.org
>
> Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
> ---
> V2:
> - Updated commit message.
>
>   drivers/bus/pci/pci_common.c | 3 ++-
>   drivers/bus/vdev/vdev.c      | 5 +++--
>   2 files changed, 5 insertions(+), 3 deletions(-)
>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>

Re: [PATCH v2] bus: fix leak for devices without driver

[David Marchand]

On Fri, Feb 10, 2023 at 10:29 AM Kevin Laatz <kevin.laatz@intel.com> wrote:
>
> On 09/02/2023 13:22, Volodymyr Fialko wrote:
> > During the bus scan, memory for device configuration is allocated.
> > Currently, if a driver wasn't attached to the device during initialization,
> > memory for that device will not be released at bus cleanup.
> > This patch address this issue and releases the memory for all allocated
> > devices.
> >
> > Fixes: 1cab1a40ea9b ("bus: cleanup devices on shutdown")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
> Acked-by: Kevin Laatz <kevin.laatz@intel.com>

Applied, thanks.


-- 
David Marchand