patches for DPDK stable branches
 help / color / mirror / Atom feed
* [PATCH 1/5] dma/idxd: fix memory leak in pci close
       [not found] <20220408141504.1319913-1-kevin.laatz@intel.com>
@ 2022-04-08 14:15 ` Kevin Laatz
  2022-04-08 14:49   ` Bruce Richardson
  2022-04-08 14:15 ` [PATCH 2/5] dma/idxd: fix memory leak due to free on incorrect pointer Kevin Laatz
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 19+ messages in thread
From: Kevin Laatz @ 2022-04-08 14:15 UTC (permalink / raw)
  To: dev; +Cc: Kevin Laatz, stable, bruce.richardson, Xingguang He, Conor Walsh

ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
struct.

This is fixed by free'ing the struct when the last queue on the PCI
device is being closed.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org
Cc: bruce.richardson@intel.com

Reported-by: Xingguang He <xingguang.he@intel.com>
Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/idxd/idxd_common.c   |  1 +
 drivers/dma/idxd/idxd_internal.h |  2 ++
 drivers/dma/idxd/idxd_pci.c      | 34 +++++++++++++++++++++++++-------
 3 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/idxd/idxd_common.c b/drivers/dma/idxd/idxd_common.c
index ea6413cc7a..57c4c1655d 100644
--- a/drivers/dma/idxd/idxd_common.c
+++ b/drivers/dma/idxd/idxd_common.c
@@ -597,6 +597,7 @@ idxd_dmadev_create(const char *name, struct rte_device *dev,
 	dmadev->fp_obj->dev_private = idxd;
 
 	idxd->dmadev->state = RTE_DMA_DEV_READY;
+	rte_atomic16_inc(&idxd->u.pci->ref_count);
 
 	return 0;
 
diff --git a/drivers/dma/idxd/idxd_internal.h b/drivers/dma/idxd/idxd_internal.h
index 3375600217..180a8587c6 100644
--- a/drivers/dma/idxd/idxd_internal.h
+++ b/drivers/dma/idxd/idxd_internal.h
@@ -7,6 +7,7 @@
 
 #include <rte_dmadev_pmd.h>
 #include <rte_spinlock.h>
+#include <rte_atomic.h>
 
 #include "idxd_hw_defs.h"
 
@@ -33,6 +34,7 @@ struct idxd_pci_common {
 	rte_spinlock_t lk;
 
 	uint8_t wq_cfg_sz;
+	rte_atomic16_t ref_count;
 	volatile struct rte_idxd_bar0 *regs;
 	volatile uint32_t *wq_regs_base;
 	volatile struct rte_idxd_grpcfg *grp_regs;
diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index 9ca1ec64e9..7036eb938d 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -6,6 +6,7 @@
 #include <rte_devargs.h>
 #include <rte_dmadev_pmd.h>
 #include <rte_malloc.h>
+#include <rte_atomic.h>
 
 #include "idxd_internal.h"
 
@@ -115,20 +116,38 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
 {
 	struct idxd_dmadev *idxd = dev->fp_obj->dev_private;
 	uint8_t err_code;
+	int is_last_wq;
 
-	/* disable the device */
-	err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
-	if (err_code) {
-		IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
-		return err_code;
+	if (idxd_is_wq_enabled(idxd)) {
+		/* disable the wq */
+		err_code = idxd_pci_dev_command(idxd, idxd_disable_wq);
+		if (err_code) {
+			IDXD_PMD_ERR("Error disabling wq: code %#x", err_code);
+			return err_code;
+		}
+		IDXD_PMD_DEBUG("IDXD WQ disabled OK");
 	}
-	IDXD_PMD_DEBUG("IDXD Device disabled OK");
 
 	/* free device memory */
 	IDXD_PMD_DEBUG("Freeing device driver memory");
 	rte_free(idxd->batch_idx_ring);
 	rte_free(idxd->desc_ring);
 
+	/* if this is the last WQ on the device, disable the device and free
+	 * the PCI struct
+	 */
+	is_last_wq = rte_atomic16_dec_and_test(&idxd->u.pci->ref_count);
+	if (is_last_wq) {
+		/* disable the device */
+		err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
+		if (err_code) {
+			IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
+			return err_code;
+		}
+		IDXD_PMD_DEBUG("IDXD device disabled OK");
+		rte_free(idxd->u.pci);
+	}
+
 	return 0;
 }
 
@@ -159,12 +178,13 @@ init_pci_device(struct rte_pci_device *dev, struct idxd_dmadev *idxd,
 	uint8_t lg2_max_batch, lg2_max_copy_size;
 	unsigned int i, err_code;
 
-	pci = malloc(sizeof(*pci));
+	pci = rte_malloc(NULL, sizeof(*pci), 0);
 	if (pci == NULL) {
 		IDXD_PMD_ERR("%s: Can't allocate memory", __func__);
 		err_code = -1;
 		goto err;
 	}
+	memset(pci, 0, sizeof(*pci));
 	rte_spinlock_init(&pci->lk);
 
 	/* assign the bar registers, and then configure device */
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 2/5] dma/idxd: fix memory leak due to free on incorrect pointer
       [not found] <20220408141504.1319913-1-kevin.laatz@intel.com>
  2022-04-08 14:15 ` [PATCH 1/5] dma/idxd: fix memory leak in pci close Kevin Laatz
@ 2022-04-08 14:15 ` Kevin Laatz
  2022-04-08 14:52   ` Bruce Richardson
  2022-04-08 14:15 ` [PATCH 5/5] examples/dma: fix missing dma close Kevin Laatz
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 19+ messages in thread
From: Kevin Laatz @ 2022-04-08 14:15 UTC (permalink / raw)
  To: dev; +Cc: Kevin Laatz, stable, bruce.richardson, Conor Walsh

During PCI device close, any allocated memory needs to be free'd.
Currently, one of the free's is being called on an incorrect idxd_dmadev
struct member, namely 'batch_idx_ring', causing a memleak from the
pointer that should have been free'd.
This patch fixes this memleak by calling free on the correct pointer.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org
Cc: bruce.richardson@intel.com

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/idxd/idxd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index 7036eb938d..fdb1f15750 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -130,7 +130,7 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
 
 	/* free device memory */
 	IDXD_PMD_DEBUG("Freeing device driver memory");
-	rte_free(idxd->batch_idx_ring);
+	rte_free(idxd->batch_comp_ring);
 	rte_free(idxd->desc_ring);
 
 	/* if this is the last WQ on the device, disable the device and free
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 5/5] examples/dma: fix missing dma close
       [not found] <20220408141504.1319913-1-kevin.laatz@intel.com>
  2022-04-08 14:15 ` [PATCH 1/5] dma/idxd: fix memory leak in pci close Kevin Laatz
  2022-04-08 14:15 ` [PATCH 2/5] dma/idxd: fix memory leak due to free on incorrect pointer Kevin Laatz
@ 2022-04-08 14:15 ` Kevin Laatz
       [not found] ` <20220703122243.929642-1-kevin.laatz@intel.com>
       [not found] ` <20220704152751.943965-1-kevin.laatz@intel.com>
  4 siblings, 0 replies; 19+ messages in thread
