[PATCH] ice: Fix wrong dsn read in ice_adapter_put

[PATCH] ice: Fix wrong dsn read in ice_adapter_put

[Cyrill Gorcunov]

When registering an adapter instance, we read the PCI configuration
space to fetch the DSN and generate an adapter index for lookups.

However, if the adapter has been physically unplugged, the PCI space
is no longer accessible. Reading it returns a zero value, which results
in either an incorrect adapter instance being put or the proper instance
not being put at all. To fix this, we will use the previously known
index instead.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 drivers/net/ethernet/intel/ice/ice_adapter.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-tip.git/drivers/net/ethernet/intel/ice/ice_adapter.c
===================================================================
--- linux-tip.git.orig/drivers/net/ethernet/intel/ice/ice_adapter.c
+++ linux-tip.git/drivers/net/ethernet/intel/ice/ice_adapter.c
@@ -130,13 +130,13 @@ struct ice_adapter *ice_adapter_get(stru
  */
 void ice_adapter_put(struct pci_dev *pdev)
 {
+	const struct ice_pf *pf = pci_get_drvdata(pdev);
+	unsigned long index = pf->adapter->index;
 	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;

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

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

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;

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

[Cyrill Gorcunov]

On Sun, May 17, 2026 at 01:53:07PM +0100, Simon Horman wrote:
...
> >  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?

If we're in recovery mode the ice_remove will not reach ice_adapter_put,
instead it will stop service work and just deinit devlink interface, no?

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

That's the good point but it seems the problem is deeper -- the xor doesn't
prevent from index collision which may lead to wrong assumption with shared
clocks as far as I can tell (and this will cause warn-on-once in ice_adapter_get
with unpredictable state of adapter in further work). So it looks that xoring
index on 32bit archs is just broken.

Still point is correct (i've been working with this adapters on 64bit archs
only so didn't get this issue). Here is an updated version.
---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: [PATCH v2] ice: Fix wrong dsn read in ice_adapter_put

When registering an adapter instance, we read the PCI configuration
space to fetch the DSN and generate an adapter index for lookups.

However, if the adapter has been physically unplugged, the PCI space
is no longer accessible. Reading it returns a zero value, which results
in either an incorrect adapter instance being put or the proper instance
not being put at all. To fix this, we will use the previously known
index instead.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 drivers/net/ethernet/intel/ice/ice_adapter.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Index: linux-tip.git/drivers/net/ethernet/intel/ice/ice_adapter.c
===================================================================
--- linux-tip.git.orig/drivers/net/ethernet/intel/ice/ice_adapter.c
+++ linux-tip.git/drivers/net/ethernet/intel/ice/ice_adapter.c
@@ -40,10 +40,8 @@ static u64 ice_adapter_index(struct pci_
 	}
 }
 
-static unsigned long ice_adapter_xa_index(struct pci_dev *pdev)
+static unsigned long xa_index_mangle(u64 index)
 {
-	u64 index = ice_adapter_index(pdev);
-
 #if BITS_PER_LONG == 64
 	return index;
 #else
@@ -51,6 +49,12 @@ static unsigned long ice_adapter_xa_inde
 #endif
 }
 
+static unsigned long ice_adapter_xa_index(struct pci_dev *pdev)
+{
+	u64 index = ice_adapter_index(pdev);
+	return xa_index_mangle(index);
+}
+
 static struct ice_adapter *ice_adapter_new(struct pci_dev *pdev)
 {
 	struct ice_adapter *adapter;
@@ -130,13 +134,13 @@ struct ice_adapter *ice_adapter_get(stru
  */
 void ice_adapter_put(struct pci_dev *pdev)
 {
+	const struct ice_pf *pf = pci_get_drvdata(pdev);
+	unsigned long index = xa_index_mangle(pf->adapter->index);
 	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;

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

[Cyrill Gorcunov]

On Mon, May 18, 2026 at 01:21:21AM +0300, Cyrill Gorcunov wrote:
> On Sun, May 17, 2026 at 01:53:07PM +0100, Simon Horman wrote:
> ...
> > >  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?
> 
> If we're in recovery mode the ice_remove will not reach ice_adapter_put,
> instead it will stop service work and just deinit devlink interface, no?

Thinking more I think I got what sashiko meant here: the pullout of the
adapter when it been in recovery mode already, and reading register state
is obviously incorrect here too, instead we have to save the state upon
probing in some flag and use it later. I'll update the patch.

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

[Cyrill Gorcunov]

On Mon, May 18, 2026 at 01:02:32PM +0300, Cyrill Gorcunov wrote:
...
> 
> Thinking more I think I got what sashiko meant here: the pullout of the
> adapter when it been in recovery mode already, and reading register state
> is obviously incorrect here too, instead we have to save the state upon
> probing in some flag and use it later. I'll update the patch.

Here is an updated. Actually even in recovery mode the ice_deinit_devlink()
may call for rd32() with undefined result as

 | ice_health_deinit
 |  ice_config_health_events
 |    ice_sq_send_cmd_retry
 |      ice_sq_send_cmd

unless I miss something obvious... (i don't address this in the patch).
Please take a look once time permits.

---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: [PATCH v3] ice: Fix wrong dsn read in ice_adapter_put

When registering an adapter instance, we read the PCI configuration
space to fetch the DSN and generate an adapter index for lookups.

However, if the adapter has been physically unplugged, the PCI space
is no longer accessible. Reading it returns a zero value, which results
in either an incorrect adapter instance being put or the proper instance
not being put at all. To fix this, we will use the previously known
index instead.

Similar applies to recovery mode detection -- if card has been in
recovery mode and physically removed we're not allowed to read its
state from hardware registers but rather provide state flag for
this.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 drivers/net/ethernet/intel/ice/ice.h         |    1 +
 drivers/net/ethernet/intel/ice/ice_adapter.c |   18 +++++++++++++-----
 drivers/net/ethernet/intel/ice/ice_main.c    |    9 ++++++++-
 3 files changed, 22 insertions(+), 6 deletions(-)

Index: linux-tip.git/drivers/net/ethernet/intel/ice/ice.h
===================================================================
--- linux-tip.git.orig/drivers/net/ethernet/intel/ice/ice.h
+++ linux-tip.git/drivers/net/ethernet/intel/ice/ice.h
@@ -310,6 +310,7 @@ enum ice_pf_state {
 	ICE_PHY_INIT_COMPLETE,
 	ICE_FD_VF_FLUSH_CTX,		/* set at FD Rx IRQ or timeout */
 	ICE_AUX_ERR_PENDING,
+	ICE_FW_RECOVERY_MODE,	/* Firmware in recovery mode */
 	ICE_STATE_NBITS		/* must be last */
 };
 
Index: linux-tip.git/drivers/net/ethernet/intel/ice/ice_adapter.c
===================================================================
--- linux-tip.git.orig/drivers/net/ethernet/intel/ice/ice_adapter.c
+++ linux-tip.git/drivers/net/ethernet/intel/ice/ice_adapter.c
@@ -40,10 +40,8 @@ static u64 ice_adapter_index(struct pci_
 	}
 }
 
-static unsigned long ice_adapter_xa_index(struct pci_dev *pdev)
+static unsigned long xa_index_mangle(u64 index)
 {
-	u64 index = ice_adapter_index(pdev);
-
 #if BITS_PER_LONG == 64
 	return index;
 #else
@@ -51,6 +49,12 @@ static unsigned long ice_adapter_xa_inde
 #endif
 }
 
+static unsigned long ice_adapter_xa_index(struct pci_dev *pdev)
+{
+	u64 index = ice_adapter_index(pdev);
+	return xa_index_mangle(index);
+}
+
 static struct ice_adapter *ice_adapter_new(struct pci_dev *pdev)
 {
 	struct ice_adapter *adapter;
@@ -130,13 +134,17 @@ struct ice_adapter *ice_adapter_get(stru
  */
 void ice_adapter_put(struct pci_dev *pdev)
 {
+	const struct ice_pf *pf = pci_get_drvdata(pdev);
 	struct ice_adapter *adapter;
 	unsigned long index;
 
-	index = ice_adapter_xa_index(pdev);
+	if (WARN_ON(!pf->adapter))
+		return;
+
 	scoped_guard(mutex, &ice_adapters_mutex) {
+		index = xa_index_mangle(pf->adapter->index);
 		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;
Index: linux-tip.git/drivers/net/ethernet/intel/ice/ice_main.c
===================================================================
--- linux-tip.git.orig/drivers/net/ethernet/intel/ice/ice_main.c
+++ linux-tip.git/drivers/net/ethernet/intel/ice/ice_main.c
@@ -5121,6 +5121,8 @@ static int ice_probe_recovery_mode(struc
 
 	dev_err(dev, "Firmware recovery mode detected. Limiting functionality. Refer to the Intel(R) Ethernet Adapters and Devices User Guide for details on firmware recovery mode\n");
 
+	set_bit(ICE_FW_RECOVERY_MODE, pf->state);
+
 	INIT_HLIST_HEAD(&pf->aq_wait_list);
 	spin_lock_init(&pf->aq_wait_lock);
 	init_waitqueue_head(&pf->aq_wait_queue);
@@ -5366,7 +5368,12 @@ static void ice_remove(struct pci_dev *p
 		msleep(100);
 	}
 
-	if (ice_is_recovery_mode(&pf->hw)) {
+	/*
+	 * .remove() may be called when adapter is physically
+	 * unplugged so we can't read the fw state but use
+	 * the flag instead.
+	 */
+	if (test_bit(ICE_FW_RECOVERY_MODE, pf->state)) {
 		ice_service_task_stop(pf);
 		scoped_guard(devl, priv_to_devlink(pf)) {
 			ice_deinit_devlink(pf);