On 10/1/2025 4:53 AM, Haotian Zhang wrote:
> When ice_adapter_new() fails, the reserved XArray entry created by
> xa_insert() is not released. This causes subsequent insertions at
> the same index to return -EBUSY, potentially leading to
> NULL pointer dereferences.
>
> Reorder the operations as suggested by Przemek Kitszel:
> 1. Check if adapter already exists (xa_load)
> 2. Reserve the XArray slot (xa_reserve)
> 3. Allocate the adapter (ice_adapter_new)
> 4. Store the adapter (xa_store)
>
> Fixes: 0f0023c649c7 ("ice: do not init struct ice_adapter more times than
> needed")
> Suggested-by: Przemek Kitszel <[email protected]>
> Suggested-by: Jacob Keller <[email protected]>
> Signed-off-by: Haotian Zhang <[email protected]>
> Thanks. I think this flow is a bit easier to understand and everything works well now. Reviewed-by: Jacob Keller <[email protected]> > --- > Changes in v3: > - Reorder xa_load/xa_reserve/ice_adapter_new/xa_store calls as > suggested by Przemek Kitszel, instead of just adding xa_release(). > Changes in v2: > - Instead of checking the return value of xa_store(), fix the real bug > where a failed ice_adapter_new() would leave a stale entry in the > XArray. > - Use xa_release() to clean up the reserved entry, as suggested by > Jacob Keller. > --- > drivers/net/ethernet/intel/ice/ice_adapter.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c > b/drivers/net/ethernet/intel/ice/ice_adapter.c > index b53561c34708..0a8a48cd4bce 100644 > --- a/drivers/net/ethernet/intel/ice/ice_adapter.c > +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c > @@ -99,19 +99,21 @@ struct ice_adapter *ice_adapter_get(struct pci_dev *pdev) > > index = ice_adapter_xa_index(pdev); > scoped_guard(mutex, &ice_adapters_mutex) { > - err = xa_insert(&ice_adapters, index, NULL, GFP_KERNEL); > - if (err == -EBUSY) { > - adapter = xa_load(&ice_adapters, index); > + adapter = xa_load(&ice_adapters, index); > + if (adapter) { > refcount_inc(&adapter->refcount); > WARN_ON_ONCE(adapter->index != ice_adapter_index(pdev)); > return adapter; > } > + err = xa_reserve(&ice_adapters, index, GFP_KERNEL); > if (err) > return ERR_PTR(err); > > adapter = ice_adapter_new(pdev); > - if (!adapter) > + if (!adapter) { > + xa_release(&ice_adapters, index); Strictly we might not actually need xa_release now, because xa_load will return NULL on a reserved entry, then xa_reserve will be a no-op if the entry is already reserved, I believe, but I think its best to keep it for clarity and because it frees up otherwise unused memory which seems important since ice_adapter_new should only really fail if we're out of memory. Additionally, this is an error path and not something that happens every run so it is unlikely to be part of a performance critical bottleneck. Thanks! > return ERR_PTR(-ENOMEM); > + } > xa_store(&ice_adapters, index, adapter, GFP_KERNEL); > } > return adapter;
OpenPGP_signature.asc
Description: OpenPGP digital signature
