[PATCH 1/2] net/iavf: fix local memory leaks in TM hierarchy commit

[PATCH 1/2] net/iavf: fix local memory leaks in TM hierarchy commit

[Bruce Richardson]

The iavf_hierachy_commit function uses a number of temporary variables,
which, though small, are still leaked at function end. Clean this up by
freeing them before the function returns. Since these are not variables
that need to be in hugepage memory, also switch from using rte_zmalloc
to calloc.

Fixes: 44d0a720a538 ("net/iavf: query QoS capabilities and set queue TC mapping")
Fixes: 5779a8894d15 ("net/iavf: support queue rate limit configuration")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/intel/iavf/iavf_tm.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/net/intel/iavf/iavf_tm.c b/drivers/net/intel/iavf/iavf_tm.c
index e3492ec491..cc5c86b4ce 100644
--- a/drivers/net/intel/iavf/iavf_tm.c
+++ b/drivers/net/intel/iavf/iavf_tm.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2010-2017 Intel Corporation
  */
 #include <rte_tm_driver.h>
+#include <stdlib.h>
 
 #include "iavf.h"
 
@@ -795,8 +796,8 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
 	struct iavf_adapter *adapter =
 		IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
-	struct virtchnl_queue_tc_mapping *q_tc_mapping;
-	struct virtchnl_queues_bw_cfg *q_bw;
+	struct virtchnl_queue_tc_mapping *q_tc_mapping = NULL;
+	struct virtchnl_queues_bw_cfg *q_bw = NULL;
 	struct iavf_tm_node_list *queue_list = &vf->tm_conf.queue_list;
 	struct iavf_tm_node *tm_node;
 	struct iavf_qtc_map *qtc_map;
@@ -832,7 +833,7 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 
 	size = sizeof(*q_tc_mapping) + sizeof(q_tc_mapping->tc[0]) *
 		(vf->qos_cap->num_elem - 1);
-	q_tc_mapping = rte_zmalloc("q_tc", size, 0);
+	q_tc_mapping = calloc(1, size);
 	if (!q_tc_mapping) {
 		ret_val = IAVF_ERR_NO_MEMORY;
 		goto fail_clear;
@@ -840,7 +841,7 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 
 	size_q = sizeof(*q_bw) + sizeof(q_bw->cfg[0]) *
 		(vf->num_queue_pairs - 1);
-	q_bw = rte_zmalloc("q_bw", size_q, 0);
+	q_bw = calloc(1, size_q);
 	if (!q_bw) {
 		ret_val = IAVF_ERR_NO_MEMORY;
 		goto fail_clear;
@@ -889,8 +890,10 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 	/* store the queue TC mapping info */
 	qtc_map = rte_zmalloc("qtc_map",
 		sizeof(struct iavf_qtc_map) * q_tc_mapping->num_tc, 0);
-	if (!qtc_map)
-		return IAVF_ERR_NO_MEMORY;
+	if (!qtc_map) {
+		ret_val = IAVF_ERR_NO_MEMORY;
+		goto fail_clear;
+	}
 
 	for (i = 0; i < q_tc_mapping->num_tc; i++) {
 		q_tc_mapping->tc[i].req.start_queue_id = index;
@@ -908,6 +911,8 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 	vf->qtc_map = qtc_map;
 	if (adapter->stopped == 1)
 		vf->tm_conf.committed = true;
+	free(q_bw);
+	free(q_tc_mapping);
 	return ret_val;
 
 fail_clear:
@@ -917,5 +922,7 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 		iavf_tm_conf_init(dev);
 	}
 err:
+	free(q_bw);
+	free(q_tc_mapping);
 	return ret_val;
 }
-- 
2.53.0

[PATCH 2/2] net/iavf: fix leak of queue to traffic class mapping data

[Bruce Richardson]

On iavf TM hierarchy commit, the queue to traffic class mapping is
generated and stored in a pointer from the VF structure. However, that
data is never later freed. The data is also unnecessarily allocated from
hugepage memory.

Fix these issues by replacing rte_zmalloc with calloc, and then
appropriately freeing the memory at a) uninit of the device, b) at
failure of apply of the new settings and c) replacement of old data by
new.

