net/ice: fix memory leaks
cover net/ice: fix memory leaks
This series fixes memory leaks, and one double free, in the ice PMD.
a. The DDP package copy was freed twice when hardware table
initialization failed during probe.
b. The switch filter management structures, the DDP package copy and
any remaining VSI contexts were never freed on port close or reset.
c. The DDP filename devarg string leaked on probe failure, and when
the devarg was given more than once.
d. The main VSI leaked when probe failed after PF setup.
e. MAC and VLAN filter entries could stay allocated when a VSI was
released.
f. A failed flow director setup leaked the FDIR VSI, hash table, hash
map and counter pools.
g. The flow segment used to create the ACL profiles leaked on every
ACL engine init.
Anurag Mandal (7):
net/ice: fix DDP package double free on init failure net/ice: fix memory leaks on port close net/ice: fix DDP filename leak
net/ice: fix main VSI leak on init failure net/ice: fix MAC and VLAN filter leak on VSI release net/ice: fix FDIR resource leak on setup failure net/ice: fix ACL profile segment leak
drivers/net/intel/ice/ice_acl_filter.c | 1 + drivers/net/intel/ice/ice_ethdev.c | 33 +++++++++++++++++++++---- drivers/net/intel/ice/ice_fdir_filter.c | 10 ++++++-- 3 files changed, 37 insertions(+), 7 deletions(-)
Signature · 1 line
2.25.1
PATCH 1/7 net/ice: fix DDP package double free on init failure
When ice_init_hw_tbls() fails, ice_dev_init() frees the DDP package copy but leaves hw->pkg_copy and hw->seg pointing into it. Without safe mode, the error path then calls ice_deinit_hw(), which frees the package copy again through ice_free_seg().
Use ice_free_seg(), which also clears these pointers.
--- drivers/net/intel/ice/ice_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/intel/ice/ice_ethdev.c +1 -1
@@ -2785,7 +2785,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); -ice_free (hw, hw->pkg_copy); +ice_free_seg (hw); } }
Signature · 1 line
2.25.1
Re: [PATCH 1/7] net/ice: fix DDP package double free on init failure
Quoted 10 lines · Anurag Mandal
When
ice_init_hw_tbls()fails,ice_dev_init()frees the DDP package copy but leaveshw->pkg_copyandhw->segpointing into it. Without safe mode, the error path then callsice_deinit_hw(), which frees the package copy again throughice_free_seg().Use
ice_free_seg(), which also clears these pointers.Fixes 9c4f15901ada ("net/ice: free memory with matching allocator")Signed-off-by Anurag Mandal <[email protected]>
Quoted 20 lines
--- drivers/net/intel/ice/ice_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/net/intel/ice/ice_ethdev.c +1
-1@@ -2785,7 +2785,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); -ice_free (hw, hw->pkg_copy); +ice_free_seg (hw); } }Signature · 1 line
2.25.1
Re: [PATCH 1/7] net/ice: fix DDP package double free on init failure
Quoted 6 lines · Bruce Richardson
Quoted 5 lines · Anurag Mandal
When
ice_init_hw_tbls()fails,ice_dev_init()frees the DDP package copy but leaveshw->pkg_copyandhw->segpointing into it. Without safe mode, the error path then callsice_deinit_hw(), which frees the package copy again throughice_free_seg().
Quoted 6 lines
Quoted 5 lines
Use
ice_free_seg(), which also clears these pointers.Fixes 9c4f15901ada ("net/ice: free memory with matching allocator")Signed-off-by Anurag Mandal <[email protected]>Acked-by Bruce Richardson <[email protected]>
I think this needs a backport, because it also fixes:
So will add that and CC stable on apply.
Quoted 20 lines
Quoted 20 lines
--- drivers/net/intel/ice/ice_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/net/intel/ice/ice_ethdev.c +1
-1@@ -2785,7 +2785,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); -ice_free (hw, hw->pkg_copy); +ice_free_seg (hw); } }Signature · 1 line
2.25.1
PATCH 2/7 net/ice: fix memory leaks on port close
In reply to Anurag Mandalice_dev_close() tears down the hardware state rather than with ice_deinit_hw(), and never frees the switch filter management structures, the DDP package copy or any remaining VSI contexts. As ice_dev_reset() is a close followed by an init, they also leak on every port reset.
Free them on close.
--- drivers/net/intel/ice/ice_ethdev.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/net/intel/ice/ice_ethdev.c +4 -0
@@ -3109,8 +3109,12 @@ ice_dev_close(struct rte_eth_dev *dev)ice_res_pool_destroy (&pf->msix_pool);ice_release_vsi (pf->main_vsi); +ice_cleanup_fltr_mgmt_struct (hw); + hw->switch_info =NULL ;ice_sched_cleanup_all (hw);ice_free_hw_tbls (hw); +ice_free_seg (hw); +ice_clear_all_vsi_ctx (hw);ice_free (hw, hw->port_info); hw->port_info =NULL ;free ((void *)(uintptr_t )ad->devargs.ddp_filename);
Signature · 1 line
2.25.1
Re: [PATCH 2/7] net/ice: fix memory leaks on port close
Quoted 10 lines · Anurag Mandal
ice_dev_close()tears down the hardware state rather than withice_deinit_hw(), and never frees the switch filter management structures, the DDP package copy or any remaining VSI contexts. Asice_dev_reset()is a close followed by an init, they also leak on every port reset.Free them on close.
Fixes f9cf4f864150 ("net/ice: support device initialization")Fixes 9c4f15901ada ("net/ice: free memory with matching allocator")
Quoted 14 lines
Signed-off-by Anurag Mandal <[email protected]>--- drivers/net/intel/ice/ice_ethdev.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/net/intel/ice/ice_ethdev.c +0
-0@@ -3109,8 +3109,12 @@ ice_dev_close(struct rte_eth_dev *dev)ice_res_pool_destroy (&pf->msix_pool);
drivers/net/intel/ice/ice_ethdev.c ice_dev_close(struct rte_eth_dev *dev) · line 3120
Quoted 10 lines
drivers/net/intel/ice/ice_ethdev.c +4
-0ice_release_vsi (pf->main_vsi); +ice_cleanup_fltr_mgmt_struct (hw); + hw->switch_info =NULL ;ice_sched_cleanup_all (hw);ice_free_hw_tbls (hw); +ice_free_seg (hw); +ice_clear_all_vsi_ctx (hw);ice_free (hw, hw->port_info); hw->port_info =NULL ;free ((void *)(uintptr_t )ad->devargs.ddp_filename);
AI review flagged a potential improvement:
“net/ice: fix memory leaks on port close”: some close-time resources remain unreleased. This is a pre-existing gap, not introduced by these commits:ice_dev_close() calls ice_sched_cleanup_all(), which does not free the scheduler aggregation lists; the canonical ice_deinit_hw() also calls ice_sched_clear_agg(). It likewise frees fd_ctr_base at ice_deinit_hw(), while ice_init_hw() allocates that resource. Thus, close/reset can still leave aggregation-list memory and the FD counter resource behind.
Confidence: 9/10.
Can you check if more work is needed here?
/Bruce
PATCH 3/7 net/ice: fix DDP filename leak
In reply to Anurag MandalThe DDP package filename given with the ddp_pkg_file devarg is duplicated with strdup() during devargs parsing, but only freed on close. It leaks when ice_dev_init() fails at or after devargs parsing, and when the devarg is given more than once.
Free the previous string before storing a new one, and free it on all init error paths.
--- drivers/net/intel/ice/ice_ethdev.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
drivers/net/intel/ice/ice_ethdev.c +10 -3
@@ -805,6 +805,7 @@ handle_ddp_filename_arg(__rte_unused const char *key, const char *value, void *nPMD_DRV_LOG (ERR ,"The DDP package filename is too long : '%s'" , value);return -1 ; } +free ((void *)(uintptr_t )*filename); *filename =strdup (value);return 0 ; }
@@ -2738,7 +2739,8 @@ ice_dev_init(struct rte_eth_dev *dev) ret =ice_parse_devargs (dev);if (ret) {PMD_INIT_LOG (ERR ,"Failed to parse devargs" ); -return -EINVAL ; + ret = -EINVAL ; + goto err_free_devargs; }ice_init_controlq_parameter (hw);
@@ -2746,7 +2748,8 @@ ice_dev_init(struct rte_eth_dev *dev) ret =ice_init_hw (hw);if (ret) {PMD_INIT_LOG (ERR ,"Failed to initialize HW" ); -return -EINVAL ; + ret = -EINVAL ; + goto err_free_devargs; }/*
@@ -2761,7 +2764,8 @@ ice_dev_init(struct rte_eth_dev *dev)PMD_INIT_LOG (ERR ,"Invalid rl_burst_size %u bytes" , ad->devargs.rl_burst_size);ice_deinit_hw (hw); -return -EINVAL ; + ret = -EINVAL ; + goto err_free_devargs; }#ifndef RTE_EXEC_ENV_WINDOWS
@@ -2912,6 +2916,9 @@ ice_dev_init(struct rte_eth_dev *dev) err_init_fw:#endif ice_deinit_hw (hw); +err_free_devargs: +free ((void *)(uintptr_t )ad->devargs.ddp_filename); + ad->devargs.ddp_filename =NULL ;return ret; }
Signature · 1 line
2.25.1
Re: [PATCH 3/7] net/ice: fix DDP filename leak
Quoted 11 lines · Anurag Mandal
The DDP package filename given with the
ddp_pkg_filedevarg is duplicated withstrdup()during devargs parsing, but only freed on close. It leaks whenice_dev_init()fails at or after devargs parsing, and when the devarg is given more than once.Free the previous string before storing a new one, and free it on all init error paths.
Fixes 9dd6dd1db834 ("net/ice: add option to choose DDP package file")Fixes 603beeb970b5 ("net/ice: add safe mode devarg")Fixes f9cf4f864150 ("net/ice: support device initialization")
Quoted 11 lines
Signed-off-by Anurag Mandal <[email protected]>--- drivers/net/intel/ice/ice_ethdev.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)drivers/net/intel/ice/ice_ethdev.c +0
-0
drivers/net/intel/ice/ice_ethdev.c handle_ddp_filename_arg(__rte_unused const char *key, const char *value, void *n · line 808
Quoted 4 lines
drivers/net/intel/ice/ice_ethdev.c +1
-0@@ -805,6 +805,7 @@ handle_ddp_filename_arg(__rte_unused const char *key, const char *value, void *nPMD_DRV_LOG (ERR ,"The DDP package filename is too long : '%s'" , value);return -1 ; } +free ((void *)(uintptr_t )*filename);
You can avoid the casting by changing the type of filename to char * in this function only.
Quoted 34 lines
drivers/net/intel/ice/ice_ethdev.c +6
-3*filename =strdup (value);return 0 ; }@@ -2738,7 +2739,8 @@ ice_dev_init(struct rte_eth_dev *dev) ret =ice_parse_devargs (dev);if (ret) {PMD_INIT_LOG (ERR ,"Failed to parse devargs" ); -return -EINVAL ; + ret = -EINVAL ; + goto err_free_devargs; }ice_init_controlq_parameter (hw);@@ -2746,7 +2748,8 @@ ice_dev_init(struct rte_eth_dev *dev) ret =ice_init_hw (hw);if (ret) {PMD_INIT_LOG (ERR ,"Failed to initialize HW" ); -return -EINVAL ; + ret = -EINVAL ; + goto err_free_devargs; }/* @@ -2761,7 +2764,8 @@ ice_dev_init(struct rte_eth_dev *dev)PMD_INIT_LOG (ERR ,"Invalid rl_burst_size %u bytes" , ad->devargs.rl_burst_size);ice_deinit_hw (hw); -return -EINVAL ; + ret = -EINVAL ; + goto err_free_devargs; }#ifndef RTE_EXEC_ENV_WINDOWS
drivers/net/intel/ice/ice_ethdev.c ice_dev_init(struct rte_eth_dev *dev) · line 2920
Quoted 5 lines
drivers/net/intel/ice/ice_ethdev.c +2
-0@@ -2912,6 +2916,9 @@ ice_dev_init(struct rte_eth_dev *dev) err_init_fw:#endif ice_deinit_hw (hw); +err_free_devargs: +free ((void *)(uintptr_t )ad->devargs.ddp_filename);
I think this would be better using RTE_CAST_PTR, rather than using explicit casts.
Quoted 7 lines
drivers/net/intel/ice/ice_ethdev.c +1
-0+ ad->devargs.ddp_filename =NULL ;return ret; }Signature · 1 line
2.25.1
PATCH 4/7 net/ice: fix main VSI leak on init failure
In reply to Anurag MandalWhen ice_send_driver_ver(), ice_flow_init() or ice_reset_fxp_resource() fails in ice_dev_init(), the error path skips releasing the main VSI created by ice_pf_setup(). This leaks the VSI structure, its MAC filters and the VSI in hardware.
Release the main VSI on these error paths.
--- drivers/net/intel/ice/ice_ethdev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/net/intel/ice/ice_ethdev.c +4 -1
@@ -2833,7 +2833,7 @@ ice_dev_init(struct rte_eth_dev *dev) ret =ice_send_driver_ver (hw);if (ret) {PMD_INIT_LOG (ERR ,"Failed to send driver version" ); - goto err_pf_setup; + goto err_release_vsi; } vsi = pf->main_vsi;
@@ -2905,6 +2905,9 @@ ice_dev_init(struct rte_eth_dev *dev)ice_pf_disable_irq0 (hw);rte_intr_callback_unregister (intr_handle, ice_interrupt_handler, dev); +err_release_vsi: +ice_release_vsi (pf->main_vsi); + pf->main_vsi =NULL ; err_pf_setup:ice_res_pool_destroy (&pf->msix_pool); err_msix_pool_init:
Signature · 1 line
2.25.1
Re: [PATCH 4/7] net/ice: fix main VSI leak on init failure
Quoted 9 lines · Anurag Mandal
When
ice_send_driver_ver(),ice_flow_init()orice_reset_fxp_resource()fails inice_dev_init(), the error path skips releasing the main VSI created byice_pf_setup(). This leaks the VSI structure, its MAC filters and the VSI in hardware.Release the main VSI on these error paths.
Fixes 9481b0902efe ("net/ice: send driver version to firmware")Fixes 96b25c496aad ("net/ice: fix memzone leak after device init failure")
Quoted 5 lines
Signed-off-by Anurag Mandal <[email protected]>--- drivers/net/intel/ice/ice_ethdev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
PATCH 5/7 net/ice: fix MAC and VLAN filter leak on VSI release
In reply to Anurag Mandalice_release_vsi() relies on ice_remove_all_mac_vlan_filters() to free the MAC and VLAN filter entries. That function returns early when the VSI has no MAC filter, or when removing a filter from hardware fails, leaving entries in the lists. The VSI is then freed with them still allocated.
Free any entries left in the lists before freeing the VSI.
--- drivers/net/intel/ice/ice_ethdev.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/net/intel/ice/ice_ethdev.c +9 -0
@@ -2931,6 +2931,9 @@ ice_release_vsi(struct ice_vsi *vsi) {struct ice_hw *hw;struct ice_vsi_ctx vsi_ctx; +struct ice_mac_filter *m_f; +struct ice_vlan_filter *v_f; +void *temp;int ret, error =0 ;if (!vsi)
@@ -2940,6 +2943,12 @@ ice_release_vsi(struct ice_vsi *vsi)ice_remove_all_mac_vlan_filters (vsi); +/* filters whose HW removal failed or was skipped are still listed */ + RTE_TAILQ_FOREACH_SAFE(m_f, &vsi->mac_list, next, temp) +rte_free (m_f); + RTE_TAILQ_FOREACH_SAFE(v_f, &vsi->vlan_list, next, temp) +rte_free (v_f); +memset (&vsi_ctx,0 ,sizeof (vsi_ctx)); vsi_ctx.vsi_num = vsi->vsi_id;
Signature · 1 line
2.25.1
Re: [PATCH 5/7] net/ice: fix MAC and VLAN filter leak on VSI release
Quoted 9 lines · Anurag Mandal
ice_release_vsi()relies onice_remove_all_mac_vlan_filters()to free the MAC and VLAN filter entries. That function returns early when the VSI has no MAC filter, or when removing a filter from hardware fails, leaving entries in the lists. The VSI is then freed with them still allocated.Free any entries left in the lists before freeing the VSI.
Fixes e0dcf94a0d7f ("net/ice: support VLAN ops")
Quoted 5 lines
Signed-off-by Anurag Mandal <[email protected]>--- drivers/net/intel/ice/ice_ethdev.c | 9 +++++++++ 1 file changed, 9 insertions(+)
Quoted 31 lines
drivers/net/intel/ice/ice_ethdev.c +9
-0@@ -2931,6 +2931,9 @@ ice_release_vsi(struct ice_vsi *vsi) {struct ice_hw *hw;struct ice_vsi_ctx vsi_ctx; +struct ice_mac_filter *m_f; +struct ice_vlan_filter *v_f; +void *temp;int ret, error =0 ;if (!vsi)@@ -2940,6 +2943,12 @@ ice_release_vsi(struct ice_vsi *vsi)ice_remove_all_mac_vlan_filters (vsi); +/* filters whose HW removal failed or was skipped are still listed */ + RTE_TAILQ_FOREACH_SAFE(m_f, &vsi->mac_list, next, temp) +rte_free (m_f); + RTE_TAILQ_FOREACH_SAFE(v_f, &vsi->vlan_list, next, temp) +rte_free (v_f); +memset (&vsi_ctx,0 ,sizeof (vsi_ctx)); vsi_ctx.vsi_num = vsi->vsi_id;Signature · 1 line
2.25.1
PATCH 6/7 net/ice: fix FDIR resource leak on setup failure
In reply to Anurag Mandalice_fdir_setup() returns directly when the filter list or counter initialization fails, leaking the FDIR VSI, plus the filter hash table and map in the counter case. Its other error paths release the queues and the VSI, but not the hash table, hash map or counter pools.
As fdir_vsi is reset on failure, ice_fdir_teardown() cannot release them later either. The hash table name also stays registered, so the next FDIR setup for the device fails.
Release everything set up so far on all error paths.
--- drivers/net/intel/ice/ice_fdir_filter.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/net/intel/ice/ice_fdir_filter.c +8 -2
@@ -477,13 +477,15 @@ ice_fdir_setup(struct ice_pf *pf) err =ice_fdir_init_filter_list (pf);if (err) {PMD_DRV_LOG (ERR ,"Failed to init FDIR filter list." ); -return -EINVAL ; + err = -EINVAL ; + goto fail_filter_list; } err =ice_fdir_counter_init (pf);if (err) {PMD_DRV_LOG (ERR ,"Failed to init FDIR counter." ); -return -EINVAL ; + err = -EINVAL ; + goto fail_counter; }/*Fdir tx queue setup*/
@@ -554,6 +556,10 @@ ice_fdir_setup(struct ice_pf *pf)ice_tx_queue_release (pf->fdir.txq); pf->fdir.txq =NULL ; fail_setup_tx: +ice_fdir_counter_release (pf); +fail_counter: +ice_fdir_release_filter_list (pf); +fail_filter_list:ice_release_vsi (vsi); pf->fdir.fdir_vsi =NULL ;return err;
Signature · 1 line
2.25.1
Re: [PATCH 6/7] net/ice: fix FDIR resource leak on setup failure
Quoted 9 lines · Anurag Mandal
ice_fdir_setup()returns directly when the filter list or counter initialization fails, leaking the FDIR VSI, plus the filter hash table and map in the counter case. Its other error paths release the queues and the VSI, but not the hash table, hash map or counter pools.As
fdir_vsiis reset on failure,ice_fdir_teardown()cannot release them later either. The hash table name also stays registered, so the next FDIR setup for the device fails.
Quoted 5 lines
Release everything set up so far on all error paths.
Fixes 1a2fc1799f09 ("net/ice: reject duplicated flow for flow director")Fixes 0f880c3df192 ("net/ice: add flow director counter resource init/release")Fixes 84dc7a95a2d3 ("net/ice: enable flow director engine")
Quoted 6 lines
Signed-off-by Anurag Mandal <[email protected]>--- drivers/net/intel/ice/ice_fdir_filter.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
PATCH 7/7 net/ice: fix ACL profile segment leak
In reply to Anurag Mandalice_acl_prof_init() allocates a flow segment to add the ACL profiles, but only frees it on the error path. ice_flow_add_prof() copies the segments, so the buffer is no longer needed once the profiles are added, and it leaks on every ACL engine init.
Free it on success as well.
--- drivers/net/intel/ice/ice_acl_filter.c | 1 + 1 file changed, 1 insertion(+)
drivers/net/intel/ice/ice_acl_filter.c +1 -0
@@ -308,6 +308,7 @@ ice_acl_prof_init(struct ice_pf *pf) goto err_assoc_prof; } } +ice_free (hw, seg);return 0 ; err_assoc_prof:
Signature · 1 line
2.25.1
Re: [PATCH 7/7] net/ice: fix ACL profile segment leak
Quoted 19 lines · Anurag Mandal
ice_acl_prof_init()allocates a flow segment to add the ACL profiles, but only frees it on the error path.ice_flow_add_prof()copies the segments, so the buffer is no longer needed once the profiles are added, and it leaks on every ACL engine init.Free it on success as well.
Fixes 40d466fa9f76 ("net/ice: support ACL filter in DCF")Signed-off-by Anurag Mandal <[email protected]>--- drivers/net/intel/ice/ice_acl_filter.c | 1 + 1 file changed, 1 insertion(+)drivers/net/intel/ice/ice_acl_filter.c +0
-0
drivers/net/intel/ice/ice_acl_filter.c ice_acl_prof_init(struct ice_pf *pf) · line 313
Quoted 6 lines
drivers/net/intel/ice/ice_acl_filter.c +1
-0@@ -308,6 +308,7 @@ ice_acl_prof_init(struct ice_pf *pf) goto err_assoc_prof; } } +ice_free (hw, seg);return 0 ;
While this change fixes the bug, I see no reason why we can't avoid the bug completely by changing seg to be allocated on the stack rather than dynamically allocated. Define and zero-init as:
struct ice_flow_seg_info seg = {0};And then the memory management issues disappear as its just a local temporary buffer.
What do you think?
Quoted 23 lines · Anurag Mandal
This series fixes memory leaks, and one double free, in the ice PMD.
a. The DDP package copy was freed twice when hardware table
initialization failed during probe.b. The switch filter management structures, the DDP package copy and
any remaining VSI contexts were never freed on port close or reset.c. The DDP filename devarg string leaked on probe failure, and when
the devarg was given more than once.d. The main VSI leaked when probe failed after PF setup.
e. MAC and VLAN filter entries could stay allocated when a VSI was
released.f. A failed flow director setup leaked the FDIR VSI, hash table, hash
map and counter pools.g. The flow segment used to create the ACL profiles leaked on every
ACL engine init.
Quoted 9 lines
Anurag Mandal (7):
net/ice: fix DDP package double free on init failure net/ice: fix memory leaks on port close net/ice: fix DDP filename leak
net/ice: fix main VSI leak on init failure net/ice: fix MAC and VLAN filter leak on VSI release net/ice: fix FDIR resource leak on setup failure net/ice: fix ACL profile segment leak
See comments inline on each patch. Please respin with all patches CC'ed to stable for backport, since all the underlying issues are present in earlier releases.
Thanks,
/Bruce
v2 cover net/ice: fix memory leaks
In reply to Anurag MandalV2-changes:
- Addressed Bruce Richardson's review comments.
- Added "Cc: [email protected]" in all patches.
- Use ice_deinit_hw() on close instead of open-coding it, which also returns the FD counter block to firmware on Patch-2.
- Declare the devarg handler argument as char ** so the filename it points to is char * and can be freed without a cast on Patch-3.
- use RTE_CAST_PTR when freeing the filename in ice_dev_init() on Patch-3.
- allocate the flow segment on the stack instead of freeing it on Patch-7.
This series fixes memory leaks, and one double free, in the ice PMD.
a. The DDP package copy was freed twice when hardware table
initialization failed during probe.
b. The switch filter management structures, the DDP package copy and
any remaining VSI contexts were never freed on port close or reset.
c. The DDP filename devarg string leaked on probe failure, and when
the devarg was given more than once.
d. The main VSI leaked when probe failed after PF setup.
e. MAC and VLAN filter entries could stay allocated when a VSI was
released.
f. A failed flow director setup leaked the FDIR VSI, hash table, hash
map and counter pools.
g. The flow segment used to create the ACL profiles leaked on every
ACL engine init.
Anurag Mandal (7):
net/ice: fix DDP package double free on init failure net/ice: fix resource leaks on port close net/ice: fix DDP filename leak
net/ice: fix main VSI leak on init failure net/ice: fix MAC and VLAN filter leak on VSI release net/ice: fix FDIR resource leak on setup failure net/ice: fix ACL profile segment leak
drivers/net/intel/ice/ice_acl_filter.c | 46 +++++++++++-------------- drivers/net/intel/ice/ice_ethdev.c | 37 ++++++++++++++------ drivers/net/intel/ice/ice_fdir_filter.c | 10 ++++-- 3 files changed, 54 insertions(+), 39 deletions(-)
Signature · 1 line
2.25.1
PATCH v2 1/7 net/ice: fix DDP package double free on init failure
When ice_init_hw_tbls() fails, ice_dev_init() frees the DDP package copy but leaves hw->pkg_copy and hw->seg pointing into it. Without safe mode, the error path then calls ice_deinit_hw(), which frees the package copy again through ice_free_seg().
Use ice_free_seg(), which also clears these pointers.
--- drivers/net/intel/ice/ice_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/intel/ice/ice_ethdev.c +1 -1
@@ -2785,7 +2785,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); -ice_free (hw, hw->pkg_copy); +ice_free_seg (hw); } }
Signature · 1 line
2.25.1
PATCH v2 2/7 net/ice: fix resource leaks on port close
In reply to Anurag Mandalice_dev_close() tears down the hardware state rather than with ice_deinit_hw(), and misses part of it.
The switch filter management structures, the DDP package copy and any remaining VSI contexts are never freed, and the flow director counter block, allocated by ice_init_hw(), is never given back to the firmware.
As ice_dev_reset() is a close followed by an init, the memory also leaks on every port reset.
Call ice_deinit_hw() instead.
--- drivers/net/intel/ice/ice_ethdev.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/net/intel/ice/ice_ethdev.c +1 -5
@@ -3109,13 +3109,9 @@ ice_dev_close(struct rte_eth_dev *dev)ice_res_pool_destroy (&pf->msix_pool);ice_release_vsi (pf->main_vsi); -ice_sched_cleanup_all (hw); -ice_free_hw_tbls (hw); -ice_free (hw, hw->port_info); - hw->port_info =NULL ; +ice_deinit_hw (hw);free ((void *)(uintptr_t )ad->devargs.ddp_filename); ad->devargs.ddp_filename =NULL ; -ice_shutdown_all_ctrlq (hw, true);rte_free (pf->proto_xtr); pf->proto_xtr =NULL ;
Signature · 1 line
2.25.1
Re: [PATCH v2 2/7] net/ice: fix resource leaks on port close
Quoted 11 lines · Anurag Mandal
ice_dev_close()tears down the hardware state rather than withice_deinit_hw(), and misses part of it.
The switch filter management structures, the DDP package copy and any remaining VSI contexts are never freed, and the flow director counter block, allocated byice_init_hw(), is never given back to the firmware.
Asice_dev_reset()is a close followed by an init, the memory also leaks on every port reset.Call
ice_deinit_hw()instead.
Quoted 9 lines
Fixes f9cf4f864150 ("net/ice: support device initialization")Fixes 9c4f15901ada ("net/ice: free memory with matching allocator")Signed-off-by Anurag Mandal <[email protected]>--- drivers/net/intel/ice/ice_ethdev.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
PATCH v2 3/7 net/ice: fix DDP filename leak
In reply to Anurag MandalThe DDP package filename given with the ddp_pkg_file devarg is duplicated with strdup() during devargs parsing, but only freed on close. It leaks when ice_dev_init() fails at or after devargs parsing, and when the devarg is given more than once.
Free the previous string before storing a new one, and free it on all init error paths.
--- drivers/net/intel/ice/ice_ethdev.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/net/intel/ice/ice_ethdev.c +11 -4
@@ -800,11 +800,12 @@ handle_field_name_arg(__rte_unused const char *key, const char *value,static int handle_ddp_filename_arg(__rte_unused const char *key, const char *value, void *name_args) { -const char **filename = name_args; +char **filename = name_args;if (strlen (value) >=ICE_MAX_PKG_FILENAME_SIZE ) {PMD_DRV_LOG (ERR ,"The DDP package filename is too long : '%s'" , value);return -1 ; } +free (*filename); *filename =strdup (value);return 0 ; }
@@ -2738,7 +2739,8 @@ ice_dev_init(struct rte_eth_dev *dev) ret =ice_parse_devargs (dev);if (ret) {PMD_INIT_LOG (ERR ,"Failed to parse devargs" ); -return -EINVAL ; + ret = -EINVAL ; + goto err_free_devargs; }ice_init_controlq_parameter (hw);
@@ -2746,7 +2748,8 @@ ice_dev_init(struct rte_eth_dev *dev) ret =ice_init_hw (hw);if (ret) {PMD_INIT_LOG (ERR ,"Failed to initialize HW" ); -return -EINVAL ; + ret = -EINVAL ; + goto err_free_devargs; }/*
@@ -2761,7 +2764,8 @@ ice_dev_init(struct rte_eth_dev *dev)PMD_INIT_LOG (ERR ,"Invalid rl_burst_size %u bytes" , ad->devargs.rl_burst_size);ice_deinit_hw (hw); -return -EINVAL ; + ret = -EINVAL ; + goto err_free_devargs; }#ifndef RTE_EXEC_ENV_WINDOWS
@@ -2912,6 +2916,9 @@ ice_dev_init(struct rte_eth_dev *dev) err_init_fw: #endifice_deinit_hw (hw); +err_free_devargs: + free(RTE_CAST_PTR(void *, ad->devargs.ddp_filename)); + ad->devargs.ddp_filename =NULL ; return ret; }
Signature · 1 line
2.25.1
Re: [PATCH v2 3/7] net/ice: fix DDP filename leak
Quoted 13 lines · Anurag Mandal
The DDP package filename given with the
ddp_pkg_filedevarg is duplicated withstrdup()during devargs parsing, but only freed on close. It leaks whenice_dev_init()fails at or after devargs parsing, and when the devarg is given more than once.Free the previous string before storing a new one, and free it on all init error paths.
Fixes 9dd6dd1db834 ("net/ice: add option to choose DDP package file")Fixes 603beeb970b5 ("net/ice: add safe mode devarg")Fixes f9cf4f864150 ("net/ice: support device initialization")
Quoted 5 lines
Signed-off-by Anurag Mandal <[email protected]>--- drivers/net/intel/ice/ice_ethdev.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
PATCH v2 4/7 net/ice: fix main VSI leak on init failure
In reply to Anurag MandalWhen ice_send_driver_ver(), ice_flow_init() or ice_reset_fxp_resource() fails in ice_dev_init(), the error path skips releasing the main VSI created by ice_pf_setup(). This leaks the VSI structure, its MAC filters and the VSI in hardware.
Release the main VSI on these error paths.
--- drivers/net/intel/ice/ice_ethdev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/net/intel/ice/ice_ethdev.c +4 -1
@@ -2833,7 +2833,7 @@ ice_dev_init(struct rte_eth_dev *dev) ret =ice_send_driver_ver (hw);if (ret) {PMD_INIT_LOG (ERR ,"Failed to send driver version" ); - goto err_pf_setup; + goto err_release_vsi; } vsi = pf->main_vsi;
@@ -2905,6 +2905,9 @@ ice_dev_init(struct rte_eth_dev *dev)ice_pf_disable_irq0 (hw);rte_intr_callback_unregister (intr_handle, ice_interrupt_handler, dev); +err_release_vsi: +ice_release_vsi (pf->main_vsi); + pf->main_vsi =NULL ; err_pf_setup:ice_res_pool_destroy (&pf->msix_pool); err_msix_pool_init:
Signature · 1 line
2.25.1
PATCH v2 5/7 net/ice: fix MAC and VLAN filter leak on VSI release
In reply to Anurag Mandalice_release_vsi() relies on ice_remove_all_mac_vlan_filters() to free the MAC and VLAN filter entries. That function returns early when the VSI has no MAC filter, or when removing a filter from hardware fails, leaving entries in the lists. The VSI is then freed with them still allocated.
Free any entries left in the lists before freeing the VSI.
--- drivers/net/intel/ice/ice_ethdev.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/net/intel/ice/ice_ethdev.c +9 -0
@@ -2931,6 +2931,9 @@ ice_release_vsi(struct ice_vsi *vsi) {struct ice_hw *hw;struct ice_vsi_ctx vsi_ctx; +struct ice_mac_filter *m_f; +struct ice_vlan_filter *v_f; +void *temp;int ret, error =0 ;if (!vsi)
@@ -2940,6 +2943,12 @@ ice_release_vsi(struct ice_vsi *vsi)ice_remove_all_mac_vlan_filters (vsi); +/* filters whose HW removal failed or was skipped are still listed */ + RTE_TAILQ_FOREACH_SAFE(m_f, &vsi->mac_list, next, temp) +rte_free (m_f); + RTE_TAILQ_FOREACH_SAFE(v_f, &vsi->vlan_list, next, temp) +rte_free (v_f); +memset (&vsi_ctx,0 ,sizeof (vsi_ctx)); vsi_ctx.vsi_num = vsi->vsi_id;
Signature · 1 line
2.25.1
PATCH v2 6/7 net/ice: fix FDIR resource leak on setup failure
In reply to Anurag Mandalice_fdir_setup() returns directly when the filter list or counter initialization fails, leaking the FDIR VSI, plus the filter hash table and map in the counter case. Its other error paths release the queues and the VSI, but not the hash table, hash map or counter pools.
As fdir_vsi is reset on failure, ice_fdir_teardown() cannot release them later either. The hash table name also stays registered, so the next FDIR setup for the device fails.
Release everything set up so far on all error paths.
--- drivers/net/intel/ice/ice_fdir_filter.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/net/intel/ice/ice_fdir_filter.c +8 -2
@@ -477,13 +477,15 @@ ice_fdir_setup(struct ice_pf *pf) err =ice_fdir_init_filter_list (pf);if (err) {PMD_DRV_LOG (ERR ,"Failed to init FDIR filter list." ); -return -EINVAL ; + err = -EINVAL ; + goto fail_filter_list; } err =ice_fdir_counter_init (pf);if (err) {PMD_DRV_LOG (ERR ,"Failed to init FDIR counter." ); -return -EINVAL ; + err = -EINVAL ; + goto fail_counter; }/*Fdir tx queue setup*/
@@ -554,6 +556,10 @@ ice_fdir_setup(struct ice_pf *pf)ice_tx_queue_release (pf->fdir.txq); pf->fdir.txq =NULL ; fail_setup_tx: +ice_fdir_counter_release (pf); +fail_counter: +ice_fdir_release_filter_list (pf); +fail_filter_list:ice_release_vsi (vsi); pf->fdir.fdir_vsi =NULL ;return err;
Signature · 1 line
2.25.1
PATCH v2 7/7 net/ice: fix ACL profile segment leak
In reply to Anurag Mandalice_acl_prof_init() allocates a flow segment to add the ACL profiles, but only frees it on the error path, so it leaks on every ACL engine init.
ice_flow_add_prof() copies the segments, so the buffer is only needed while adding the profiles.
Allocate it on the stack instead, which removes the need to free it.
--- drivers/net/intel/ice/ice_acl_filter.c | 46 +++++++++++--------------- 1 file changed, 20 insertions(+), 26 deletions(-)
drivers/net/intel/ice/ice_acl_filter.c +20 -26
@@ -226,62 +226,57 @@ ice_acl_prof_init(struct ice_pf *pf)struct ice_flow_prof *prof_ipv4_udp =NULL ;struct ice_flow_prof *prof_ipv4_tcp =NULL ;struct ice_flow_prof *prof_ipv4_sctp =NULL ; -struct ice_flow_seg_info *seg; +struct ice_flow_seg_info seg = {0 };int i;int ret; - seg = (struct ice_flow_seg_info *) -ice_malloc (hw,sizeof (*seg)); -if (!seg) -return -ENOMEM ; - -ICE_FLOW_SET_HDRS (seg,ICE_FLOW_SEG_HDR_IPV4 ); -acl_add_prof_prepare (hw, seg, false,0 ,0 ); +ICE_FLOW_SET_HDRS (&seg,ICE_FLOW_SEG_HDR_IPV4 ); +acl_add_prof_prepare (hw, &seg, false,0 ,0 ); ret =ice_flow_add_prof (hw,ICE_BLK_ACL ,ICE_FLOW_RX ,ICE_FLTR_PTYPE_NONF_IPV4_OTHER , - seg,1 ,NULL ,0 , &prof_ipv4); + &seg,1 ,NULL ,0 , &prof_ipv4);if (ret) goto err_add_prof; -ice_memset (seg,0 ,sizeof (*seg),ICE_NONDMA_MEM ); -ICE_FLOW_SET_HDRS (seg,ICE_FLOW_SEG_HDR_IPV_FRAG ); -acl_add_prof_prepare (hw, seg, false,0 ,0 ); +ice_memset (&seg,0 ,sizeof (seg),ICE_NONDMA_MEM ); +ICE_FLOW_SET_HDRS (&seg,ICE_FLOW_SEG_HDR_IPV_FRAG ); +acl_add_prof_prepare (hw, &seg, false,0 ,0 ); ret =ice_flow_add_prof (hw,ICE_BLK_ACL ,ICE_FLOW_RX ,ICE_FLTR_PTYPE_FRAG_IPV4 , - seg,1 ,NULL ,0 , &prof_ipv4_frag); + &seg,1 ,NULL ,0 , &prof_ipv4_frag);if (ret) goto err_add_prof_ipv4_udp_frag; -ice_memset (seg,0 ,sizeof (*seg),ICE_NONDMA_MEM ); -ICE_FLOW_SET_HDRS (seg,ICE_FLOW_SEG_HDR_UDP |ICE_FLOW_SEG_HDR_IPV4 ); -acl_add_prof_prepare (hw, seg, true, +ice_memset (&seg,0 ,sizeof (seg),ICE_NONDMA_MEM ); +ICE_FLOW_SET_HDRS (&seg,ICE_FLOW_SEG_HDR_UDP |ICE_FLOW_SEG_HDR_IPV4 ); +acl_add_prof_prepare (hw, &seg, true,ICE_FLOW_FIELD_IDX_UDP_SRC_PORT ,ICE_FLOW_FIELD_IDX_UDP_DST_PORT ); ret =ice_flow_add_prof (hw,ICE_BLK_ACL ,ICE_FLOW_RX ,ICE_FLTR_PTYPE_NONF_IPV4_UDP , - seg,1 ,NULL ,0 , &prof_ipv4_udp); + &seg,1 ,NULL ,0 , &prof_ipv4_udp);if (ret) goto err_add_prof_ipv4_udp; -ice_memset (seg,0 ,sizeof (*seg),ICE_NONDMA_MEM ); -ICE_FLOW_SET_HDRS (seg,ICE_FLOW_SEG_HDR_TCP |ICE_FLOW_SEG_HDR_IPV4 ); -acl_add_prof_prepare (hw, seg, true, +ice_memset (&seg,0 ,sizeof (seg),ICE_NONDMA_MEM ); +ICE_FLOW_SET_HDRS (&seg,ICE_FLOW_SEG_HDR_TCP |ICE_FLOW_SEG_HDR_IPV4 ); +acl_add_prof_prepare (hw, &seg, true,ICE_FLOW_FIELD_IDX_TCP_SRC_PORT ,ICE_FLOW_FIELD_IDX_TCP_DST_PORT ); ret =ice_flow_add_prof (hw,ICE_BLK_ACL ,ICE_FLOW_RX ,ICE_FLTR_PTYPE_NONF_IPV4_TCP , - seg,1 ,NULL ,0 , &prof_ipv4_tcp); + &seg,1 ,NULL ,0 , &prof_ipv4_tcp);if (ret) goto err_add_prof_ipv4_tcp; -ice_memset (seg,0 ,sizeof (*seg),ICE_NONDMA_MEM ); -ICE_FLOW_SET_HDRS (seg,ICE_FLOW_SEG_HDR_SCTP |ICE_FLOW_SEG_HDR_IPV4 ); -acl_add_prof_prepare (hw, seg, true, +ice_memset (&seg,0 ,sizeof (seg),ICE_NONDMA_MEM ); +ICE_FLOW_SET_HDRS (&seg,ICE_FLOW_SEG_HDR_SCTP |ICE_FLOW_SEG_HDR_IPV4 ); +acl_add_prof_prepare (hw, &seg, true,ICE_FLOW_FIELD_IDX_SCTP_SRC_PORT ,ICE_FLOW_FIELD_IDX_SCTP_DST_PORT ); ret =ice_flow_add_prof (hw,ICE_BLK_ACL ,ICE_FLOW_RX ,ICE_FLTR_PTYPE_NONF_IPV4_SCTP , - seg,1 ,NULL ,0 , &prof_ipv4_sctp); + &seg,1 ,NULL ,0 , &prof_ipv4_sctp);if (ret) goto err_add_prof_ipv4_sctp;
@@ -321,7 +316,6 @@ ice_acl_prof_init(struct ice_pf *pf) err_add_prof_ipv4_udp_frag:ice_flow_rem_prof (hw,ICE_BLK_ACL ,ICE_FLTR_PTYPE_NONF_IPV4_OTHER ); err_add_prof: -ice_free (hw, seg);return ret; }
Signature · 1 line
2.25.1
Re: [PATCH v2 7/7] net/ice: fix ACL profile segment leak
Quoted 9 lines · Anurag Mandal
ice_acl_prof_init()allocates a flow segment to add the ACL profiles, but only frees it on the error path, so it leaks on every ACL engine init.
ice_flow_add_prof()copies the segments, so the buffer is only needed while adding the profiles.Allocate it on the stack instead, which removes the need to free it.
Quoted 8 lines
Fixes 40d466fa9f76 ("net/ice: support ACL filter in DCF")Signed-off-by Anurag Mandal <[email protected]>--- drivers/net/intel/ice/ice_acl_filter.c | 46 +++++++++++--------------- 1 file changed, 20 insertions(+), 26 deletions(-)
Quoted 37 lines · Anurag Mandal
V2-changes:
- Addressed Bruce Richardson's review comments.
- Added "Cc: [email protected]" in all patches.
- Useice_deinit_hw()on close instead of open-coding it, which also returns the FD counter block to firmware on Patch-2.
- Declare the devarg handler argument as char ** so the filename it points to is char * and can be freed without a cast on Patch-3.
- useRTE_CAST_PTRwhen freeing the filename inice_dev_init()on Patch-3.
- allocate the flow segment on the stack instead of freeing it on Patch-7.This series fixes memory leaks, and one double free, in the ice PMD.
a. The DDP package copy was freed twice when hardware table
initialization failed during probe.b. The switch filter management structures, the DDP package copy and
any remaining VSI contexts were never freed on port close or reset.c. The DDP filename devarg string leaked on probe failure, and when
the devarg was given more than once.d. The main VSI leaked when probe failed after PF setup.
e. MAC and VLAN filter entries could stay allocated when a VSI was
released.f. A failed flow director setup leaked the FDIR VSI, hash table, hash
map and counter pools.g. The flow segment used to create the ACL profiles leaked on every
ACL engine init.
Quoted 9 lines
Anurag Mandal (7):
net/ice: fix DDP package double free on init failure net/ice: fix resource leaks on port close net/ice: fix DDP filename leak
net/ice: fix main VSI leak on init failure net/ice: fix MAC and VLAN filter leak on VSI release net/ice: fix FDIR resource leak on setup failure net/ice: fix ACL profile segment leak
Series applied to dpdk-next-net-intel.
Thanks,
/Bruce