Re: [PATCH net v3] bonding: fix slave_cnt leak on XDP error paths

[Nikolay Aleksandrov]

On 11/09/2026 10:19, Hangbin Liu wrote:
> On Tue, Sep 08, 2026 at 07:09:51PM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential
> > issue(s) to consider:
> >
> > Critical: 1 · High: 0 · Medium: 0 (1) · Low: 0
> >
> > - [Critical] Newly introduced slab out-of-bounds write in
> >    bond_update_slave_arr(): moving `WRITE_ONCE(bond->slave_cnt,…
> >
> > Pre-existing issues:
> > - [Medium] This problem wasn't introduced by this patch, but the XDP
> >    error paths the patch touches are still incompletely unwound: the…
> >
> > --
> >
> > bonding: fix slave_cnt leak on XDP error paths
> >
> > This moves the WRITE_ONCE(bond->slave_cnt, bond->slave_cnt + 1) in
> > bond_enslave() from just above netdev_compute_master_upper_features() down
> > to after the XDP setup block, so that the three XDP failure exits which
> > "goto err_sysfs_del" no longer leave slave_cnt incremented.  The increment
> > is placed just before the bond_update_slave_arr() call.
> >
> > Link to v2: https://lore.kernel.org/r/20260903-bond_slave_cnt-v2-1-02e27304ca36@xxxxxxxxxx
> > Link to v1: https://lore.kernel.org/r/20260902-bond_slave_cnt-v1-1-36e95bf4a6ff@xxxxxxxxxx
> >
> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > > index a9bff7663eeca..1514a8ba0888d 100644
> > > --- a/drivers/net/bonding/bond_main.c
> > > +++ b/drivers/net/bonding/bond_main.c
> > > @@ -2284,7 +2284,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> > >   		}
> > >   	}
> > >   
> > > -	WRITE_ONCE(bond->slave_cnt, bond->slave_cnt + 1);
> >         ^^^^
> >
> > [Severity: Critical]
> > Can removing the increment from here overflow all_slaves->arr[] in
> > bond_update_slave_arr()?
> >
> > By the time control reaches this point, bond_master_upper_dev_link() has
> > already linked the new slave into bond->dev->adj_list.lower, so
> > bond_for_each_slave() now walks N+1 slaves while bond->slave_cnt is still N.
> > bond_update_slave_arr() sizes its arrays from slave_cnt and then fills them
> > with an unchecked store:
> >
> > drivers/net/bonding/bond_main.c:bond_update_slave_arr() {
> > 	...
> > 	usable_slaves = kzalloc_flex(*usable_slaves, arr, bond->slave_cnt);
> > 	all_slaves = kzalloc_flex(*all_slaves, arr, bond->slave_cnt);
> > 	...
> > 		all_slaves->arr[all_slaves->count++] = slave;
> > 	...
> > }
> >
> > The window that now runs with the stale count includes
> > bond_select_active_slave(), which is a few lines below:
> >
> > 	if (bond_uses_primary(bond)) {
> > 		block_netpoll_tx();
> > 		bond_select_active_slave(bond);
> > 		unblock_netpoll_tx();
> > 	}
> >
> > and that reaches bond_update_slave_arr() for balance-tlb/balance-alb:
> >
> > bond_select_active_slave()
> >    bond_find_best_slave()        /* returns a slave in BOND_LINK_BACK */
> >    bond_change_active_slave()    /* BOND_LINK_BACK branch */
> >      bond_alb_handle_link_change()
> >        if (bond_is_nondyn_tlb(bond))
> >                bond_update_slave_arr(bond, NULL);
>
> Oh, hold one. I miss read the comment. It's not about the
> bond_update_slave_arr() at bond_enslave() we moved to.
>
> This is the bond_update_slave_arr() called by bond_uses_primary(), which
> is before the ndo_bpf ops. We need to either also move the
> if (bond_uses_primary(bond)) block after bond_update_slave_arr().
> Or just use my patch v1 version, that add a new err out.
>
> Nikolay, what do you think?
>
> pw-bot: changes-requested
>
> Thanks
> Hangbin

Oh well, it is becoming too dangerous. I'd go with your v1. That is the safest bet
that can be easily backported. Sorry for leading you down this path, I missed
we can still reach the slave arr update via active slave selection.

Cheers,
 Nik