Fixes: 3fd32df381f8 ("net/iavf: check Tx packet with correct UP and queue")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/intel/iavf/iavf_ethdev.c |  2 ++
 drivers/net/intel/iavf/iavf_tm.c     | 11 ++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
index 80e740ef29..1cd8c88384 100644
--- a/drivers/net/intel/iavf/iavf_ethdev.c
+++ b/drivers/net/intel/iavf/iavf_ethdev.c
@@ -2755,6 +2755,8 @@ iavf_uninit_vf(struct rte_eth_dev *dev)
 
 	rte_free(vf->qos_cap);
 	vf->qos_cap = NULL;
+	free(vf->qtc_map);
+	vf->qtc_map = NULL;
 
 	rte_free(vf->rss_lut);
 	vf->rss_lut = NULL;
diff --git a/drivers/net/intel/iavf/iavf_tm.c b/drivers/net/intel/iavf/iavf_tm.c
index cc5c86b4ce..0f39da232d 100644
--- a/drivers/net/intel/iavf/iavf_tm.c
+++ b/drivers/net/intel/iavf/iavf_tm.c
@@ -96,6 +96,9 @@ iavf_tm_conf_uninit(struct rte_eth_dev *dev)
 			     shaper_profile, node);
 		rte_free(shaper_profile);
 	}
+
+	free(vf->qtc_map);
+	vf->qtc_map = NULL;
 }
 
 static inline struct iavf_tm_node *
@@ -800,7 +803,8 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 	struct virtchnl_queues_bw_cfg *q_bw = NULL;
 	struct iavf_tm_node_list *queue_list = &vf->tm_conf.queue_list;
 	struct iavf_tm_node *tm_node;
-	struct iavf_qtc_map *qtc_map;
+	struct iavf_qtc_map *qtc_map = NULL;
+	struct iavf_qtc_map *old_qtc_map = vf->qtc_map; /* to free memory if new map assigned */
 	uint16_t size, size_q;
 	int index = 0, node_committed = 0;
 	int i, ret_val = IAVF_SUCCESS;
@@ -888,8 +892,7 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 		goto fail_clear;
 
 	/* store the queue TC mapping info */
-	qtc_map = rte_zmalloc("qtc_map",
-		sizeof(struct iavf_qtc_map) * q_tc_mapping->num_tc, 0);
+	qtc_map = calloc(q_tc_mapping->num_tc, sizeof(struct iavf_qtc_map));
 	if (!qtc_map) {
 		ret_val = IAVF_ERR_NO_MEMORY;
 		goto fail_clear;
@@ -909,6 +912,7 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 		goto fail_clear;
 
 	vf->qtc_map = qtc_map;
+	free(old_qtc_map);
 	if (adapter->stopped == 1)
 		vf->tm_conf.committed = true;
 	free(q_bw);
@@ -924,5 +928,6 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 err:
 	free(q_bw);
 	free(q_tc_mapping);
+	free(qtc_map);
 	return ret_val;
 }
-- 
2.53.0

[PATCH v2 0/6] Fix some small memory leaks

[Bruce Richardson]

AI review of a patch to traffic management code identified a couple of
small memory leaks in that function. When fixing those, I then asked AI
to identify other similar leaks other Intel drivers, leading to this
patchset.

V2: expand from 2 patches to 6 to cover some more leaks

Bruce Richardson (6):
  net/iavf: fix local memory leaks in TM hierarchy commit
  net/iavf: fix leak of queue to traffic class mapping data
  net/iavf: fix memory leak on error when adding flow parser
  net/ice: fix buffer leak in config of Tx queue TM node
  net/iavf: fix leak of flex metadata extraction field
  net/iavf: fix leak of IPsec crypto capabilities array

 drivers/net/intel/iavf/iavf_ethdev.c       |  4 +++
 drivers/net/intel/iavf/iavf_generic_flow.c |  1 +
 drivers/net/intel/iavf/iavf_ipsec_crypto.c |  2 ++
 drivers/net/intel/iavf/iavf_tm.c           | 31 +++++++++++++++-------
 drivers/net/intel/ice/ice_tm.c             |  6 +++--
 5 files changed, 33 insertions(+), 11 deletions(-)

--
2.53.0

[PATCH v2 1/6] net/iavf: fix local memory leaks in TM hierarchy commit

[Bruce Richardson]

