[PATCH iwl-net] ice: wait for in-flight Rx polls before freeing a representor

[PATCH iwl-net] ice: wait for in-flight Rx polls before freeing a representor

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

ice_eswitch_detach() unregisters the representor netdev, removes it from
the eswitch xarray and frees it:

	repr->ops.rem(repr);
	xa_erase(&pf->eswitch.reprs, repr->id);
	...
	ice_repr_destroy(repr);

The Rx path holds no reference to what it finds in that xarray. While
the uplink VSI has ICE_RX_FLAGS_MULTIDEV set, ice_process_skb_fields()
calls ice_eswitch_get_target(), which is a bare xa_load(), and then uses
the returned netdev in ice_repr_inc_rx_stats() and eth_type_trans().

unregister_netdev() does wait for RCU readers, but it runs before the
erase, so it does not cover the polls that pick the representor up after
it. Between xa_erase() and ice_repr_destroy() there is no grace period
at all: a poll that loaded the pointer just before the erase then reads
repr->stats through ice_netdev_to_repr() and the netdev itself after
free_percpu(), free_netdev() and kfree() have run.

That order is not new: commit e9942bfe4931 ("ice: Fix use after free
during unload with ports in bridge") moved unregister_netdev() ahead of
the erase so that the bridge can still find the representor by id while
its notifier chain runs, which also keeps the representor reachable for
longer. The missing grace period goes back to the lookup itself, so the
Fixes: tag below points at the commit that added it.

Take the missing grace period. The xarray entry is erased first so that
no further lookup can find the representor, then synchronize_net() waits
for the polls that already did before it is released and destroyed.
repr->ops.rem() has to stay ahead of the erase, because
ice_eswitch_br_port_deinit() looks the representor up by id while
unregister_netdev() runs its notifier chain.

Make the lookup side explicit as well and hold the RCU read lock around
the whole block in ice_process_skb_fields(), so that the netdev returned
by ice_eswitch_get_target() is covered by that synchronize_net() whatever
RCU flavour the kernel is built with.

ice_eswitch_detach() runs once per representor, so this adds one grace
period per representor to the teardown path. All of its callers are in
sleepable process context, so this is safe; batching the grace periods
would mean splitting the erase from the destroy, which is not done here.

The window is reachable in switchdev mode when traffic arrives on the
uplink VSI while a VF or SF representor is being torn down, for example
by removing VFs or by switching the eswitch back to legacy mode. It was
found by code inspection of that teardown path, it was not reproduced at
runtime and no stack trace was observed.

Fixes: 44ba608db509 ("ice: do switchdev slow-path Rx using PF VSI")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 drivers/net/ethernet/intel/ice/ice_eswitch.c  | 10 +++++++++-
 drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 13 +++++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c
index b069e6c514fb..465ed7957506 100644
--- a/drivers/net/ethernet/intel/ice/ice_eswitch.c
+++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c
@@ -556,6 +556,13 @@ static void ice_eswitch_detach(struct ice_pf *pf, struct ice_repr *repr)
 
 	xa_erase(&pf->eswitch.reprs, repr->id);
 
+	/* The Rx path looks the representor up by its id and dereferences the
+	 * result without holding a reference to it. Nothing can find it in the
+	 * xarray anymore, so wait for the polls that picked it up before the
+	 * erase to finish before the representor is torn down.
+	 */
+	synchronize_net();
+
 	if (xa_empty(&pf->eswitch.reprs))
 		ice_eswitch_disable_switchdev(pf);
 
@@ -614,7 +621,8 @@ void ice_eswitch_detach_sf(struct ice_pf *pf, struct ice_dynamic_port *sf)
  * @rx_desc: descriptor used to get src_vsi value
  *
  * Get src_vsi value from descriptor and load correct representor. If it isn't
- * found return rx_ring->netdev.
+ * found return rx_ring->netdev. The caller must hold the RCU read lock, the
+ * returned netdev can go away as soon as the read-side section ends.
  */
 struct net_device *ice_eswitch_get_target(struct ice_rx_ring *rx_ring,
 					  union ice_32b_rx_flex_desc *rx_desc)
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
index e695a664e53d..bbbf13b29cbf 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
@@ -223,8 +223,16 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring,
 
 	/* modifies the skb - consumes the enet header */
 	if (unlikely(rx_ring->flags & ICE_RX_FLAGS_MULTIDEV)) {
-		struct net_device *netdev = ice_eswitch_get_target(rx_ring,
-								   rx_desc);
+		struct net_device *netdev;
+
+		/* ice_eswitch_get_target() looks the representor up in the
+		 * eswitch xarray without taking a reference to it, so hold the
+		 * RCU read lock while the netdev it returns is in use.
+		 * ice_eswitch_detach() waits for the section to end before it
+		 * frees the representor.
+		 */
+		rcu_read_lock();
+		netdev = ice_eswitch_get_target(rx_ring, rx_desc);
 
 		if (ice_is_port_repr_netdev(netdev))
 			ice_repr_inc_rx_stats(netdev, skb->len);
@@ -234,6 +242,7 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring,
 		 */
 		__skb_push(skb, ETH_HLEN);
 		skb->protocol = eth_type_trans(skb, netdev);
+		rcu_read_unlock();
 	}
 
 	ice_rx_csum(rx_ring, skb, rx_desc, ptype);
-- 
2.25.1

Re: [PATCH iwl-net] ice: wait for in-flight Rx polls before freeing a representor

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH iwl-net] ice: wait for in-flight Rx polls before freeing a representor

