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

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

[Hangbin Liu]

From: Hangbin Liu <liuhangbin@kylinos.cn>

When bond_enslave() succeeds up to the XDP setup stage, slave_cnt is
already incremented. If XDP setup subsequently fails, the error paths
jump directly to err_sysfs_del, bypassing the slave_cnt decrement.

This causes slave_cnt to drift upward on each failed enslaving attempt,
which would lead to unbalanced traffic distribution with round-robin
mode.

Fix it by moving the slave_cnt increase after XDP setup.

Fixes: 9e2ee5c7e7c3 ("net, bonding: Add XDP support to the bonding driver")
Signed-off-by: Hangbin Liu <liuhangbin@kylinos.cn>
---
Changes in v3:
- move the slave_cnt increasement before bond_update_slave_arr() (selftest)
- run all bonding selftests on debug kernel to make sure no regression (Jakub)
- Link to v2: https://lore.kernel.org/r/20260903-bond_slave_cnt-v2-1-02e27304ca36@kylinos.cn

Changes in v2:
- move the slave_cnt increasement after XDP setup (Nikolay Aleksandrov)
- balance-xor mode is not affected, not mention it (Nikolay Aleksandrov)
- Link to v1: https://lore.kernel.org/r/20260902-bond_slave_cnt-v1-1-36e95bf4a6ff@kylinos.cn
---
 drivers/net/bonding/bond_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index a9bff7663eec..1514a8ba0888 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);
 	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);
 	}
 
