Skip to contentSkip to conversation

net/ice: fix memory handling issues

PATCH In review Anurag Mandal 2 participants 5 messages Oct 7 Network drivers

lore
Share & tools
lore

Anurag Mandal

author
#0 lore raw

cover net/ice: fix memory handling issues

This patch series addresses two memory handling issues in the ice PMD.

- The first one makes memory allocation and release symmetric. Several buffers obtained through ice_malloc(), ice_calloc() or ice_memdup() were released with rte_free() instead of ice_free(). Both APIs currently map to the same underlying allocator. However, mixing them is inconsistent and would break if the OS abstraction layer ever changes.

- The second one fixes an unchecked allocation in ice_tx_queue_start() which dereferences the Tx time queue context buffer returned by ice_malloc() without NULL check.
On allocation failure, the E830 send-on-timestamp path therefore crashes instead of reporting "No Memory" error, and also the already allocated txq_elem buffer gets leaked.

Anurag Mandal (2):
net/ice: free memory with matching allocator net/ice: fix unchecked Tx time context allocation

 drivers/net/intel/ice/ice_acl_filter.c  | 10 +++++-----
 drivers/net/intel/ice/ice_dcf.c         |  2 +-
 drivers/net/intel/ice/ice_dcf_parent.c  |  2 +-
 drivers/net/intel/ice/ice_ethdev.c      | 14 +++++++-------
 drivers/net/intel/ice/ice_fdir_filter.c | 12 ++++++------
 drivers/net/intel/ice/ice_rxtx.c        | 22 +++++++++++++---------
 6 files changed, 33 insertions(+), 29 deletions(-)
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH 1/2 net/ice: free memory with matching allocator

Several buffers allocated with ice_malloc(), ice_calloc() or ice_memdup() were released using rte_free() instead of ice_free(). Both APIs currently map to the same underlying allocator. However, mixing them is inconsistent and could break if the inconsistent and could break if the OS abstraction layer changes.

Use ice_free() for all memory obtained through the ice_malloc() family.

Signed-off-by Anurag Mandal <[email protected]>
---
 drivers/net/intel/ice/ice_acl_filter.c  | 10 +++++-----
 drivers/net/intel/ice/ice_dcf.c         |  2 +-
 drivers/net/intel/ice/ice_dcf_parent.c  |  2 +-
 drivers/net/intel/ice/ice_ethdev.c      | 14 +++++++-------
 drivers/net/intel/ice/ice_fdir_filter.c | 12 ++++++------
 drivers/net/intel/ice/ice_rxtx.c        | 18 +++++++++---------
 6 files changed, 29 insertions(+), 29 deletions(-)
drivers/net/intel/ice/ice_acl_filter.c +5 -5
index 30ff1254c9..a91bd558f3 100644
@@ -86,11 +86,11 @@ ice_acl_prof_alloc(struct ice_hw *hw)
 fail_mem:
 	for (fltr_ptype = ICE_FLTR_PTYPE_NONF_NONE + 1;
 	     fltr_ptype < ptype; fltr_ptype++) {
-		rte_free(hw->acl_prof[fltr_ptype]);
+		ice_free(hw, hw->acl_prof[fltr_ptype]);
 		hw->acl_prof[fltr_ptype] = NULL;
 	}
 
-	rte_free(hw->acl_prof);
+	ice_free(hw, hw->acl_prof);
 	hw->acl_prof = NULL;
 
 	return -ENOMEM;

@@ -148,7 +148,7 @@ static void ice_deinit_acl(struct ice_pf *pf)
 
 	ice_acl_destroy_tbl(hw);
 
-	rte_free(hw->acl_tbl);
+	ice_free(hw, hw->acl_tbl);
 	hw->acl_tbl = NULL;
 
 	if (pf->acl.slots) {

@@ -1102,11 +1102,11 @@ ice_acl_prof_free(struct ice_hw *hw)
 
 	for (ptype = ICE_FLTR_PTYPE_NONF_NONE + 1;
 	     ptype < ICE_FLTR_PTYPE_MAX; ptype++) {
-		rte_free(hw->acl_prof[ptype]);
+		ice_free(hw, hw->acl_prof[ptype]);
 		hw->acl_prof[ptype] = NULL;
 	}
 
-	rte_free(hw->acl_prof);
+	ice_free(hw, hw->acl_prof);
 	hw->acl_prof = NULL;
 }
