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

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<--

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.

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