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

[Hangbin Liu]

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