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

[Weiming Shi <bestswngs@gmail.com>]

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?