drivers/net/intel/ice/ice_dcf.c +1 -1
index 3b635c0822..976f2ed260 100644
@@ -913,7 +913,7 @@ ice_dcf_uninit_hw(struct rte_eth_dev *eth_dev, struct ice_dcf_hw *hw)
 	rte_free(hw->qos_bw_cfg);
 	hw->qos_bw_cfg = NULL;
 
-	rte_free(hw->ets_config);
+	ice_free(hw, hw->ets_config);
 	hw->ets_config = NULL;
 }
drivers/net/intel/ice/ice_dcf_parent.c +1 -1
index 2c7d94b50d..e6398205ce 100644
@@ -295,7 +295,7 @@ ice_dcf_query_port_ets(struct ice_hw *parent_hw, struct ice_dcf_hw *real_hw)
 			NULL);
 	if (ret) {
 		PMD_DRV_LOG(ERR, "DCF Query Port ETS failed");
-		rte_free(real_hw->ets_config);
+		ice_free(real_hw, real_hw->ets_config);
 		real_hw->ets_config = NULL;
 		return ret;
 	}
drivers/net/intel/ice/ice_ethdev.c +7 -7
index 9bfa585c06..11b549a452 100644
@@ -1178,7 +1178,7 @@ ice_add_mac_filter(struct ice_vsi *vsi, struct rte_ether_addr *mac_addr)
 	ret = 0;
 
 DONE:
-	rte_free(m_list_itr);
+	ice_free(hw, m_list_itr);
 	return ret;
 }
 

@@ -1229,7 +1229,7 @@ ice_remove_mac_filter(struct ice_vsi *vsi, struct rte_ether_addr *mac_addr)
 
 	ret = 0;
 DONE:
-	rte_free(m_list_itr);
+	ice_free(hw, m_list_itr);
 	return ret;
 }
 

@@ -1314,7 +1314,7 @@ ice_add_vlan_filter(struct ice_vsi *vsi, struct ice_vlan *vlan)
 	ret = 0;
 
 DONE:
-	rte_free(v_list_itr);
+	ice_free(hw, v_list_itr);
 	return ret;
 }
 

@@ -1372,7 +1372,7 @@ ice_remove_vlan_filter(struct ice_vsi *vsi, struct ice_vlan *vlan)
 
 	ret = 0;
 DONE:
-	rte_free(v_list_itr);
+	ice_free(hw, v_list_itr);
 	return ret;
 }
 

@@ -2572,7 +2572,7 @@ ice_get_hw_res(struct ice_hw *hw, uint16_t res_type,
 			(*num_prof), ICE_NONDMA_TO_NONDMA);
 
 exit:
-	rte_free(resp_buf);
+	ice_free(hw, resp_buf);
 	return ret;
 }
 static int

@@ -2772,7 +2772,7 @@ ice_dev_init(struct rte_eth_dev *dev)
 		ret = ice_init_hw_tbls(hw);
 		if (ret) {
 			PMD_INIT_LOG(ERR, "ice_init_hw_tbls failed: %d", ret);
-			rte_free(hw->pkg_copy);
+			ice_free(hw, hw->pkg_copy);
 		}
 	}
 

@@ -3098,7 +3098,7 @@ ice_dev_close(struct rte_eth_dev *dev)
 	ice_release_vsi(pf->main_vsi);
 	ice_sched_cleanup_all(hw);
 	ice_free_hw_tbls(hw);
-	rte_free(hw->port_info);
+	ice_free(hw, hw->port_info);
 	hw->port_info = NULL;
 	free((void *)(uintptr_t)ad->devargs.ddp_filename);
 	ad->devargs.ddp_filename = NULL;
drivers/net/intel/ice/ice_fdir_filter.c +6 -6
index d7ddd32f90..517c32cecf 100644
@@ -231,11 +231,11 @@ ice_fdir_prof_alloc(struct ice_hw *hw)
 	for (fltr_ptype = ICE_FLTR_PTYPE_NONF_NONE + 1;
 	     fltr_ptype < ptype;
 	     fltr_ptype++) {
-		rte_free(hw->fdir_prof[fltr_ptype]);
+		ice_free(hw, hw->fdir_prof[fltr_ptype]);
 		hw->fdir_prof[fltr_ptype] = NULL;
 	}
 