+	/* 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)

---
base-commit: 38b6be101006d3e7af972999f45d4f1e8250587a
change-id: 20260807-bond_slave_cnt-88e78c5f0ab9

Best regards,
-- 
Hangbin Liu <liuhangbin@kylinos.cn>

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

[Nikolay Aleksandrov]

On 07/09/2026 10:07, Hangbin Liu wrote:
> From: Hangbin Liu <liuhangbin@kylinos.cn>
> 
> When bond_enslave() succeeds up to the XDP setup stage, slave_cnt is
> already incremented. If XDP setup subsequently fails, the error paths
> jump directly to err_sysfs_del, bypassing the slave_cnt decrement.
> 
> This causes slave_cnt to drift upward on each failed enslaving attempt,
> which would lead to unbalanced traffic distribution with round-robin
> mode.
> 
> Fix it by moving the slave_cnt increase after XDP setup.
> 
> Fixes: 9e2ee5c7e7c3 ("net, bonding: Add XDP support to the bonding driver")
> Signed-off-by: Hangbin Liu <liuhangbin@kylinos.cn>
> ---
> Changes in v3:
> - move the slave_cnt increasement before bond_update_slave_arr() (selftest)
> - run all bonding selftests on debug kernel to make sure no regression (Jakub)
> - Link to v2: https://lore.kernel.org/r/20260903-bond_slave_cnt-v2-1-02e27304ca36@kylinos.cn
> 
> Changes in v2:
> - move the slave_cnt increasement after XDP setup (Nikolay Aleksandrov)
> - balance-xor mode is not affected, not mention it (Nikolay Aleksandrov)
> - Link to v1: https://lore.kernel.org/r/20260902-bond_slave_cnt-v1-1-36e95bf4a6ff@kylinos.cn
> ---
>   drivers/net/bonding/bond_main.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index a9bff7663eec..1514a8ba0888 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);
>   	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);
>   	}
>   
> +	/* 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)
> 
> ---
> base-commit: 38b6be101006d3e7af972999f45d4f1e8250587a
> change-id: 20260807-bond_slave_cnt-88e78c5f0ab9
> 
> Best regards,

I think the comment is redundant, of course we should change slave_cnt before it
is used by anything that needs it.

Anyway, the patch looks good:
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

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

[Hangbin Liu]

On Mon, Sep 07, 2026 at 10:41:50AM +0300, Nikolay Aleksandrov wrote:
> On 07/09/2026 10:07, Hangbin Liu wrote:
> > From: Hangbin Liu <liuhangbin@kylinos.cn>
> > 
> > When bond_enslave() succeeds up to the XDP setup stage, slave_cnt is
> > already incremented. If XDP setup subsequently fails, the error paths
> > jump directly to err_sysfs_del, bypassing the slave_cnt decrement.
> > 
> > This causes slave_cnt to drift upward on each failed enslaving attempt,
> > which would lead to unbalanced traffic distribution with round-robin
> > mode.
> > 
> > Fix it by moving the slave_cnt increase after XDP setup.
> > 
> > Fixes: 9e2ee5c7e7c3 ("net, bonding: Add XDP support to the bonding driver")
> > Signed-off-by: Hangbin Liu <liuhangbin@kylinos.cn>
> > ---
> > Changes in v3:
> > - move the slave_cnt increasement before bond_update_slave_arr() (selftest)
> > - run all bonding selftests on debug kernel to make sure no regression (Jakub)
> > - Link to v2: https://lore.kernel.org/r/20260903-bond_slave_cnt-v2-1-02e27304ca36@kylinos.cn
> > 
> > Changes in v2:
> > - move the slave_cnt increasement after XDP setup (Nikolay Aleksandrov)
> > - balance-xor mode is not affected, not mention it (Nikolay Aleksandrov)
> > - Link to v1: https://lore.kernel.org/r/20260902-bond_slave_cnt-v1-1-36e95bf4a6ff@kylinos.cn
> > ---
> >   drivers/net/bonding/bond_main.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index a9bff7663eec..1514a8ba0888 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);
> >   	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);
> >   	}
> > +	/* 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)
> > 
> > ---
> > base-commit: 38b6be101006d3e7af972999f45d4f1e8250587a
> > change-id: 20260807-bond_slave_cnt-88e78c5f0ab9
> > 
> > Best regards,
> 
> I think the comment is redundant, of course we should change slave_cnt before it
> is used by anything that needs it.

Yeah, this is a reminder if someone else need to move it to other place.
I can remove it if others also ask.

> 
> Anyway, the patch looks good:
> Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

Thanks again for your review.

Regards
Hangbin

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

[netdev-bot+sashiko]

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@kylinos.cn
Link to v1: https://lore.kernel.org/r/20260902-bond_slave_cnt-v1-1-36e95bf4a6ff@kylinos.cn

> 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);

With one pre-existing slave, slave_cnt == 1 while the lower list holds two
entries, so arr[1] is stored past the end of the allocation, and count is
left at 2 so the RCU TX fast path then dereferences a garbage slave pointer.

The configuration needed looks like supported options only, and no XDP
program is required:

	ip link add bond0 type bond mode balance-tlb miimon 100 \
		updelay 10000 tlb_dynamic_lb 0
	# enslave eth0 (carrier up) so it enters BOND_LINK_BACK
	# wait one miimon tick so slave->delay < params.updelay
	# enslave eth1

A second entry point exists for 8023AD/XOR/TLB/ALB if a slave driver's
ndo_bpf delivers NETDEV_UP/NETDEV_CHANGE/NETDEV_DOWN synchronously during
dev_xdp_propagate(), since bond_slave_netdev_event() also calls
bond_update_slave_arr() for bond_mode_can_use_xmit_hash() modes.

For comparison, the release side keeps the invariant in the safe direction:
__bond_release_one() rebuilds the arrays before decrementing slave_cnt, so
slave_cnt is never smaller than the number of linked slaves there.  Would it
be safer to keep the increment where it was and instead add the matching
decrement to the XDP failure paths (or add a dedicated error label that
undoes it)?

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

>  
> +	/* 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)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907-bond_slave_cnt-v3-1-57df3b3cf2cb%40kylinos.cn

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@kernel.org 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@kylinos.cn
> Link to v1: https://lore.kernel.org/r/20260902-bond_slave_cnt-v1-1-36e95bf4a6ff@kylinos.cn
> 
> > 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