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

  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(-)
  

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
  

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