-	rte_free(hw->fdir_prof);
+	ice_free(hw, hw->fdir_prof);
 	hw->fdir_prof = NULL;
 
 	return -ENOMEM;

@@ -608,11 +608,11 @@ ice_fdir_prof_free(struct ice_hw *hw)
 	for (ptype = ICE_FLTR_PTYPE_NONF_NONE + 1;
 	     ptype < ICE_FLTR_PTYPE_MAX;
 	     ptype++) {
-		rte_free(hw->fdir_prof[ptype]);
+		ice_free(hw, hw->fdir_prof[ptype]);
 		hw->fdir_prof[ptype] = NULL;
 	}
 
-	rte_free(hw->fdir_prof);
+	ice_free(hw, hw->fdir_prof);
 	hw->fdir_prof = NULL;
 }
 

@@ -644,7 +644,7 @@ ice_fdir_prof_rm(struct ice_pf *pf, enum ice_fltr_ptype ptype, bool is_tunnel)
 		}
 	}
 	ice_flow_rem_prof(hw, ICE_BLK_FD, prof_id);
-	rte_free(hw_prof->fdir_seg[is_tunnel]);
+	ice_free(hw, hw_prof->fdir_seg[is_tunnel]);
 	hw_prof->fdir_seg[is_tunnel] = NULL;
 
 	for (i = 0; i < hw_prof->cnt; i++)

@@ -1350,7 +1350,7 @@ ice_fdir_input_set_conf(struct ice_pf *pf, enum ice_fltr_ptype flow,
 	if (!ret) {
 		return ret;
 	} else if (ret < 0) {
-		rte_free(seg_tun);
+		ice_free(pf->adapter->hw, seg_tun);
 		return (ret == -EEXIST) ? 0 : ret;
 	} else {
 		return ret;
drivers/net/intel/ice/ice_rxtx.c +9 -9
index 882af80833..c8b88fb235 100644
@@ -854,13 +854,13 @@ ice_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 		}
 		if (cgd_idx >= ICE_MAX_TRAFFIC_CLASS) {
 			PMD_DRV_LOG(ERR, "Bad queue mapping configuration");
-			rte_free(txq_elem);
+			ice_free(hw, txq_elem);
 			return -EINVAL;
 		}
 	} else if (pf->dcb_num_tcs > 1) {
 		/* TM only manages the TC0 scheduler subtree. */
 		PMD_DRV_LOG(ERR, "TM hierarchy is not supported together with multi-TC DCB");
-		rte_free(txq_elem);
+		ice_free(hw, txq_elem);
 		return -EINVAL;
 	}
 

@@ -874,7 +874,7 @@ ice_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 			txq_elem, buf_len, NULL);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to add lan txq");
-		rte_free(txq_elem);
+		ice_free(hw, txq_elem);
 		return -EIO;
 	}
 	/* store the schedule node id */

@@ -884,7 +884,7 @@ ice_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 	if (pf->tm_conf.committed)
 		if (ice_tm_setup_txq_node(pf, hw, tx_queue_id, txq->q_teid) != 0) {
 			PMD_DRV_LOG(ERR, "Failed to set up txq traffic management node");
-			rte_free(txq_elem);
+			ice_free(hw, txq_elem);
 			return -EIO;
 		}
 

@@ -909,10 +909,10 @@ ice_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 		ICE_PCI_REG_WRITE(txq->qtx_tail, 0);
 
 		err = ice_aq_set_txtimeq(hw, txq->reg_idx, 1, ts_elem, ts_buf_len, NULL);
-		rte_free(ts_elem);
+		ice_free(hw, ts_elem);
 		if (err) {
 			PMD_DRV_LOG(ERR, "Failed to set Tx Time queue context, error: %d", err);
-			rte_free(txq_elem);
+			ice_free(hw, txq_elem);
 			return err;
 		}
 	} else {

@@ -924,7 +924,7 @@ ice_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 
 	dev->data->tx_queue_state[tx_queue_id] = RTE_ETH_QUEUE_STATE_STARTED;
 
-	rte_free(txq_elem);
+	ice_free(hw, txq_elem);
 	return 0;
 }
 