[Linkui Xiao]

On 2026/10/12 15:34, netdev-bot+sinfo@kernel.org wrote:
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>   - What hardware the change was tested on. For driver fixes please
>     mention the device (and if relevant firmware version) used for
>     testing, or say that the change was not tested on real hardware.
> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

Hi,

Thanks for the reminder. This change was not tested on real hardware. It was
found by code inspection of the representor teardown path; I did not 
reproduce
the UAF at runtime and no stack trace was observed. The fix is based on the
RCU/NAPI lifetime analysis described in the commit message.

If a future revision is needed for other reasons, I will make sure to 
include
this information in the commit message.

Thanks,
Linkui

Re: [PATCH iwl-net] ice: wait for in-flight Rx polls before freeing a representor

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ice: UAF in RX path due to premature rcu_read_unlock and shared NAPI
--

commit 85b479dcdaa34d1d1340c09f178fce9d7d04ceda
Author: Linkui Xiao <xiaolinkui@kylinos.cn>

ice: wait for in-flight Rx polls before freeing a representor

This commit adds an explicit RCU read-side critical section in the Rx path
when looking up a representor netdev. It also adds a synchronize_net() call
in the detach path to wait for the Rx path to complete before tearing down
the representor.

> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> index c32d6b201369b..0e5b98a05981d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> @@ -566,6 +566,13 @@ static void ice_eswitch_detach(struct ice_pf *pf, struct ice_repr *repr)
>  
>  	xa_erase(&pf->eswitch.reprs, repr->id);
>  
> +	/* The Rx path looks the representor up by its id and dereferences the
> +	 * result without holding a reference to it. Nothing can find it in the
> +	 * xarray anymore, so wait for the polls that picked it up before the
> +	 * erase to finish before the representor is torn down.
> +	 */
> +	synchronize_net();
> +

[Severity: High]
Does this synchronize_net() return too early?

Since the explicitly added RCU read lock is dropped in
ice_process_skb_fields(), the packet is passed to napi_gro_receive()
outside of the RCU critical section:

ice_clean_rx_irq()
  ice_process_skb_fields()
    rcu_read_unlock()
  napi_gro_receive()

This means the packet can be queued in the uplink VSI's NAPI GRO hash.

Because the representor shares the uplink VSI's NAPI, unregistering the
representor will not flush this GRO hash.

Could synchronize_net() return while the packet is still held in the uplink
VSI's GRO hash because the RCU lock was already dropped, leading to the
premature freeing of the representor netdev?

>  	if (xa_empty(&pf->eswitch.reprs))
>  		ice_eswitch_disable_switchdev(pf);

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> index 19307086c238f..e0712db9273cd 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> @@ -235,6 +243,7 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring,
>  		 */
>  		__skb_push(skb, ETH_HLEN);
>  		skb->protocol = eth_type_trans(skb, netdev);
> +		rcu_read_unlock();

[Severity: High]
Is it safe to drop the explicitly added RCU read lock here?

It seems this leaves skb->dev unprotected from being freed while the
packet is still being processed and passed up the stack in
ice_clean_rx_irq(). Should the lock be held longer to ensure the packet
safely traverses the stack or gets flushed from the GRO hash?

>  	}
>  
>  	ice_rx_csum(rx_ring, skb, rx_desc, ptype);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012072959.254206-1-xiaolinkui@126.com?part=1