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

[PATCH net v4] 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.

Add an error out to make sure slave_cnt gets decreased correctly.

Fixes: 9e2ee5c7e7c3 ("net, bonding: Add XDP support to the bonding driver")
Signed-off-by: Hangbin Liu <liuhangbin@kylinos.cn>
---
Changes in v4:
- Revert to v1 version as the bond_select_active_slave() just under
  slave_cnt increase also calls bond_update_slave_arr() (sashiko)
- re-run all bonding selftests with debug kernel, all passed.
- Link to v3: https://lore.kernel.org/r/20260907-bond_slave_cnt-v3-1-57df3b3cf2cb@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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index a9bff7663eec..577d85c83804 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2305,7 +2305,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 			SLAVE_NL_ERR(bond_dev, slave_dev, extack,
 				     "Slave does not support XDP");
 			res = -EOPNOTSUPP;
-			goto err_sysfs_del;
+			goto err_slave_cnt;
 		}
 	} else if (bond->xdp_prog) {
 		struct netdev_bpf xdp = {
@@ -2319,14 +2319,14 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 			SLAVE_NL_ERR(bond_dev, slave_dev, extack,
 				     "Slave has XDP program loaded, please unload before enslaving");
 			res = -EOPNOTSUPP;
-			goto err_sysfs_del;
+			goto err_slave_cnt;
 		}
 
 		res = dev_xdp_propagate(slave_dev, &xdp);
 		if (res < 0) {
 			/* ndo_bpf() sets extack error message */
 			slave_dbg(bond_dev, slave_dev, "Error %d calling ndo_bpf\n", res);
-			goto err_sysfs_del;
+			goto err_slave_cnt;
 		}
 		if (bond->xdp_prog)
 			bpf_prog_inc(bond->xdp_prog);
@@ -2348,6 +2348,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	return 0;
 
 /* Undo stages on error */
+err_slave_cnt:
+	WRITE_ONCE(bond->slave_cnt, bond->slave_cnt - 1);
+
 err_sysfs_del:
 	bond_sysfs_slave_del(new_slave);
 

---
base-commit: e6b6078ea1731b05b3b552497b3bce4bf8b014ae
change-id: 20260807-bond_slave_cnt-88e78c5f0ab9

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

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

[Nikolay Aleksandrov]

On 14/09/2026 06:09, 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.
> 
> Add an error out to make sure slave_cnt gets decreased correctly.
> 
> Fixes: 9e2ee5c7e7c3 ("net, bonding: Add XDP support to the bonding driver")
> Signed-off-by: Hangbin Liu <liuhangbin@kylinos.cn>
> ---
> Changes in v4:
> - Revert to v1 version as the bond_select_active_slave() just under
>    slave_cnt increase also calls bond_update_slave_arr() (sashiko)
> - re-run all bonding selftests with debug kernel, all passed.
> - Link to v3: https://lore.kernel.org/r/20260907-bond_slave_cnt-v3-1-57df3b3cf2cb@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 | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 

Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

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

[Jakub Kicinski]

Clashiko classified the broader issues as independent but I'm not sure.
If there are more problems with pre-publishing the count we need
a different fix? Or am I not reading closely enough?

AI review:
---
bonding: fix slave_cnt leak on XDP error paths

The three XDP failure exits in bond_enslave() used to jump straight to
err_sysfs_del, which skipped the slave_cnt decrement and let the counter
drift upward on every failed enslave. A new err_slave_cnt label is added
ahead of err_sysfs_del to undo the increment.

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index a9bff7663eeca..577d85c838040 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2305,7 +2305,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  			SLAVE_NL_ERR(bond_dev, slave_dev, extack,
>  				     "Slave does not support XDP");
>  			res = -EOPNOTSUPP;
> -			goto err_sysfs_del;
> +			goto err_slave_cnt;
>  		}
>  	} else if (bond->xdp_prog) {
>  		struct netdev_bpf xdp = {

[Severity: High]

This is a pre-existing ordering issue that dates back to 9e2ee5c7e7c3 and
is not introduced by this patch, but since the new err_slave_cnt stage sits
right here it seems worth asking about.

By the time bond_enslave() reaches this ndo_bpf / ndo_xdp_xmit test, the
slave has already been published to the lockless transmit path:

	res = bond_master_upper_dev_link(bond, new_slave, extack);
	...
	WRITE_ONCE(bond->slave_cnt, bond->slave_cnt + 1);
	...
	if (bond_uses_primary(bond)) {
		block_netpoll_tx();
		bond_select_active_slave(bond);
		unblock_netpoll_tx();
	}

	if (!slave_dev->netdev_ops->ndo_bpf ||
	    !slave_dev->netdev_ops->ndo_xdp_xmit) {

For the trunk modes the switch default arm already ran
bond_set_active_slave(new_slave) with new_slave->link == BOND_LINK_UP, so
bond_slave_can_tx() is true for it.

Can a concurrent bond_xdp_xmit(), which only takes rcu_read_lock() and does
not hold RTNL, pick that not-yet-validated slave in this window?

bond_xdp_xmit()
  bond_xdp_xmit_roundrobin_slave_get()
    slave_cnt = READ_ONCE(bond->slave_cnt);
    if (likely(slave_cnt)) {
        slave_id = bond_rr_gen_slave_id(bond) % slave_cnt;
        return bond_get_slave_by_id(bond, slave_id);

bond_get_slave_by_id() only checks bond_slave_can_tx(), never XDP
capability, and bond_xdp_xmit() then does:

	err = slave_dev->netdev_ops->ndo_xdp_xmit(slave_dev, 1, frames1, flags);

with no NULL test, so would this be an indirect call through a NULL
ndo_xdp_xmit for a slave that lacks it?

The devmap gate does not appear to help here either: __xdp_enqueue() and
is_valid_dst() only look at the bond device's xdp_features, which still
advertise NETDEV_XDP_ACT_NDO_XMIT during this window because
bond_xdp_set_features() only runs after the XDP block. The same exposure
looks reachable through bond_xmit_activebackup_slave_get() for
bond_uses_primary() modes, where bond_select_active_slave() may publish the
new slave as curr_active_slave before the capability check.

Would it make sense to move the ndo_bpf / ndo_xdp_xmit validation earlier,
before the slave is linked and counted?

[ ... ]

> @@ -2348,6 +2348,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  	return 0;
>  
>  /* Undo stages on error */
> +err_slave_cnt:
> +	WRITE_ONCE(bond->slave_cnt, bond->slave_cnt - 1);
> +
>  err_sysfs_del:
>  	bond_sysfs_slave_del(new_slave);
>  

[Severity: Medium]

These leaks are pre-existing, the three XDP exits already fell into this
same unwind chain before this patch, so they are not introduced here. Since
this patch adds the stage where the symmetric undo would belong, is it worth
handling the rest of the state applied just above the XDP block?

Reaching the XDP checks means this group has already run, with no
intervening label:

	WRITE_ONCE(bond->slave_cnt, bond->slave_cnt + 1);
	netdev_compute_master_upper_features(bond->dev, true);
	bond_set_carrier(bond);

	bond_slave_ns_maddrs_add(bond, new_slave);

err_slave_cnt reverses only the counter and falls through to err_sysfs_del,
so does the bond keep a feature mask recomputed from a device that never
became a slave, and for a first slave with an up link keep reporting
LOWER_UP with zero slaves, since nothing on the unwind path re-runs
netdev_compute_master_upper_features() or bond_set_carrier()?

Does bond_slave_ns_maddrs_add() also leak solicited-node multicast
addresses on the rejected netdev? bond_options.c takes real references:

	void bond_slave_ns_maddrs_add(struct bonding *bond, struct slave *slave)
	{
		if (!bond->params.arp_validate)
			return;
		slave_set_ns_maddrs(bond, slave, true);
	}

and no label between err_slave_cnt and err_undo_flags calls
bond_slave_ns_maddrs_del(). err_detach only re-runs
bond_change_active_slave() / bond_select_active_slave() when the new slave
had become curr_active_slave, so with mode=active-backup plus arp_validate
plus ns_ip6_target and the new slave staying backup, would these accumulate
on each retried enslave?

For round-robin and 802.3ad, which are both XDP-capable and not
bond_uses_primary(), the block above also ran:

	if (bond_dev->flags & IFF_PROMISC) {
		res = dev_set_promiscuity(slave_dev, 1);
	...
	if (bond_dev->flags & IFF_ALLMULTI) {
		res = dev_set_allmulti(slave_dev, 1);
	...
		dev_mc_sync_multiple(slave_dev, bond_dev);
		dev_uc_sync_multiple(slave_dev, bond_dev);
		if (BOND_MODE(bond) == BOND_MODE_8023AD)
			dev_mc_add(slave_dev, lacpdu_mcast_addr);

Does the released device stay in promiscuous and allmulti mode with the
multicast and unicast sync entries still programmed after the XDP failure?

__bond_release_one() performs exactly the mirror operations, which suggests
these are the required undo steps:

	bond_slave_ns_maddrs_del(bond, slave);
	...
	WRITE_ONCE(bond->slave_cnt, bond->slave_cnt - 1);
	...
	netdev_compute_master_upper_features(bond->dev, true);
	...
		if (old_flags & IFF_PROMISC)
			dev_set_promiscuity(slave_dev, -1);
	...
		if (old_flags & IFF_ALLMULTI)
			dev_set_allmulti(slave_dev, -1);
	...
		if (old_flags & IFF_UP)
			bond_hw_addr_flush(bond_dev, slave_dev);

and that path is not reachable here because the enslave failed.
-- 
pw-bot: cr

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

[Hangbin Liu]

Hi Jakub, 

Thanks for the notify.

Jay, Nikolay, please correct me if you disagree with my comments.

On Tue, Sep 15, 2026 at 06:27:22PM -0700, Jakub Kicinski wrote:
> Clashiko classified the broader issues as independent but I'm not sure.
> If there are more problems with pre-publishing the count we need
> a different fix? Or am I not reading closely enough?
> 
> AI review:
> ---
> bonding: fix slave_cnt leak on XDP error paths
> 
> The three XDP failure exits in bond_enslave() used to jump straight to
> err_sysfs_del, which skipped the slave_cnt decrement and let the counter
> drift upward on every failed enslave. A new err_slave_cnt label is added
> ahead of err_sysfs_del to undo the increment.
> 
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index a9bff7663eeca..577d85c838040 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -2305,7 +2305,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> >  			SLAVE_NL_ERR(bond_dev, slave_dev, extack,
> >  				     "Slave does not support XDP");
> >  			res = -EOPNOTSUPP;
> > -			goto err_sysfs_del;
> > +			goto err_slave_cnt;
> >  		}
> >  	} else if (bond->xdp_prog) {
> >  		struct netdev_bpf xdp = {
> 
> [Severity: High]
> 
> This is a pre-existing ordering issue that dates back to 9e2ee5c7e7c3 and
> is not introduced by this patch, but since the new err_slave_cnt stage sits
> right here it seems worth asking about.
> 
> By the time bond_enslave() reaches this ndo_bpf / ndo_xdp_xmit test, the
> slave has already been published to the lockless transmit path:
> 
> 	res = bond_master_upper_dev_link(bond, new_slave, extack);
> 	...
> 	WRITE_ONCE(bond->slave_cnt, bond->slave_cnt + 1);
> 	...
> 	if (bond_uses_primary(bond)) {
> 		block_netpoll_tx();
> 		bond_select_active_slave(bond);
> 		unblock_netpoll_tx();
> 	}
> 
> 	if (!slave_dev->netdev_ops->ndo_bpf ||
> 	    !slave_dev->netdev_ops->ndo_xdp_xmit) {
> 
> For the trunk modes the switch default arm already ran
> bond_set_active_slave(new_slave) with new_slave->link == BOND_LINK_UP, so
> bond_slave_can_tx() is true for it.

Not since the bond_uses_primary() if checking, the new_slave could be set to
active even more early during enslave. e.g. in bond_main.c:2174

  switch (BOND_MODE(bond)) {
  ...
  case BOND_MODE_TLB:
  case BOND_MODE_ALB:
      bond_set_active_slave(new_slave);
  default:
      bond_set_active_slave(new_slave);

I think the bond assume the new slave is in trunk modes in this situation,
which could pass any traffic.

> 
> Can a concurrent bond_xdp_xmit(), which only takes rcu_read_lock() and does
> not hold RTNL, pick that not-yet-validated slave in this window?
> 
> bond_xdp_xmit()
>   bond_xdp_xmit_roundrobin_slave_get()
>     slave_cnt = READ_ONCE(bond->slave_cnt);
>     if (likely(slave_cnt)) {
>         slave_id = bond_rr_gen_slave_id(bond) % slave_cnt;
>         return bond_get_slave_by_id(bond, slave_id);

bond_get_slave_by_id() loop each slaves, even the slave_cnt not updated, the
new slave still could be chosen.

> 
> bond_get_slave_by_id() only checks bond_slave_can_tx(), never XDP
> capability, and bond_xdp_xmit() then does:
> 
> 	err = slave_dev->netdev_ops->ndo_xdp_xmit(slave_dev, 1, frames1, flags);
> 
> with no NULL test, so would this be an indirect call through a NULL
> ndo_xdp_xmit for a slave that lacks it?
> 
> The devmap gate does not appear to help here either: __xdp_enqueue() and
> is_valid_dst() only look at the bond device's xdp_features, which still
> advertise NETDEV_XDP_ACT_NDO_XMIT during this window because
> bond_xdp_set_features() only runs after the XDP block. The same exposure
> looks reachable through bond_xmit_activebackup_slave_get() for
> bond_uses_primary() modes, where bond_select_active_slave() may publish the
> new slave as curr_active_slave before the capability check.
> 
> Would it make sense to move the ndo_bpf / ndo_xdp_xmit validation earlier,
> before the slave is linked and counted?

I saw Nikolay has done a similar fix e9acda52fd2e ("bonding: fix use-after-free
due to enslave fail after slave array update"). But the risk is till there.
So how up we should move the xdp ops?

Maybe add a checking as a workaround before we do a safe re-order for
bond_enslave?

  if (likely(slave_dev->netdev_ops->ndo_xdp_xmit))
      err = slave_dev->netdev_ops->ndo_xdp_xmit(slave_dev, 1, frames1, flags);

Nikolay, what do you think?

> 
> [ ... ]
> 
> > @@ -2348,6 +2348,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> >  	return 0;
> >  
> >  /* Undo stages on error */
> > +err_slave_cnt:
> > +	WRITE_ONCE(bond->slave_cnt, bond->slave_cnt - 1);
> > +
> >  err_sysfs_del:
> >  	bond_sysfs_slave_del(new_slave);
> >  
> 
> [Severity: Medium]
> 
> These leaks are pre-existing, the three XDP exits already fell into this
> same unwind chain before this patch, so they are not introduced here. Since
> this patch adds the stage where the symmetric undo would belong, is it worth
> handling the rest of the state applied just above the XDP block?
> 
> Reaching the XDP checks means this group has already run, with no
> intervening label:
> 
> 	WRITE_ONCE(bond->slave_cnt, bond->slave_cnt + 1);
> 	netdev_compute_master_upper_features(bond->dev, true);
> 	bond_set_carrier(bond);
> 
> 	bond_slave_ns_maddrs_add(bond, new_slave);
> 
> err_slave_cnt reverses only the counter and falls through to err_sysfs_del,
> so does the bond keep a feature mask recomputed from a device that never
> became a slave, and for a first slave with an up link keep reporting
> LOWER_UP with zero slaves, since nothing on the unwind path re-runs
> netdev_compute_master_upper_features() or bond_set_carrier()?

Yes, I think we could do it, in a separate patch.

> 
> Does bond_slave_ns_maddrs_add() also leak solicited-node multicast
> addresses on the rejected netdev? bond_options.c takes real references:
> 
> 	void bond_slave_ns_maddrs_add(struct bonding *bond, struct slave *slave)
> 	{
> 		if (!bond->params.arp_validate)
> 			return;
> 		slave_set_ns_maddrs(bond, slave, true);
> 	}
> 
> and no label between err_slave_cnt and err_undo_flags calls
> bond_slave_ns_maddrs_del(). err_detach only re-runs
> bond_change_active_slave() / bond_select_active_slave() when the new slave
> had become curr_active_slave, so with mode=active-backup plus arp_validate
> plus ns_ip6_target and the new slave staying backup, would these accumulate
> on each retried enslave?

Yes, we could.

> 
> For round-robin and 802.3ad, which are both XDP-capable and not
> bond_uses_primary(), the block above also ran:
> 
> 	if (bond_dev->flags & IFF_PROMISC) {
> 		res = dev_set_promiscuity(slave_dev, 1);
> 	...
> 	if (bond_dev->flags & IFF_ALLMULTI) {
> 		res = dev_set_allmulti(slave_dev, 1);
> 	...
> 		dev_mc_sync_multiple(slave_dev, bond_dev);
> 		dev_uc_sync_multiple(slave_dev, bond_dev);
> 		if (BOND_MODE(bond) == BOND_MODE_8023AD)
> 			dev_mc_add(slave_dev, lacpdu_mcast_addr);
> 
> Does the released device stay in promiscuous and allmulti mode with the
> multicast and unicast sync entries still programmed after the XDP failure?

Yes

> 
> __bond_release_one() performs exactly the mirror operations, which suggests
> these are the required undo steps:
> 
> 	bond_slave_ns_maddrs_del(bond, slave);
> 	...
> 	WRITE_ONCE(bond->slave_cnt, bond->slave_cnt - 1);
> 	...
> 	netdev_compute_master_upper_features(bond->dev, true);
> 	...
> 		if (old_flags & IFF_PROMISC)
> 			dev_set_promiscuity(slave_dev, -1);
> 	...
> 		if (old_flags & IFF_ALLMULTI)
> 			dev_set_allmulti(slave_dev, -1);
> 	...
> 		if (old_flags & IFF_UP)
> 			bond_hw_addr_flush(bond_dev, slave_dev);
> 
> and that path is not reachable here because the enslave failed.

Yes, I think we can do it in another patch.

Thanks
Hangbin