The iavf_hierachy_commit function uses a number of temporary variables,
which, though small, are still leaked at function end. Clean this up by
freeing them before the function returns. Since these are not variables
that need to be in hugepage memory, also switch from using rte_zmalloc
to calloc.

Fixes: 44d0a720a538 ("net/iavf: query QoS capabilities and set queue TC mapping")
Fixes: 5779a8894d15 ("net/iavf: support queue rate limit configuration")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/intel/iavf/iavf_tm.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/intel/iavf/iavf_tm.c b/drivers/net/intel/iavf/iavf_tm.c
index e3492ec491..5f888d654f 100644
--- a/drivers/net/intel/iavf/iavf_tm.c
+++ b/drivers/net/intel/iavf/iavf_tm.c
@@ -1,6 +1,8 @@
 /* SPDX-License-Identifier: BSD-3-Clause
  * Copyright(c) 2010-2017 Intel Corporation
  */
+#include <stdlib.h>
+
 #include <rte_tm_driver.h>
 
 #include "iavf.h"
@@ -795,8 +797,8 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
 	struct iavf_adapter *adapter =
 		IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
-	struct virtchnl_queue_tc_mapping *q_tc_mapping;
-	struct virtchnl_queues_bw_cfg *q_bw;
+	struct virtchnl_queue_tc_mapping *q_tc_mapping = NULL;
+	struct virtchnl_queues_bw_cfg *q_bw = NULL;
 	struct iavf_tm_node_list *queue_list = &vf->tm_conf.queue_list;
 	struct iavf_tm_node *tm_node;
 	struct iavf_qtc_map *qtc_map;
@@ -832,7 +834,7 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 
 	size = sizeof(*q_tc_mapping) + sizeof(q_tc_mapping->tc[0]) *
 		(vf->qos_cap->num_elem - 1);
-	q_tc_mapping = rte_zmalloc("q_tc", size, 0);
+	q_tc_mapping = calloc(1, size);
 	if (!q_tc_mapping) {
 		ret_val = IAVF_ERR_NO_MEMORY;
 		goto fail_clear;
@@ -840,7 +842,7 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 
 	size_q = sizeof(*q_bw) + sizeof(q_bw->cfg[0]) *
 		(vf->num_queue_pairs - 1);
-	q_bw = rte_zmalloc("q_bw", size_q, 0);
+	q_bw = calloc(1, size_q);
 	if (!q_bw) {
 		ret_val = IAVF_ERR_NO_MEMORY;
 		goto fail_clear;
@@ -889,8 +891,10 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 	/* store the queue TC mapping info */
 	qtc_map = rte_zmalloc("qtc_map",
 		sizeof(struct iavf_qtc_map) * q_tc_mapping->num_tc, 0);
-	if (!qtc_map)
-		return IAVF_ERR_NO_MEMORY;
+	if (!qtc_map) {
+		ret_val = IAVF_ERR_NO_MEMORY;
+		goto fail_clear;
+	}
 
 	for (i = 0; i < q_tc_mapping->num_tc; i++) {
 		q_tc_mapping->tc[i].req.start_queue_id = index;
@@ -908,6 +912,8 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 	vf->qtc_map = qtc_map;
 	if (adapter->stopped == 1)
 		vf->tm_conf.committed = true;
+	free(q_bw);
+	free(q_tc_mapping);
 	return ret_val;
 
 fail_clear:
@@ -917,5 +923,7 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 		iavf_tm_conf_init(dev);
 	}
 err:
+	free(q_bw);
+	free(q_tc_mapping);
 	return ret_val;
 }
-- 
2.53.0

[PATCH v2 2/6] net/iavf: fix leak of queue to traffic class mapping data

[Bruce Richardson]

On iavf TM hierarchy commit, the queue to traffic class mapping is
generated and stored in a pointer from the VF structure. However, that
data is never later freed. The data is also unnecessarily allocated from
hugepage memory.

Fix these issues by replacing rte_zmalloc with calloc, and then
appropriately freeing the memory at a) uninit of the device, b) at
failure of apply of the new settings and c) replacement of old data by
new.

Fixes: 3fd32df381f8 ("net/iavf: check Tx packet with correct UP and queue")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/intel/iavf/iavf_ethdev.c |  2 ++
 drivers/net/intel/iavf/iavf_tm.c     | 11 ++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
