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()?

This is a false positive report. The bond->slave_cnt update is not removed.
It's moved just before bond_update_slave_arr().

Thanks
Hangbin

> >  	netdev_compute_master_upper_features(bond->dev, true);
> >  	bond_set_carrier(bond);
> >  
> > @@ -2332,6 +2331,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> >  			bpf_prog_inc(bond->xdp_prog);
> >  	}
>        ^^^^
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but the XDP error exits being
> adjusted here are still only partially unwound.
> 
> The three "goto err_sysfs_del" cases in this block ("Slave does not support
> XDP", "Slave has XDP program loaded" and the dev_xdp_propagate() failure)
> enter a chain of labels:
> 
> 	err_sysfs_del -> err_upper_unlink -> err_unregister -> err_detach ->
> 	err_close -> err_restore_mac -> err_restore_mtu -> err_free ->
> 	err_undo_flags
> 
> None of those labels calls dev_set_promiscuity(slave_dev, -1),
> dev_set_allmulti(slave_dev, -1), bond_hw_addr_flush() or
> bond_slave_ns_maddrs_del().  Those undo steps exist only in
> __bond_release_one():
> 
> drivers/net/bonding/bond_main.c:__bond_release_one() {
> 	...
> 		if (old_flags & IFF_ALLMULTI)
> 			dev_set_allmulti(slave_dev, -1);
> 
> 		if (old_flags & IFF_UP)
> 			bond_hw_addr_flush(bond_dev, slave_dev);
> 	...
> }
> 
> which a failed enslave never reaches.  So when the bond is IFF_PROMISC,
> IFF_ALLMULTI or IFF_UP and has an xdp_prog attached, does a failed enslave
> leave the candidate device with a leaked promiscuity/allmulti refcount, the
> bond's synced mc/uc addresses (plus the lacpdu multicast address in 8023AD
> mode) still installed, and the NS-target multicast addresses added by
> bond_slave_ns_maddrs_add() still present?  Since the device is no longer a
> slave, is there any path left that can drop those refcounts?

Maybe flush multicast addr after dev_close(slave_dev)?
Anyway, this is need another patch.

> 
> >  
> > +	/* Increase the slave count before rebuilding the slave arrays. */
> > +	WRITE_ONCE(bond->slave_cnt, bond->slave_cnt + 1);
> > +
> >  	/* broadcast mode uses the all_slaves to loop through slaves. */
> >  	if (bond_mode_can_use_xmit_hash(bond) ||
> >  	    BOND_MODE(bond) == BOND_MODE_BROADCAST)

Thanks
Hangbin