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

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

On 5/19/2026 12:46 PM, Simon Horman wrote:
> On Thu, May 14, 2026 at 09:55:21AM -0700, 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.
>>
>> Add a call to xa_destroy the structure during ice_deinit_hw(), and one to
>> the unrolling cleanup path during errors in ice_init_hw(). While here,
>> remove the overly verbose comment explaining the nature of the
>> sched_node_ids xarray.
>>
>> 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>
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 

Sashiko pointed out the following concerns:

> Is it possible for hw->port_info to be freed but not set to NULL before we
> reach this point?
> In ice_init_hw(), if an error occurs after port_info is allocated, the
> err_unroll_alloc path calls devm_kfree() on hw->port_info but leaves the
> pointer dangling:
> ice_common.c:ice_init_hw() {
> ...
> err_unroll_alloc:
>         devm_kfree(ice_hw_to_dev(hw), hw->port_info);
> ...
> }
> If the device falls back to safe mode after this failure and later calls
> ice_deinit_hw(), does dereferencing hw->port_info here result in a
> use-after-free regression?
> Additionally, if the device boots in firmware recovery mode, ice_init_hw()
> might be skipped entirely, leaving hw->port_info as NULL.
> If a devlink reload later triggers ice_deinit_hw(), will taking the address
> of sched_node_ids from a NULL port_info pointer cause a null pointer
> dereference?
> Would it make sense to guard this call by checking if hw->port_info is
> valid?
This is a valid concern because ice_init_hw and ice_deinit_hw() can be
called from devlink reload, which doesn't appear to be blocked during
recovery mode. As a result, we might potentially access a NULL pointer.

Its concern about a dangling port_info is invalid is harder to trace,
because at least ice_probe() exits immediately if it fails ice_init_hw.
However, we also call it from other places that was less obvious.

Regardless, I submitted a v2 which moves the sched_node_ids out of
port_info and into hw, which makes the life cycle management much simpler.

As a result, I did *not* include either your or Aleksandrs reviewd-by
tags, because the patch is substantially different from v1.

Thanks,
Jake