From: Kevin Laatz @ 2022-04-08 14:15 UTC (permalink / raw)
  To: dev; +Cc: Kevin Laatz, stable, bruce.richardson, Chengwen Feng, Conor Walsh

The application stops all dmadevs that it used but never closed any,
meaning device cleanup was not done.
This patch adds device cleanup for all dmadevs. All devices need to be
closed for completeness, since devices not used by the application may
also have been created during PCI probe of EAL init.

Fixes: d047310407a3 ("examples/ioat: port application to dmadev API")
Cc: stable@dpdk.org
Cc: bruce.richardson@intel.com

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 examples/dma/dmafwd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/examples/dma/dmafwd.c b/examples/dma/dmafwd.c
index 608487e35c..4c612a0e0b 100644
--- a/examples/dma/dmafwd.c
+++ b/examples/dma/dmafwd.c
@@ -1097,6 +1097,12 @@ main(int argc, char **argv)
 			rte_ring_free(cfg.ports[i].rx_to_tx_ring);
 	}
 
+	/* close all dmadevs */
+	RTE_DMA_FOREACH_DEV(i) {
+		printf("Closing dmadev %d\n", i);
+		rte_dma_close(i);
+	}
+
 	/* clean up the EAL */
 	rte_eal_cleanup();
 
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/5] dma/idxd: fix memory leak in pci close
  2022-04-08 14:15 ` [PATCH 1/5] dma/idxd: fix memory leak in pci close Kevin Laatz
@ 2022-04-08 14:49   ` Bruce Richardson
  0 siblings, 0 replies; 19+ messages in thread
From: Bruce Richardson @ 2022-04-08 14:49 UTC (permalink / raw)
  To: Kevin Laatz; +Cc: dev, stable, Xingguang He, Conor Walsh

On Fri, Apr 08, 2022 at 03:15:00PM +0100, Kevin Laatz wrote:
> ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
> struct.
> 
> This is fixed by free'ing the struct when the last queue on the PCI
> device is being closed.
> 
> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> Cc: stable@dpdk.org
> Cc: bruce.richardson@intel.com
> 
> Reported-by: Xingguang He <xingguang.he@intel.com>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] dma/idxd: fix memory leak due to free on incorrect pointer
  2022-04-08 14:15 ` [PATCH 2/5] dma/idxd: fix memory leak due to free on incorrect pointer Kevin Laatz
@ 2022-04-08 14:52   ` Bruce Richardson
  2022-04-19 16:20     ` Kevin Laatz
  0 siblings, 1 reply; 19+ messages in thread
From: Bruce Richardson @ 2022-04-08 14:52 UTC (permalink / raw)
  To: Kevin Laatz; +Cc: dev, stable, Conor Walsh

On Fri, Apr 08, 2022 at 03:15:01PM +0100, Kevin Laatz wrote:
> During PCI device close, any allocated memory needs to be free'd.
> Currently, one of the free's is being called on an incorrect idxd_dmadev
> struct member, namely 'batch_idx_ring', causing a memleak from the
> pointer that should have been free'd.
> This patch fixes this memleak by calling free on the correct pointer.
> 
> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> Cc: stable@dpdk.org
> Cc: bruce.richardson@intel.com
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
>  drivers/dma/idxd/idxd_pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
> index 7036eb938d..fdb1f15750 100644
> --- a/drivers/dma/idxd/idxd_pci.c
> +++ b/drivers/dma/idxd/idxd_pci.c
> @@ -130,7 +130,7 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
>  
>  	/* free device memory */
>  	IDXD_PMD_DEBUG("Freeing device driver memory");
> -	rte_free(idxd->batch_idx_ring);
> +	rte_free(idxd->batch_comp_ring);
>  	rte_free(idxd->desc_ring);
>  
This is largely my fault, I expect, for being "smart" and allocating the
memory for both arrays from the one allocation. To clarify things, we need
to:
1) update the commit log message explaining why it's the wrong pointer,
i.e. that the two are in the one memory reservation
2) similarly add a comment to the rte_free call noting that it frees the
idx_ring too.

Alternatively, we can also consider adjusting the allocation code so both
arrays are allocated separately, and on free are freed similarly. We would,
however, need to double check that doing so introduces no perf hit.

/Bruce

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] dma/idxd: fix memory leak due to free on incorrect pointer
  2022-04-08 14:52   ` Bruce Richardson
