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 <[email protected]>
>
> Reviewed-by: Simon Horman <[email protected]>
>
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
