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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d15ad6a2-db2e-481a-ab16-f0358e65aa8d@intel.com>
Date: Wed, 1 Oct 2025 16:10:54 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Haotian Zhang <vulab@...as.ac.cn>, Tony Nguyen
	<anthony.l.nguyen@...el.com>, Przemek Kitszel <przemyslaw.kitszel@...el.com>
CC: Andrew Lunn <andrew+netdev@...n.ch>, "David S . Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	<intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] ice: ice_adapter: release xa entry on adapter
 allocation failure



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@...el.com>
> Suggested-by: Jacob Keller <jacob.e.keller@...el.com>
> Signed-off-by: Haotian Zhang <vulab@...as.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@...el.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;



Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)