@ 2022-04-19 16:20     ` Kevin Laatz
  0 siblings, 0 replies; 19+ messages in thread
From: Kevin Laatz @ 2022-04-19 16:20 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev, stable, Conor Walsh

On 08/04/2022 15:52, Bruce Richardson wrote:
> On Fri, Apr 08, 2022 at 03:15:01PM +0100, Kevin Laatz wrote:
>> During PCI device close, any allocated memory needs to be free'd.
>> Currently, one of the free's is being called on an incorrect idxd_dmadev
>> struct member, namely 'batch_idx_ring', causing a memleak from the
>> pointer that should have been free'd.
>> This patch fixes this memleak by calling free on the correct pointer.
>>
>> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
>> Cc: stable@dpdk.org
>> Cc: bruce.richardson@intel.com
>>
>> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
>> ---
>>   drivers/dma/idxd/idxd_pci.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
>> index 7036eb938d..fdb1f15750 100644
>> --- a/drivers/dma/idxd/idxd_pci.c
>> +++ b/drivers/dma/idxd/idxd_pci.c
>> @@ -130,7 +130,7 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
>>   
>>   	/* free device memory */
>>   	IDXD_PMD_DEBUG("Freeing device driver memory");
>> -	rte_free(idxd->batch_idx_ring);
>> +	rte_free(idxd->batch_comp_ring);
>>   	rte_free(idxd->desc_ring);
>>   
> This is largely my fault, I expect, for being "smart" and allocating the
> memory for both arrays from the one allocation. To clarify things, we need
> to:
> 1) update the commit log message explaining why it's the wrong pointer,
> i.e. that the two are in the one memory reservation
> 2) similarly add a comment to the rte_free call noting that it frees the
> idx_ring too.
>
> Alternatively, we can also consider adjusting the allocation code so both
> arrays are allocated separately, and on free are freed similarly. We would,
> however, need to double check that doing so introduces no perf hit.
>
I'll add explanations/comments where needed in v2, thanks for the feedback.

Kevin


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2 1/3] dma/idxd: fix memory leak in pci close
       [not found] ` <20220703122243.929642-1-kevin.laatz@intel.com>
@ 2022-07-03 12:22   ` Kevin Laatz
  2022-07-04 13:19     ` Bruce Richardson
  2022-07-03 12:22   ` [PATCH v2 2/3] dma/idxd: fix memory leak due to free on incorrect pointer Kevin Laatz
  2022-07-03 12:22   ` [PATCH v2 3/3] dma/idxd: fix null pointer dereference during pci remove Kevin Laatz
  2 siblings, 1 reply; 19+ messages in thread
From: Kevin Laatz @ 2022-07-03 12:22 UTC (permalink / raw)
  To: dev; +Cc: bruce.richardson, Kevin Laatz, stable, Xingguang He

ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
struct.

This is fixed by free'ing the struct when the last queue on the PCI
device is being closed.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org
Cc: bruce.richardson@intel.com

