Re: [PATCH] ice: Fix wrong dsn read in ice_adapter_put

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
ice: Fix wrong dsn read in ice_adapter_put

When registering an adapter instance, the PCI configuration space is read
to fetch the DSN and generate an adapter index. If the adapter is unplugged,
the PCI space is no longer accessible, resulting in a zero value. This patch
fixes the issue by using the previously known index instead.

> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.c
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c

[ ... ]

> @@ -130,13 +130,13 @@ struct ice_adapter *ice_adapter_get(struct pci_dev *pdev)
>   */
>  void ice_adapter_put(struct pci_dev *pdev)
>  {
> +	const struct ice_pf *pf = pci_get_drvdata(pdev);
> +	unsigned long index = pf->adapter->index;

Is it possible for pf->adapter to be NULL here if the device was probed in
firmware recovery mode?

If the device enters firmware recovery mode during ice_probe(), the driver
calls ice_probe_recovery_mode() and skips the ice_adapter_get() allocation.
If the device is subsequently unplugged, the memory-mapped read for the
firmware state register might return 0.

This would cause ice_is_recovery_mode() to evaluate to false in ice_remove(),
allowing the normal teardown sequence to proceed and call ice_adapter_put().
Would the unconditional dereference of pf->adapter->index then cause a NULL
pointer dereference?

Also, does this implicit cast to unsigned long bypass the XOR folding used
during insertion on 32-bit architectures?

The ice_adapters XArray is keyed by an unsigned long index. During insertion
in ice_adapter_get(), the index is computed using ice_adapter_xa_index(pdev).
On 32-bit architectures, this helper explicitly applies a bitwise XOR fold
to the 64-bit Device Serial Number:

(u32)index ^ (u32)(index >> 32)

Since pf->adapter->index is a 64-bit value, assigning it directly to a
32-bit unsigned long implicitly truncates the upper 32 bits, omitting the
XOR operation.

Because the lookup index would differ from the insertion index, xa_load()
might fail to find the adapter. Would this trigger the WARN_ON and return
early, permanently leaking the adapter memory?

>  	struct ice_adapter *adapter;
> -	unsigned long index;
>  
> -	index = ice_adapter_xa_index(pdev);
>  	scoped_guard(mutex, &ice_adapters_mutex) {
>  		adapter = xa_load(&ice_adapters, index);
> -		if (WARN_ON(!adapter))
> +		if (WARN_ON(!adapter || adapter != pf->adapter))
>  			return;
>  		if (!refcount_dec_and_test(&adapter->refcount))
>  			return;