Re: [PATCH v3] ice: ice_adapter: release xa entry on adapter allocation failure

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

[-- Attachment #1.1: Type: text/plain, Size: 3163 bytes --]



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 <przemyslaw.kitszel@intel.com>
> Suggested-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
> 

Thanks. I think this flow is a bit easier to understand and everything
works well now.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]