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 <[email protected]>
> ---
--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