Reported-by: Xingguang He <xingguang.he@intel.com>
Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/idxd/idxd_common.c   |  2 ++
 drivers/dma/idxd/idxd_internal.h |  2 ++
 drivers/dma/idxd/idxd_pci.c      | 34 +++++++++++++++++++++++++-------
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/idxd/idxd_common.c b/drivers/dma/idxd/idxd_common.c
index c77200a457..d347bbed21 100644
--- a/drivers/dma/idxd/idxd_common.c
+++ b/drivers/dma/idxd/idxd_common.c
@@ -620,6 +620,8 @@ idxd_dmadev_create(const char *name, struct rte_device *dev,
 	dmadev->fp_obj->dev_private = idxd;
 
 	idxd->dmadev->state = RTE_DMA_DEV_READY;
+	if (idxd->u.pci != NULL)
+		rte_atomic16_inc(&idxd->u.pci->ref_count);
 
 	return 0;
 
diff --git a/drivers/dma/idxd/idxd_internal.h b/drivers/dma/idxd/idxd_internal.h
index 3375600217..180a8587c6 100644
--- a/drivers/dma/idxd/idxd_internal.h
+++ b/drivers/dma/idxd/idxd_internal.h
@@ -7,6 +7,7 @@
 
 #include <rte_dmadev_pmd.h>
 #include <rte_spinlock.h>
+#include <rte_atomic.h>
 
 #include "idxd_hw_defs.h"
 
@@ -33,6 +34,7 @@ struct idxd_pci_common {
 	rte_spinlock_t lk;
 
 	uint8_t wq_cfg_sz;
+	rte_atomic16_t ref_count;
 	volatile struct rte_idxd_bar0 *regs;
 	volatile uint32_t *wq_regs_base;
 	volatile struct rte_idxd_grpcfg *grp_regs;
diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index 65c6bbf4c1..918981f2ea 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -6,6 +6,7 @@
 #include <rte_devargs.h>
 #include <rte_dmadev_pmd.h>
 #include <rte_malloc.h>
+#include <rte_atomic.h>
 
 #include "idxd_internal.h"
 
@@ -115,20 +116,38 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
 {
 	struct idxd_dmadev *idxd = dev->fp_obj->dev_private;
 	uint8_t err_code;
+	int is_last_wq;
 
-	/* disable the device */
-	err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
-	if (err_code) {
-		IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
-		return err_code;
+	if (idxd_is_wq_enabled(idxd)) {
+		/* disable the wq */
+		err_code = idxd_pci_dev_command(idxd, idxd_disable_wq);
+		if (err_code) {
+			IDXD_PMD_ERR("Error disabling wq: code %#x", err_code);
+			return err_code;
+		}
+		IDXD_PMD_DEBUG("IDXD WQ disabled OK");
 	}
-	IDXD_PMD_DEBUG("IDXD Device disabled OK");
 
 	/* free device memory */
 	IDXD_PMD_DEBUG("Freeing device driver memory");
 	rte_free(idxd->batch_idx_ring);
 	rte_free(idxd->desc_ring);
 
+	/* if this is the last WQ on the device, disable the device and free
+	 * the PCI struct
+	 */
+	is_last_wq = rte_atomic16_dec_and_test(&idxd->u.pci->ref_count);
+	if (is_last_wq) {
+		/* disable the device */
+		err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
+		if (err_code) {
+			IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
+			return err_code;
+		}
+		IDXD_PMD_DEBUG("IDXD device disabled OK");
+		rte_free(idxd->u.pci);
+	}
+
 	return 0;
 }
 
@@ -159,12 +178,13 @@ init_pci_device(struct rte_pci_device *dev, struct idxd_dmadev *idxd,
 	uint8_t lg2_max_batch, lg2_max_copy_size;
 	unsigned int i, err_code;
 
-	pci = malloc(sizeof(*pci));
+	pci = rte_malloc(NULL, sizeof(*pci), 0);
 	if (pci == NULL) {
 		IDXD_PMD_ERR("%s: Can't allocate memory", __func__);
 		err_code = -1;
 		goto err;
 	}
+	memset(pci, 0, sizeof(*pci));
 	rte_spinlock_init(&pci->lk);
 
 	/* assign the bar registers, and then configure device */
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2 2/3] dma/idxd: fix memory leak due to free on incorrect pointer
       [not found] ` <20220703122243.929642-1-kevin.laatz@intel.com>
  2022-07-03 12:22   ` [PATCH v2 1/3] dma/idxd: fix memory leak in pci close Kevin Laatz
@ 2022-07-03 12:22   ` Kevin Laatz
  2022-07-04 13:23     ` Bruce Richardson
  2022-07-03 12:22   ` [PATCH v2 3/3] dma/idxd: fix null pointer dereference during pci remove Kevin Laatz
  2 siblings, 1 reply; 19+ messages in thread
From: Kevin Laatz @ 2022-07-03 12:22 UTC (permalink / raw)
  To: dev; +Cc: bruce.richardson, Kevin Laatz, stable

During PCI device close, any allocated memory needs to be free'd.
Currently, one of the free's is being called on an incorrect idxd_dmadev
struct member, namely 'batch_idx_ring', causing a memleak from the
pointer that should have been free'd.
This patch fixes this memleak by calling free on the correct pointer.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org
Cc: bruce.richardson@intel.com

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/idxd/idxd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index 918981f2ea..9349c56b3f 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -130,7 +130,7 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
 
 	/* free device memory */
 	IDXD_PMD_DEBUG("Freeing device driver memory");
-	rte_free(idxd->batch_idx_ring);
+	rte_free(idxd->batch_comp_ring);
 	rte_free(idxd->desc_ring);
 
 	/* if this is the last WQ on the device, disable the device and free
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2 3/3] dma/idxd: fix null pointer dereference during pci remove
       [not found] ` <20220703122243.929642-1-kevin.laatz@intel.com>
  2022-07-03 12:22   ` [PATCH v2 1/3] dma/idxd: fix memory leak in pci close Kevin Laatz
  2022-07-03 12:22   ` [PATCH v2 2/3] dma/idxd: fix memory leak due to free on incorrect pointer Kevin Laatz
@ 2022-07-03 12:22   ` Kevin Laatz
  2022-07-04 13:25     ` Bruce Richardson
  2 siblings, 1 reply; 19+ messages in thread
From: Kevin Laatz @ 2022-07-03 12:22 UTC (permalink / raw)
  To: dev; +Cc: bruce.richardson, Kevin Laatz, stable

The 'info' struct was being declared as a NULL pointer. If a NULL
pointer is passed to 'rte_dma_info_get', EINVAL is returned and the
struct is not populated. This subsequently causes a segfault when
dereferencing 'info'.

This patch fixes the issue by simply declaring 'info' as a variable and
passing its address to 'rte_dma_info_get'.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/idxd/idxd_pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index 9349c56b3f..3c1effeb84 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -379,10 +379,10 @@ idxd_dmadev_remove_pci(struct rte_pci_device *dev)
 	IDXD_PMD_INFO("Closing %s on NUMA node %d", name, dev->device.numa_node);
 
 	RTE_DMA_FOREACH_DEV(i) {
-		struct rte_dma_info *info = {0};
-		rte_dma_info_get(i, info);
-		if (strncmp(name, info->dev_name, strlen(name)) == 0)
-			idxd_dmadev_destroy(info->dev_name);
+		struct rte_dma_info info;
+		rte_dma_info_get(i, &info);
+		if (strncmp(name, info.dev_name, strlen(name)) == 0)
+			idxd_dmadev_destroy(info.dev_name);
 	}
 
 	return 0;
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/3] dma/idxd: fix memory leak in pci close
  2022-07-03 12:22   ` [PATCH v2 1/3] dma/idxd: fix memory leak in pci close Kevin Laatz
@ 2022-07-04 13:19     ` Bruce Richardson
  2022-07-04 13:34       ` Kevin Laatz
  0 siblings, 1 reply; 19+ messages in thread
From: Bruce Richardson @ 2022-07-04 13:19 UTC (permalink / raw)
  To: Kevin Laatz; +Cc: dev, stable, Xingguang He

On Sun, Jul 03, 2022 at 01:22:41PM +0100, Kevin Laatz wrote:
> ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
> struct.
> 
> This is fixed by free'ing the struct when the last queue on the PCI
> device is being closed.
> 
> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> Cc: stable@dpdk.org
> Cc: bruce.richardson@intel.com
> 
> Reported-by: Xingguang He <xingguang.he@intel.com>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
>  drivers/dma/idxd/idxd_common.c   |  2 ++
>  drivers/dma/idxd/idxd_internal.h |  2 ++
>  drivers/dma/idxd/idxd_pci.c      | 34 +++++++++++++++++++++++++-------
>  3 files changed, 31 insertions(+), 7 deletions(-)
> 

Some comments inline below.

/Bruce

> diff --git a/drivers/dma/idxd/idxd_common.c b/drivers/dma/idxd/idxd_common.c
> index c77200a457..d347bbed21 100644
> --- a/drivers/dma/idxd/idxd_common.c
> +++ b/drivers/dma/idxd/idxd_common.c
> @@ -620,6 +620,8 @@ idxd_dmadev_create(const char *name, struct rte_device *dev,
>  	dmadev->fp_obj->dev_private = idxd;
>  
>  	idxd->dmadev->state = RTE_DMA_DEV_READY;
> +	if (idxd->u.pci != NULL)
> +		rte_atomic16_inc(&idxd->u.pci->ref_count);
>  
>  	return 0;
>  

I don't think this belongs in the common code. Can it be put somewhere in
the pci-specific driver code to avoid issues, e.g. after idxd_dmadev_create
returns in probe_pci() function.


> diff --git a/drivers/dma/idxd/idxd_internal.h b/drivers/dma/idxd/idxd_internal.h
> index 3375600217..180a8587c6 100644
> --- a/drivers/dma/idxd/idxd_internal.h
> +++ b/drivers/dma/idxd/idxd_internal.h
> @@ -7,6 +7,7 @@
>  
>  #include <rte_dmadev_pmd.h>
>  #include <rte_spinlock.h>
> +#include <rte_atomic.h>
>  
>  #include "idxd_hw_defs.h"
>  
> @@ -33,6 +34,7 @@ struct idxd_pci_common {
>  	rte_spinlock_t lk;
>  
>  	uint8_t wq_cfg_sz;
> +	rte_atomic16_t ref_count;
>  	volatile struct rte_idxd_bar0 *regs;
>  	volatile uint32_t *wq_regs_base;
>  	volatile struct rte_idxd_grpcfg *grp_regs;
> diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
> index 65c6bbf4c1..918981f2ea 100644
> --- a/drivers/dma/idxd/idxd_pci.c
> +++ b/drivers/dma/idxd/idxd_pci.c
> @@ -6,6 +6,7 @@
>  #include <rte_devargs.h>
>  #include <rte_dmadev_pmd.h>
>  #include <rte_malloc.h>
> +#include <rte_atomic.h>
>  
>  #include "idxd_internal.h"
>  
> @@ -115,20 +116,38 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
>  {
>  	struct idxd_dmadev *idxd = dev->fp_obj->dev_private;
>  	uint8_t err_code;
> +	int is_last_wq;
>  
> -	/* disable the device */
> -	err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
> -	if (err_code) {
> -		IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
> -		return err_code;
> +	if (idxd_is_wq_enabled(idxd)) {
> +		/* disable the wq */
> +		err_code = idxd_pci_dev_command(idxd, idxd_disable_wq);
> +		if (err_code) {
> +			IDXD_PMD_ERR("Error disabling wq: code %#x", err_code);
> +			return err_code;
> +		}
> +		IDXD_PMD_DEBUG("IDXD WQ disabled OK");
>  	}
> -	IDXD_PMD_DEBUG("IDXD Device disabled OK");
>  
>  	/* free device memory */
>  	IDXD_PMD_DEBUG("Freeing device driver memory");
>  	rte_free(idxd->batch_idx_ring);
>  	rte_free(idxd->desc_ring);
>  
> +	/* if this is the last WQ on the device, disable the device and free
> +	 * the PCI struct
> +	 */
> +	is_last_wq = rte_atomic16_dec_and_test(&idxd->u.pci->ref_count);
> +	if (is_last_wq) {
> +		/* disable the device */
> +		err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
> +		if (err_code) {
> +			IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
> +			return err_code;
> +		}
> +		IDXD_PMD_DEBUG("IDXD device disabled OK");
> +		rte_free(idxd->u.pci);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -159,12 +178,13 @@ init_pci_device(struct rte_pci_device *dev, struct idxd_dmadev *idxd,
>  	uint8_t lg2_max_batch, lg2_max_copy_size;
>  	unsigned int i, err_code;
>  
> -	pci = malloc(sizeof(*pci));
> +	pci = rte_malloc(NULL, sizeof(*pci), 0);

Any particular reason for the change from regular malloc to rte_malloc?

>  	if (pci == NULL) {
>  		IDXD_PMD_ERR("%s: Can't allocate memory", __func__);
>  		err_code = -1;
>  		goto err;
>  	}
> +	memset(pci, 0, sizeof(*pci));
>  	rte_spinlock_init(&pci->lk);
>  
>  	/* assign the bar registers, and then configure device */
> -- 
> 2.31.1
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/3] dma/idxd: fix memory leak due to free on incorrect pointer
  2022-07-03 12:22   ` [PATCH v2 2/3] dma/idxd: fix memory leak due to free on incorrect pointer Kevin Laatz
@ 2022-07-04 13:23     ` Bruce Richardson
  2022-07-04 13:25       ` Bruce Richardson
  0 siblings, 1 reply; 19+ messages in thread
From: Bruce Richardson @ 2022-07-04 13:23 UTC (permalink / raw)
  To: Kevin Laatz; +Cc: dev, stable

On Sun, Jul 03, 2022 at 01:22:42PM +0100, Kevin Laatz wrote:
> During PCI device close, any allocated memory needs to be free'd.
> Currently, one of the free's is being called on an incorrect idxd_dmadev
> struct member, namely 'batch_idx_ring', causing a memleak from the
> pointer that should have been free'd.

I think you need to explain that the two rings are beside each other in
memory and we need to free using the pointer to the start of the block,
rather than the pointer to the middle of it.

> This patch fixes this memleak by calling free on the correct pointer.
> 
> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> Cc: stable@dpdk.org
> Cc: bruce.richardson@intel.com
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>

With more explanation in the commit log

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

> ---
>  drivers/dma/idxd/idxd_pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
> index 918981f2ea..9349c56b3f 100644
> --- a/drivers/dma/idxd/idxd_pci.c
> +++ b/drivers/dma/idxd/idxd_pci.c
> @@ -130,7 +130,7 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
>  
>  	/* free device memory */
>  	IDXD_PMD_DEBUG("Freeing device driver memory");
> -	rte_free(idxd->batch_idx_ring);
> +	rte_free(idxd->batch_comp_ring);
>  	rte_free(idxd->desc_ring);
>  
>  	/* if this is the last WQ on the device, disable the device and free
> -- 
> 2.31.1
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 3/3] dma/idxd: fix null pointer dereference during pci remove
  2022-07-03 12:22   ` [PATCH v2 3/3] dma/idxd: fix null pointer dereference during pci remove Kevin Laatz
@ 2022-07-04 13:25     ` Bruce Richardson
  0 siblings, 0 replies; 19+ messages in thread
From: Bruce Richardson @ 2022-07-04 13:25 UTC (permalink / raw)
  To: Kevin Laatz; +Cc: dev, stable

On Sun, Jul 03, 2022 at 01:22:43PM +0100, Kevin Laatz wrote:
> The 'info' struct was being declared as a NULL pointer. If a NULL
> pointer is passed to 'rte_dma_info_get', EINVAL is returned and the
> struct is not populated. This subsequently causes a segfault when
> dereferencing 'info'.
> 
> This patch fixes the issue by simply declaring 'info' as a variable and

s/as a variable/on the stack/

> passing its address to 'rte_dma_info_get'.
> 
> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

> ---
>  drivers/dma/idxd/idxd_pci.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
> index 9349c56b3f..3c1effeb84 100644
> --- a/drivers/dma/idxd/idxd_pci.c
> +++ b/drivers/dma/idxd/idxd_pci.c
> @@ -379,10 +379,10 @@ idxd_dmadev_remove_pci(struct rte_pci_device *dev)
>  	IDXD_PMD_INFO("Closing %s on NUMA node %d", name, dev->device.numa_node);
>  
>  	RTE_DMA_FOREACH_DEV(i) {
> -		struct rte_dma_info *info = {0};
> -		rte_dma_info_get(i, info);
> -		if (strncmp(name, info->dev_name, strlen(name)) == 0)
> -			idxd_dmadev_destroy(info->dev_name);
> +		struct rte_dma_info info;
> +		rte_dma_info_get(i, &info);
> +		if (strncmp(name, info.dev_name, strlen(name)) == 0)
> +			idxd_dmadev_destroy(info.dev_name);
>  	}
>  
>  	return 0;
> -- 
> 2.31.1
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/3] dma/idxd: fix memory leak due to free on incorrect pointer
  2022-07-04 13:23     ` Bruce Richardson
@ 2022-07-04 13:25       ` Bruce Richardson
  0 siblings, 0 replies; 19+ messages in thread
From: Bruce Richardson @ 2022-07-04 13:25 UTC (permalink / raw)
  To: Kevin Laatz; +Cc: dev, stable

On Mon, Jul 04, 2022 at 02:23:38PM +0100, Bruce Richardson wrote:
> On Sun, Jul 03, 2022 at 01:22:42PM +0100, Kevin Laatz wrote:
> > During PCI device close, any allocated memory needs to be free'd.
> > Currently, one of the free's is being called on an incorrect idxd_dmadev
> > struct member, namely 'batch_idx_ring', causing a memleak from the
> > pointer that should have been free'd.
> 
> I think you need to explain that the two rings are beside each other in
> memory and we need to free using the pointer to the start of the block,
> rather than the pointer to the middle of it.
> 
> > This patch fixes this memleak by calling free on the correct pointer.
> > 
> > Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> > Cc: stable@dpdk.org
> > Cc: bruce.richardson@intel.com
> > 
> > Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> 
> With more explanation in the commit log
> 

Correction (obviously!)
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/3] dma/idxd: fix memory leak in pci close
  2022-07-04 13:19     ` Bruce Richardson
@ 2022-07-04 13:34       ` Kevin Laatz
  2022-07-04 13:44         ` Bruce Richardson
  0 siblings, 1 reply; 19+ messages in thread
From: Kevin Laatz @ 2022-07-04 13:34 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev, stable, Xingguang He

On 04/07/2022 14:19, Bruce Richardson wrote:
> On Sun, Jul 03, 2022 at 01:22:41PM +0100, Kevin Laatz wrote:
>> ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
>> struct.
>>
>> This is fixed by free'ing the struct when the last queue on the PCI
>> device is being closed.
>>
>> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
>> Cc: stable@dpdk.org
>> Cc: bruce.richardson@intel.com
>>
>> Reported-by: Xingguang He <xingguang.he@intel.com>
>> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
>> ---
>>   drivers/dma/idxd/idxd_common.c   |  2 ++
>>   drivers/dma/idxd/idxd_internal.h |  2 ++
>>   drivers/dma/idxd/idxd_pci.c      | 34 +++++++++++++++++++++++++-------
>>   3 files changed, 31 insertions(+), 7 deletions(-)
>>
> Some comments inline below.
>
> /Bruce
>
>> diff --git a/drivers/dma/idxd/idxd_common.c b/drivers/dma/idxd/idxd_common.c
>> index c77200a457..d347bbed21 100644
>> --- a/drivers/dma/idxd/idxd_common.c
>> +++ b/drivers/dma/idxd/idxd_common.c
>> @@ -620,6 +620,8 @@ idxd_dmadev_create(const char *name, struct rte_device *dev,
>>   	dmadev->fp_obj->dev_private = idxd;
>>   
>>   	idxd->dmadev->state = RTE_DMA_DEV_READY;
>> +	if (idxd->u.pci != NULL)
>> +		rte_atomic16_inc(&idxd->u.pci->ref_count);
>>   
>>   	return 0;
>>   
> I don't think this belongs in the common code. Can it be put somewhere in
> the pci-specific driver code to avoid issues, e.g. after idxd_dmadev_create
> returns in probe_pci() function.

Sure, I'll look to move it in v3.

[snip]
>   
> @@ -159,12 +178,13 @@ init_pci_device(struct rte_pci_device *dev, struct idxd_dmadev *idxd,
>   	uint8_t lg2_max_batch, lg2_max_copy_size;
>   	unsigned int i, err_code;
>   
> -	pci = malloc(sizeof(*pci));
> +	pci = rte_malloc(NULL, sizeof(*pci), 0);
> Any particular reason for the change from regular malloc to rte_malloc?

Mainly consistency, its the only place in the driver where malloc is 
used instead of rte_malloc.

I have no strong preference here - I can remove the change for v3.

/Kevin


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/3] dma/idxd: fix memory leak in pci close
  2022-07-04 13:34       ` Kevin Laatz
@ 2022-07-04 13:44         ` Bruce Richardson
  0 siblings, 0 replies; 19+ messages in thread
From: Bruce Richardson @ 2022-07-04 13:44 UTC (permalink / raw)
  To: Kevin Laatz; +Cc: dev, stable, Xingguang He

On Mon, Jul 04, 2022 at 02:34:31PM +0100, Kevin Laatz wrote:
> On 04/07/2022 14:19, Bruce Richardson wrote:
> > On Sun, Jul 03, 2022 at 01:22:41PM +0100, Kevin Laatz wrote:
> > > ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
> > > struct.
> > > 
> > > This is fixed by free'ing the struct when the last queue on the PCI
> > > device is being closed.
> > > 
> > > Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> > > Cc: stable@dpdk.org
> > > Cc: bruce.richardson@intel.com
> > > 
> > > Reported-by: Xingguang He <xingguang.he@intel.com>
> > > Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> > > ---
> > >   drivers/dma/idxd/idxd_common.c   |  2 ++
> > >   drivers/dma/idxd/idxd_internal.h |  2 ++
> > >   drivers/dma/idxd/idxd_pci.c      | 34 +++++++++++++++++++++++++-------
> > >   3 files changed, 31 insertions(+), 7 deletions(-)
> > > 
> > Some comments inline below.
> > 
> > /Bruce
> > 
> > > diff --git a/drivers/dma/idxd/idxd_common.c b/drivers/dma/idxd/idxd_common.c
> > > index c77200a457..d347bbed21 100644
> > > --- a/drivers/dma/idxd/idxd_common.c
> > > +++ b/drivers/dma/idxd/idxd_common.c
> > > @@ -620,6 +620,8 @@ idxd_dmadev_create(const char *name, struct rte_device *dev,
> > >   	dmadev->fp_obj->dev_private = idxd;
> > >   	idxd->dmadev->state = RTE_DMA_DEV_READY;
> > > +	if (idxd->u.pci != NULL)
> > > +		rte_atomic16_inc(&idxd->u.pci->ref_count);
> > >   	return 0;
> > I don't think this belongs in the common code. Can it be put somewhere in
> > the pci-specific driver code to avoid issues, e.g. after idxd_dmadev_create
> > returns in probe_pci() function.
> 
> Sure, I'll look to move it in v3.
> 
> [snip]
> > @@ -159,12 +178,13 @@ init_pci_device(struct rte_pci_device *dev, struct idxd_dmadev *idxd,
> >   	uint8_t lg2_max_batch, lg2_max_copy_size;
> >   	unsigned int i, err_code;
> > -	pci = malloc(sizeof(*pci));
> > +	pci = rte_malloc(NULL, sizeof(*pci), 0);
> > Any particular reason for the change from regular malloc to rte_malloc?
> 
> Mainly consistency, its the only place in the driver where malloc is used
> instead of rte_malloc.
> 
> I have no strong preference here - I can remove the change for v3.
> 
If it's inconsistent with the rest, please do keep the change in v3.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v3 1/3] dma/idxd: fix memory leak in pci close
       [not found] ` <20220704152751.943965-1-kevin.laatz@intel.com>
@ 2022-07-04 15:27   ` Kevin Laatz
  2022-07-04 15:39     ` Bruce Richardson
  2022-07-04 15:27   ` [PATCH v3 2/3] dma/idxd: fix memory leak due to free on incorrect pointer Kevin Laatz
  2022-07-04 15:27   ` [PATCH v3 3/3] dma/idxd: fix null pointer dereference during pci remove Kevin Laatz
  2 siblings, 1 reply; 19+ messages in thread
From: Kevin Laatz @ 2022-07-04 15:27 UTC (permalink / raw)
  To: dev; +Cc: bruce.richardson, Kevin Laatz, stable, Xingguang He

ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
struct.

This is fixed by free'ing the struct when the last queue on the PCI
device is being closed.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org
Cc: bruce.richardson@intel.com

Reported-by: Xingguang He <xingguang.he@intel.com>
Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>

--
v3: move ref_count increment to pci probe
---
 drivers/dma/idxd/idxd_internal.h |  2 ++
 drivers/dma/idxd/idxd_pci.c      | 35 +++++++++++++++++++++++++-------
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/idxd/idxd_internal.h b/drivers/dma/idxd/idxd_internal.h
index 3375600217..180a8587c6 100644
--- a/drivers/dma/idxd/idxd_internal.h
+++ b/drivers/dma/idxd/idxd_internal.h
@@ -7,6 +7,7 @@
 
 #include <rte_dmadev_pmd.h>
 #include <rte_spinlock.h>
+#include <rte_atomic.h>
 
 #include "idxd_hw_defs.h"
 
@@ -33,6 +34,7 @@ struct idxd_pci_common {
 	rte_spinlock_t lk;
 
 	uint8_t wq_cfg_sz;
+	rte_atomic16_t ref_count;
 	volatile struct rte_idxd_bar0 *regs;
 	volatile uint32_t *wq_regs_base;
 	volatile struct rte_idxd_grpcfg *grp_regs;
diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index 65c6bbf4c1..fb618d34b6 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -6,6 +6,7 @@
 #include <rte_devargs.h>
 #include <rte_dmadev_pmd.h>
 #include <rte_malloc.h>
+#include <rte_atomic.h>
 
 #include "idxd_internal.h"
 
@@ -115,20 +116,38 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
 {
 	struct idxd_dmadev *idxd = dev->fp_obj->dev_private;
 	uint8_t err_code;
+	int is_last_wq;
 
-	/* disable the device */
-	err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
-	if (err_code) {
-		IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
-		return err_code;
+	if (idxd_is_wq_enabled(idxd)) {
+		/* disable the wq */
+		err_code = idxd_pci_dev_command(idxd, idxd_disable_wq);
+		if (err_code) {
+			IDXD_PMD_ERR("Error disabling wq: code %#x", err_code);
+			return err_code;
+		}
+		IDXD_PMD_DEBUG("IDXD WQ disabled OK");
 	}
-	IDXD_PMD_DEBUG("IDXD Device disabled OK");
 
 	/* free device memory */
 	IDXD_PMD_DEBUG("Freeing device driver memory");
 	rte_free(idxd->batch_idx_ring);
 	rte_free(idxd->desc_ring);
 
+	/* if this is the last WQ on the device, disable the device and free
+	 * the PCI struct
+	 */
+	is_last_wq = rte_atomic16_dec_and_test(&idxd->u.pci->ref_count);
+	if (is_last_wq) {
+		/* disable the device */
+		err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
+		if (err_code) {
+			IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
+			return err_code;
+		}
+		IDXD_PMD_DEBUG("IDXD device disabled OK");
+		rte_free(idxd->u.pci);
+	}
+
 	return 0;
 }
 
@@ -159,12 +178,13 @@ init_pci_device(struct rte_pci_device *dev, struct idxd_dmadev *idxd,
 	uint8_t lg2_max_batch, lg2_max_copy_size;
 	unsigned int i, err_code;
 
-	pci = malloc(sizeof(*pci));
+	pci = rte_malloc(NULL, sizeof(*pci), 0);
 	if (pci == NULL) {
 		IDXD_PMD_ERR("%s: Can't allocate memory", __func__);
 		err_code = -1;
 		goto err;
 	}
+	memset(pci, 0, sizeof(*pci));
 	rte_spinlock_init(&pci->lk);
 
 	/* assign the bar registers, and then configure device */
@@ -330,6 +350,7 @@ idxd_dmadev_probe_pci(struct rte_pci_driver *drv, struct rte_pci_device *dev)
 				free(idxd.u.pci);
 			return ret;
 		}
+		rte_atomic16_inc(&idxd.u.pci->ref_count);
 	}
 
 	return 0;
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v3 2/3] dma/idxd: fix memory leak due to free on incorrect pointer
       [not found] ` <20220704152751.943965-1-kevin.laatz@intel.com>
  2022-07-04 15:27   ` [PATCH v3 1/3] dma/idxd: fix memory leak in pci close Kevin Laatz
