Re: [PATCH iwl-net v2] ice: add missing xa_destroy for sched_node_ids

[Jacob Keller <jacob.e.keller@intel.com>]

On 7/6/2026 4:31 PM, Jacob Keller wrote:
> Commit 16dfa49406bc ("ice: Introduce new parameters in ice_sched_node")
> added a sched_node_ids xarray to the port info structure, but never called
> xa_destroy on it.
> 
> Since xarrays can allocate internal memory, this can result in a memory
> leak even if every element in the xarray has been removed.
> 
> The xarray is currently embedded in the port_info structure. This appears
> to have been done because its use is within functions that take the
> port_info as a primary argument.
> 
> However, this complicates managing the lifecycle of the field. The
> port_info structure is allocated in ice_init_hw() using devm, and it is
> not released until the devm cleanup when the driver is unloaded.
> 
> The ice_init_hw() function is called in many places, including devlink
> reload, and possibly during DDP load after updating the Tx scheduler
> layout.
> 
> Adding a call of xa_destroy to the ice_deinit_hw() causes Sashiko to raise
> multiple concerns due to potential ordering issues and possible ways that
> port_info could be a dangling reference.
> 
> To handle this, move the sched_node_ids out of port_info and into the hw
> structure. All users of the array already have a pointer to hw anyways, and
> there is only one sched_node_ids per adapter. While here, remove the overly
> verbose comment explaining the nature of the sched_node_ids xarray.
> 
> Add the missing xa_destroy to the cleanup path and to ice_deinit_hw(),
> ensuring that we properly release the xarray memory.
> 
> This was caught by Sashiko during development of unrelated code.
> 
> Fixes: 16dfa49406bc ("ice: Introduce new parameters in ice_sched_node")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---

--8<--

> index ef1ce106f81b..04633103e3e6 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -1146,6 +1145,8 @@ int ice_init_hw(struct ice_hw *hw)
>  	ice_cleanup_fltr_mgmt_struct(hw);
>  err_unroll_sched:
>  	ice_sched_cleanup_all(hw);
> +err_unroll_xarray:
> +	xa_destroy(&hw->sched_node_ids);
>  err_unroll_alloc:
>  	devm_kfree(ice_hw_to_dev(hw), hw->port_info);

Sashiko says:
> This is a pre-existing issue, but does this error unroll path leave a dangling
> pointer?
> In ice_init_hw(), when hw->port_info is explicitly freed here, the pointer is
> not set to NULL.
> If devlink reload retry is triggered and ice_init_hw() is called again,
> it will see a non-NULL hw->port_info and skip the allocation:
> 	if (!hw->port_info)
> 		hw->port_info = devm_kzalloc(ice_hw_to_dev(hw),
> 					     sizeof(*hw->port_info),
> 					     GFP_KERNEL);
> Additionally, during teardown in ice_deinit_hw(), the dangling hw->port_info
> is unconditionally dereferenced in ice_sched_cleanup_all() ->
> ice_sched_clear_port(hw->port_info), potentially leading to a use-after-free.
> Would it be safer to set hw->port_info to NULL immediately after freeing it?

This issue is clearly pre-existing. I think it only affects devlink
reload, because ice_probe() exits immediately if ice_init_hw() fails. I
will create and submit a separate fix for this, and do not believe it
should hold up the existing fix.

> diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
> index fff0c1afdb41..ffa18d86729a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sched.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sched.c
> @@ -977,7 +977,7 @@ ice_sched_add_elems(struct ice_port_info *pi, struct ice_sched_node *tc_node,
>  		if (!new_node->name)
>  			return -ENOMEM;
>  
> -		status = xa_alloc(&pi->sched_node_ids, &new_node->id, NULL, XA_LIMIT(0, UINT_MAX),
> +		status = xa_alloc(&hw->sched_node_ids, &new_node->id, NULL, XA_LIMIT(0, UINT_MAX),
>  				  GFP_KERNEL);
>  		if (status) {
>  			ice_debug(hw, ICE_DBG_SCHED, "xa_alloc failed for sched node status =%d\n",

Sashiko says:
> 
> This isn't a bug introduced by this patch, but does this allocation failure
> leave a partially initialized node in the software tree?
> In ice_sched_add_elems(), if kzalloc() for new_node->name or xa_alloc()
> fails, the function returns -ENOMEM or breaks the loop without removing
> new_node from the parent's children array. 
> Because new_node->id remains zero-initialized, subsequent teardown via
> ice_free_sched_node() calls xa_erase() on ID 0:
> 	xa_erase(&hw->sched_node_ids, node->id);
> Could this erroneously erase ID 0 from the xarray, corrupting the ID
> allocator and potentially causing ID collisions for active scheduler nodes?


This is also a pre-existing issue with the sched_node_ids, and likely
requires us to do some sort of cleanup. This should also be resolved as
a separate fix and not hold up this fix.

I plan to finish investigating and submit fixes for both of these issues
as a follow-up.

Thanks,
Jake