index 80e740ef29..1cd8c88384 100644
--- a/drivers/net/intel/iavf/iavf_ethdev.c
+++ b/drivers/net/intel/iavf/iavf_ethdev.c
@@ -2755,6 +2755,8 @@ iavf_uninit_vf(struct rte_eth_dev *dev)
 
 	rte_free(vf->qos_cap);
 	vf->qos_cap = NULL;
+	free(vf->qtc_map);
+	vf->qtc_map = NULL;
 
 	rte_free(vf->rss_lut);
 	vf->rss_lut = NULL;
diff --git a/drivers/net/intel/iavf/iavf_tm.c b/drivers/net/intel/iavf/iavf_tm.c
index 5f888d654f..faa2d4b8a0 100644
--- a/drivers/net/intel/iavf/iavf_tm.c
+++ b/drivers/net/intel/iavf/iavf_tm.c
@@ -97,6 +97,9 @@ iavf_tm_conf_uninit(struct rte_eth_dev *dev)
 			     shaper_profile, node);
 		rte_free(shaper_profile);
 	}
+
+	free(vf->qtc_map);
+	vf->qtc_map = NULL;
 }
 
 static inline struct iavf_tm_node *
@@ -801,7 +804,8 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 	struct virtchnl_queues_bw_cfg *q_bw = NULL;
 	struct iavf_tm_node_list *queue_list = &vf->tm_conf.queue_list;
 	struct iavf_tm_node *tm_node;
-	struct iavf_qtc_map *qtc_map;
+	struct iavf_qtc_map *qtc_map = NULL;
+	struct iavf_qtc_map *old_qtc_map = vf->qtc_map; /* to free memory if new map assigned */
 	uint16_t size, size_q;
 	int index = 0, node_committed = 0;
 	int i, ret_val = IAVF_SUCCESS;
@@ -889,8 +893,7 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 		goto fail_clear;
 
 	/* store the queue TC mapping info */