@@ -1096,13 +1096,13 @@ ice_fdir_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 			      txq_elem, buf_len, NULL);
 	if (err) {
 		PMD_DRV_LOG(ERR, "Failed to add FDIR txq");
-		rte_free(txq_elem);
+		ice_free(hw, txq_elem);
 		return -EIO;
 	}
 	/* store the schedule node id */
 	txq->q_teid = txq_elem->txqs[0].q_teid;
 
-	rte_free(txq_elem);
+	ice_free(hw, txq_elem);
 	return 0;
 }
Signature · 1 line
2.34.1
Acked-by on v1 1/2
#2 lore raw

Re: [PATCH 1/2] net/ice: free memory with matching allocator

Quoted 10 lines · Anurag Mandal

Several buffers allocated with ice_malloc(), ice_calloc() or ice_memdup() were released using rte_free() instead of ice_free(). Both APIs currently map to the same underlying allocator. However, mixing them is inconsistent and could break if the inconsistent and could break if the OS abstraction layer changes.

Use ice_free() for all memory obtained through the ice_malloc() family.

Signed-off-by Anurag Mandal <[email protected]>
Acked-by Bruce Richardson <[email protected]>
author
#3 lore raw

PATCH 2/2 net/ice: fix unchecked Tx time context allocation

In reply to Anurag Mandal

ice_tx_queue_start() dereferences the Tx time queue context buffer returned by ice_malloc() without NULL check.
On allocation failure, the E830 send-on-timestamp path therefore crashes instead of reporting "No Memory" error, and also the already allocated txq_elem buffer gets leaked.

Added the necessary allocation failure check and freed txq_elem buffer in case of failure to avoid the memory leak.

Fixes 0b6ff09a1f19 ("net/intel: support Tx packet pacing for E830")
Cc [email protected]
Signed-off-by Anurag Mandal <[email protected]>
Acked-by Anatoly Burakov <[email protected]>
---
 drivers/net/intel/ice/ice_rxtx.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/net/intel/ice/ice_rxtx.c +4 -0
index c8b88fb235..9ad4a01f87 100644
@@ -898,6 +898,10 @@ ice_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 		u8 ts_buf_len = ice_struct_size(ts_elem, txtimeqs, 1);
 
 		ts_elem = ice_malloc(hw, ts_buf_len);
+		if (!ts_elem) {
+			ice_free(hw, txq_elem);
+			return -ENOMEM;
+		}
 		ice_setup_txtime_ctx(txq, &txtime_ctx, true);
 		ice_set_ctx(hw, (u8 *)&txtime_ctx,
 				ts_elem->txtimeqs[0].txtime_ctx,
Signature · 1 line
2.34.1
#4 lore raw
In reply to Anurag Mandal
Quoted 20 lines · Anurag Mandal

This patch series addresses two memory handling issues in the ice PMD.

- The first one makes memory allocation and release symmetric. Several buffers obtained through ice_malloc(), ice_calloc() or ice_memdup() were released with rte_free() instead of ice_free(). Both APIs currently map to the same underlying allocator. However, mixing them is inconsistent and would break if the OS abstraction layer ever changes.

- The second one fixes an unchecked allocation in ice_tx_queue_start() which dereferences the Tx time queue context buffer returned by ice_malloc() without NULL check.
On allocation failure, the E830 send-on-timestamp path therefore crashes instead of reporting "No Memory" error, and also the already allocated txq_elem buffer gets leaked.

Anurag Mandal (2):
net/ice: free memory with matching allocator net/ice: fix unchecked Tx time context allocation

Quoted 8 lines
 drivers/net/intel/ice/ice_acl_filter.c  | 10 +++++-----
 drivers/net/intel/ice/ice_dcf.c         |  2 +-
 drivers/net/intel/ice/ice_dcf_parent.c  |  2 +-
 drivers/net/intel/ice/ice_ethdev.c      | 14 +++++++-------
 drivers/net/intel/ice/ice_fdir_filter.c | 12 ++++++------
 drivers/net/intel/ice/ice_rxtx.c        | 22 +++++++++++++---------
 6 files changed, 33 insertions(+), 29 deletions(-)

Patches applied to dpdk-next-net-intel.

Thanks,
/Bruce