@ 2022-07-04 15:27   ` Kevin Laatz
  2022-07-04 15:27   ` [PATCH v3 3/3] dma/idxd: fix null pointer dereference during pci remove Kevin Laatz
  2 siblings, 0 replies; 19+ messages in thread
From: Kevin Laatz @ 2022-07-04 15:27 UTC (permalink / raw)
  To: dev; +Cc: bruce.richardson, Kevin Laatz, stable

During PCI device close, any allocated memory needs to be free'd.
Currently, one of the free's is being called on an incorrect idxd_dmadev
struct member, namely 'batch_idx_ring'.

At device creation, memory is allocated for both 'batch_comp_ring' and
'batch_idx_ring' simultaeneously. Calling free only on 'batch_idx_ring'
meant the first half of this memory was not being free'd, leading to the
memleak.

This patch fixes this memleak by calling free on 'batch_comp_ring' which
will free the memory for both rings.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org
Cc: bruce.richardson@intel.com

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/dma/idxd/idxd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index fb618d34b6..2c3b01cd2b 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -130,7 +130,7 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
 
 	/* free device memory */
 	IDXD_PMD_DEBUG("Freeing device driver memory");
-	rte_free(idxd->batch_idx_ring);
+	rte_free(idxd->batch_comp_ring);
 	rte_free(idxd->desc_ring);
 
 	/* if this is the last WQ on the device, disable the device and free
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v3 3/3] dma/idxd: fix null pointer dereference during pci remove
       [not found] ` <20220704152751.943965-1-kevin.laatz@intel.com>
  2022-07-04 15:27   ` [PATCH v3 1/3] dma/idxd: fix memory leak in pci close Kevin Laatz
  2022-07-04 15:27   ` [PATCH v3 2/3] dma/idxd: fix memory leak due to free on incorrect pointer Kevin Laatz
@ 2022-07-04 15:27   ` Kevin Laatz
  2 siblings, 0 replies; 19+ messages in thread
From: Kevin Laatz @ 2022-07-04 15:27 UTC (permalink / raw)
  To: dev; +Cc: bruce.richardson, Kevin Laatz, stable

The 'info' struct was being declared as a NULL pointer. If a NULL
pointer is passed to 'rte_dma_info_get', EINVAL is returned and the
struct is not populated. This subsequently causes a segfault when
dereferencing 'info'.

This patch fixes the issue by simply declaring 'info' on the stack and
passing its address to 'rte_dma_info_get'.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/dma/idxd/idxd_pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index 2c3b01cd2b..2f8ec06d9e 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -380,10 +380,10 @@ idxd_dmadev_remove_pci(struct rte_pci_device *dev)
 	IDXD_PMD_INFO("Closing %s on NUMA node %d", name, dev->device.numa_node);
 
 	RTE_DMA_FOREACH_DEV(i) {
-		struct rte_dma_info *info = {0};
-		rte_dma_info_get(i, info);
-		if (strncmp(name, info->dev_name, strlen(name)) == 0)
-			idxd_dmadev_destroy(info->dev_name);
+		struct rte_dma_info info;
+		rte_dma_info_get(i, &info);
+		if (strncmp(name, info.dev_name, strlen(name)) == 0)
+			idxd_dmadev_destroy(info.dev_name);
 	}
 
 	return 0;
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 1/3] dma/idxd: fix memory leak in pci close
  2022-07-04 15:27   ` [PATCH v3 1/3] dma/idxd: fix memory leak in pci close Kevin Laatz
@ 2022-07-04 15:39     ` Bruce Richardson
  0 siblings, 0 replies; 19+ messages in thread
From: Bruce Richardson @ 2022-07-04 15:39 UTC (permalink / raw)
  To: Kevin Laatz; +Cc: dev, stable, Xingguang He

On Mon, Jul 04, 2022 at 04:27:49PM +0100, Kevin Laatz wrote:
> ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
> struct.
> 
> This is fixed by free'ing the struct when the last queue on the PCI
> device is being closed.
> 
> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> Cc: stable@dpdk.org
> Cc: bruce.richardson@intel.com
> 
> Reported-by: Xingguang He <xingguang.he@intel.com>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> 
> --
> v3: move ref_count increment to pci probe
> ---

Acked-by: Bruce Richardson <bruce.richardson@intel.com>


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2022-07-04 15:39 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20220408141504.1319913-1-kevin.laatz@intel.com>
2022-04-08 14:15 ` [PATCH 1/5] dma/idxd: fix memory leak in pci close Kevin Laatz
2022-04-08 14:49   ` Bruce Richardson
2022-04-08 14:15 ` [PATCH 2/5] dma/idxd: fix memory leak due to free on incorrect pointer Kevin Laatz
2022-04-08 14:52   ` Bruce Richardson
2022-04-19 16:20     ` Kevin Laatz
2022-04-08 14:15 ` [PATCH 5/5] examples/dma: fix missing dma close Kevin Laatz
     [not found] ` <20220703122243.929642-1-kevin.laatz@intel.com>
2022-07-03 12:22   ` [PATCH v2 1/3] dma/idxd: fix memory leak in pci close Kevin Laatz
2022-07-04 13:19     ` Bruce Richardson
2022-07-04 13:34       ` Kevin Laatz
2022-07-04 13:44         ` Bruce Richardson
2022-07-03 12:22   ` [PATCH v2 2/3] dma/idxd: fix memory leak due to free on incorrect pointer Kevin Laatz
2022-07-04 13:23     ` Bruce Richardson
2022-07-04 13:25       ` Bruce Richardson
2022-07-03 12:22   ` [PATCH v2 3/3] dma/idxd: fix null pointer dereference during pci remove Kevin Laatz
2022-07-04 13:25     ` Bruce Richardson
     [not found] ` <20220704152751.943965-1-kevin.laatz@intel.com>
2022-07-04 15:27   ` [PATCH v3 1/3] dma/idxd: fix memory leak in pci close Kevin Laatz
2022-07-04 15:39     ` Bruce Richardson
2022-07-04 15:27   ` [PATCH v3 2/3] dma/idxd: fix memory leak due to free on incorrect pointer Kevin Laatz
2022-07-04 15:27   ` [PATCH v3 3/3] dma/idxd: fix null pointer dereference during pci remove Kevin Laatz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).