-	qtc_map = rte_zmalloc("qtc_map",
-		sizeof(struct iavf_qtc_map) * q_tc_mapping->num_tc, 0);
+	qtc_map = calloc(q_tc_mapping->num_tc, sizeof(struct iavf_qtc_map));
 	if (!qtc_map) {
 		ret_val = IAVF_ERR_NO_MEMORY;
 		goto fail_clear;
@@ -910,6 +913,7 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 		goto fail_clear;
 
 	vf->qtc_map = qtc_map;
+	free(old_qtc_map);
 	if (adapter->stopped == 1)
 		vf->tm_conf.committed = true;
 	free(q_bw);
@@ -925,5 +929,6 @@ static int iavf_hierarchy_commit(struct rte_eth_dev *dev,
 err:
 	free(q_bw);
 	free(q_tc_mapping);
+	free(qtc_map);
 	return ret_val;
 }
-- 
2.53.0

[PATCH v2 3/6] net/iavf: fix memory leak on error when adding flow parser

[Bruce Richardson]

In iavf_register_flow_parser, the final "else" branch is an error leg
which returns immediately, without adding the newly allocated
parser_node to a TAILQ. Add a free call for that parser_node in the
error case to avoid leaking memory.

Fixes: ff2d0c345c3b ("net/iavf: support generic flow API")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/intel/iavf/iavf_generic_flow.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/intel/iavf/iavf_generic_flow.c b/drivers/net/intel/iavf/iavf_generic_flow.c
index 84bb161bd1..92ca20031c 100644
--- a/drivers/net/intel/iavf/iavf_generic_flow.c
+++ b/drivers/net/intel/iavf/iavf_generic_flow.c
@@ -1911,6 +1911,7 @@ iavf_register_parser(struct iavf_flow_parser *parser,
 		list = &vf->dist_parser_list;
 		TAILQ_INSERT_HEAD(list, parser_node, node);
 	} else {
+		rte_free(parser_node);
 		return -EINVAL;
 	}
 
-- 
2.53.0

[PATCH v2 4/6] net/ice: fix buffer leak in config of Tx queue TM node

[Bruce Richardson]

Only the error leg handled free of the adminq message buffer when
configuring a Tx queue traffic management node. Fix this by freeing the
buffer unconditionally after the adminq call. Also, remove the use of
dpdk-specific memory allocation, replacing it with generic alloc and
free routines.

Fixes: 715d449a965b ("net/ice: enhance Tx scheduler hierarchy support")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/intel/ice/ice_tm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/intel/ice/ice_tm.c b/drivers/net/intel/ice/ice_tm.c
index 94ded15fa7..2e6ef9c264 100644
--- a/drivers/net/intel/ice/ice_tm.c
+++ b/drivers/net/intel/ice/ice_tm.c
@@ -1,6 +1,8 @@
 /* SPDX-License-Identifier: BSD-3-Clause
  * Copyright(c) 2022 Intel Corporation
  */
+#include <stdlib.h>
+
 #include <rte_ethdev.h>
 #include <rte_tm_driver.h>
 
@@ -714,7 +716,7 @@ ice_tm_setup_txq_node(struct ice_pf *pf, struct ice_hw *hw, uint16_t qid, uint32
 		uint8_t txqs_moved = 0;
 		uint16_t buf_size = ice_struct_size(buf, txqs, 1);
 
-		buf = ice_malloc(hw, buf_size);
+		buf = calloc(1, buf_size);
 		if (buf == NULL)
 			return -ENOMEM;
 
@@ -727,9 +729,9 @@ ice_tm_setup_txq_node(struct ice_pf *pf, struct ice_hw *hw, uint16_t qid, uint32
 
 		int ret = ice_aq_move_recfg_lan_txq(hw, 1, true, false, false, false, 50,
 						NULL, buf, buf_size, &txqs_moved, NULL);
+		free(buf);
 		if (ret || txqs_moved == 0) {
 			PMD_DRV_LOG(ERR, "move lan queue %u failed", qid);
-			ice_free(hw, buf);
 			return ICE_ERR_PARAM;
 		}
 
-- 
2.53.0

[PATCH v2 5/6] net/iavf: fix leak of flex metadata extraction field

[Bruce Richardson]

Function iavf_init_proto_xtr() allocates vf->proto_xtr as part of the
device init sequence, but no shutdown function frees this memory again.
Add an appropriate free call to fix this memory leak.

Fixes: 12b435bf8f2f ("net/iavf: support flex desc metadata extraction")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/intel/iavf/iavf_ethdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
index 1cd8c88384..7f5b103326 100644
--- a/drivers/net/intel/iavf/iavf_ethdev.c
+++ b/drivers/net/intel/iavf/iavf_ethdev.c
@@ -2757,6 +2757,8 @@ iavf_uninit_vf(struct rte_eth_dev *dev)
 	vf->qos_cap = NULL;
 	free(vf->qtc_map);
 	vf->qtc_map = NULL;
+	rte_free(vf->proto_xtr);
+	vf->proto_xtr = NULL;
 
 	rte_free(vf->rss_lut);
 	vf->rss_lut = NULL;
-- 
2.53.0

[PATCH v2 6/6] net/iavf: fix leak of IPsec crypto capabilities array

[Bruce Richardson]

The security context setup allocates a crypto capabilities array and
stores it in iavf_sctx->crypto_capabilities. However, the security
context destroy path frees iavf_sctx and sctx directly, without first
freeing that array.

Fix this by freeing iavf_sctx->crypto_capabilities in
iavf_security_ctx_destroy() before freeing the security context itself.

Fixes: 6bc987ecb860 ("net/iavf: support IPsec inline crypto")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/intel/iavf/iavf_ipsec_crypto.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/intel/iavf/iavf_ipsec_crypto.c b/drivers/net/intel/iavf/iavf_ipsec_crypto.c
index b2e08da0aa..9c8f694de5 100644
--- a/drivers/net/intel/iavf/iavf_ipsec_crypto.c
+++ b/drivers/net/intel/iavf/iavf_ipsec_crypto.c
@@ -1572,6 +1572,8 @@ iavf_security_ctx_destroy(struct iavf_adapter *adapter)
 		return -ENODEV;
 
 	/* free and reset security data structures */
+	rte_free(iavf_sctx->crypto_capabilities);
+	iavf_sctx->crypto_capabilities = NULL;
 	rte_free(iavf_sctx);
 	rte_free(sctx);
 
-- 
2.53.0