[PATCH 1/5] dma/idxd: fix memory leak in pci close

[PATCH 1/5] dma/idxd: fix memory leak in pci close

[Kevin Laatz]

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

Re: [PATCH 1/5] dma/idxd: fix memory leak in pci close

[Bruce Richardson]

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>

[PATCH 2/5] dma/idxd: fix memory leak due to free on incorrect pointer

[Kevin Laatz]

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

Re: [PATCH 2/5] dma/idxd: fix memory leak due to free on incorrect pointer

[Bruce Richardson]

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

Re: [PATCH 2/5] dma/idxd: fix memory leak due to free on incorrect pointer

[Kevin Laatz]

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

[PATCH 5/5] examples/dma: fix missing dma close

[Kevin Laatz]

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

[PATCH v2 1/3] dma/idxd: fix memory leak in pci close

[Kevin Laatz]

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

Re: [PATCH v2 1/3] dma/idxd: fix memory leak in pci close

[Bruce Richardson]

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
> 

Re: [PATCH v2 1/3] dma/idxd: fix memory leak in pci close

[Kevin Laatz]

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

Re: [PATCH v2 1/3] dma/idxd: fix memory leak in pci close

[Bruce Richardson]

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.

[PATCH v2 2/3] dma/idxd: fix memory leak due to free on incorrect pointer

[Kevin Laatz]

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

Re: [PATCH v2 2/3] dma/idxd: fix memory leak due to free on incorrect pointer

[Bruce Richardson]

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
> 

Re: [PATCH v2 2/3] dma/idxd: fix memory leak due to free on incorrect pointer

[Bruce Richardson]

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>

[PATCH v2 3/3] dma/idxd: fix null pointer dereference during pci remove

[Kevin Laatz]

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

Re: [PATCH v2 3/3] dma/idxd: fix null pointer dereference during pci remove

[Bruce Richardson]

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
> 

[PATCH v3 1/3] dma/idxd: fix memory leak in pci close

[Kevin Laatz]

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

Re: [PATCH v3 1/3] dma/idxd: fix memory leak in pci close

[Bruce Richardson]

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>

[PATCH v3 2/3] dma/idxd: fix memory leak due to free on incorrect pointer

[Kevin Laatz]

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

[PATCH v3 3/3] dma/idxd: fix null pointer dereference during pci remove

[Kevin Laatz]

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