[PATCH net v4] bonding: 3ad: update transmit arrays without waiting for RTNL

[PATCH net v4] bonding: 3ad: update transmit arrays without waiting for RTNL

[Yun Lu]

From: Yun Lu <luyun@kylinos.cn>

During 802.3ad bring-up, packets can be dropped despite carrier being up
and LACP having enabled ports for distribution. This was observed on a
machine in real use.

The LACP worker enables ports before queuing slave_arr_work to rebuild
usable_slaves. The transmit path keeps using that array, which can remain
empty while the array worker retries rtnl_trylock(). Unrelated RTNL
contention can therefore prolong the packet loss.

Use a per-bond mutex to serialize array writers with lower-list changes.
Rebuild the arrays directly from the LACP worker after releasing
mode_lock and RCU, and remove the retry worker's RTNL dependency. Retain
delayed work to retry allocation failures.

New slaves become visible in the lower list before all fallible setup
has finished. Exclude them from both arrays until that setup, including
XDP propagation, succeeds. Retry failed final updates for running bonds.
Also rebuild broadcast arrays on open and retry failed open updates.
Protect the peer-notification array reader with RCU now that RTNL no
longer excludes writers. Updating the shared arrays also covers XDP and
lower-device queries without changing their selection rules.

An RTNL-holding test module and userspace LACP peers reproduced the
blocked update and verified array publication and packet delivery before
RTNL release with this fix. All 16 bonding selftests passed.

Fixes: ee6377147409 ("bonding: Simplify the xmit function for modes that use xmit_hash")
Signed-off-by: Yun Lu <luyun@kylinos.cn>
---

Changes in v4:
- Replace the TX fallback with synchronous control-path array updates
  using a per-bond mutex, following Paolo's and Jay's suggestions.
- Serialize array writers with lower-list link/unlink and exclude new
  slaves until all fallible enslave setup, including XDP, has completed.
- Retain allocation-failure retries, retry failed enslave/open updates,
  rebuild broadcast arrays on open, and protect the peer-notification
  array reader with RCU.

v3: https://lore.kernel.org/all/20261008024031.16224-1-luyun_611@163.com/

Changes in v3:
- Drop the pending latch and all TX-path locking (the mode_lock trylock
  and the netpoll guard); the fallback is now lockless.
- Pin one aggregator object per transmit, by pointer comparison within
  the RCU section, for both hash selection and broadcast replication.
  Never select across aggregator objects.
- Replicate broadcast_neighbor packets to all eligible members of the
  pinned object instead of sending a single copy.
- Defer hashing until the fallback finds a non-empty eligible set.
- Read aggregator is_active with READ_ONCE.

v2: https://lore.kernel.org/all/20260918090137.66515-1-luyun_611@163.com/

Changes in v2:
- Track unpublished array updates with a boolean latch, limiting fallback
  locking and list walks to the asynchronous update window.
- Use spin_trylock_bh() so recursive TX keeps the existing drop or NULL
  behavior instead of deadlocking on mode_lock.
- Use the first eligible port if eligibility changes between the two list
  walks.
- Apply the fallback to ndo_get_xmit_slave and broadcast-neighbor skb
  transmission. Keep control-path readers on the published array and leave
  XDP for a separate follow-up patch.
- Document why hash calculation is deferred until the array count is known.

v1: https://lore.kernel.org/netdev/20260915090207.110987-1-luyun_611@163.com/

 drivers/net/bonding/bond_3ad.c  |  4 +-
 drivers/net/bonding/bond_main.c | 91 +++++++++++++++++++++------------
 include/net/bonding.h           | 10 ++--
 3 files changed, 67 insertions(+), 38 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index d91533b80996..c260e2e45205 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2616,8 +2616,8 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
 	rcu_read_unlock();
 	spin_unlock_bh(&bond->mode_lock);
 
-	if (update_slave_arr)
-		bond_slave_arr_work_rearm(bond, 0);
+	if (update_slave_arr && bond_update_slave_arr(bond, NULL))
+		bond_slave_arr_work_rearm(bond, 1);
 
 	if (should_notify_rtnl && rtnl_trylock()) {
 		bond_slave_state_notify(bond);
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 09aa45d71fab..c3d97983916b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1175,7 +1175,6 @@ static struct slave *bond_find_best_slave(struct bonding *bond)
 /* must be called in RCU critical section or with RTNL held */
 static bool bond_should_notify_peers(struct bonding *bond)
 {
-	struct bond_up_slave *usable;
 	struct slave *slave = NULL;
 
 	if (!bond->send_peer_notif ||
@@ -1189,8 +1188,14 @@ static bool bond_should_notify_peers(struct bonding *bond)
 	 * It is safe to only check bond mode here.
 	 */
 	if (BOND_MODE(bond) == BOND_MODE_8023AD) {
-		usable = rcu_dereference_rtnl(bond->usable_slaves);
-		if (!usable || !READ_ONCE(usable->count))
+		struct bond_up_slave *usable;
+		bool can_notify;
+
+		rcu_read_lock();
+		usable = rcu_dereference(bond->usable_slaves);
+		can_notify = usable && READ_ONCE(usable->count);
+		rcu_read_unlock();
+		if (!can_notify)
 			return false;
 	} else {
 		slave = rcu_dereference_rtnl(bond->curr_active_slave);
@@ -1714,19 +1719,22 @@ static int bond_master_upper_dev_link(struct bonding *bond, struct slave *slave,
 	lag_upper_info.tx_type = type;
 	lag_upper_info.hash_type = bond_lag_hash_type(bond, type);
 
+	mutex_lock(&bond->slave_arr_lock);
 	err = netdev_master_upper_dev_link(slave->dev, bond->dev, slave,
 					   &lag_upper_info, extack);
-	if (err)
-		return err;
+	if (!err)
+		slave->dev->flags |= IFF_SLAVE;
+	mutex_unlock(&bond->slave_arr_lock);
 
-	slave->dev->flags |= IFF_SLAVE;
-	return 0;
+	return err;
 }
 
 static void bond_upper_dev_unlink(struct bonding *bond, struct slave *slave)
 {
+	mutex_lock(&bond->slave_arr_lock);
 	netdev_upper_dev_unlink(slave->dev, bond->dev);
 	slave->dev->flags &= ~IFF_SLAVE;
+	mutex_unlock(&bond->slave_arr_lock);
 }
 
 static void slave_kobj_release(struct kobject *kobj)
@@ -2332,10 +2340,17 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 			bpf_prog_inc(bond->xdp_prog);
 	}
 
+	/* Publish only after all fallible setup, including XDP, is complete. */
+	mutex_lock(&bond->slave_arr_lock);
+	new_slave->array_ready = true;
+	mutex_unlock(&bond->slave_arr_lock);
+
 	/* broadcast mode uses the all_slaves to loop through slaves. */
 	if (bond_mode_can_use_xmit_hash(bond) ||
-	    BOND_MODE(bond) == BOND_MODE_BROADCAST)
-		bond_update_slave_arr(bond, NULL);
+	    BOND_MODE(bond) == BOND_MODE_BROADCAST) {
+		if (bond_update_slave_arr(bond, NULL) && netif_running(bond_dev))
+			bond_slave_arr_work_rearm(bond, 1);
+	}
 
 	bond_xdp_set_features(bond_dev);
 
@@ -4399,8 +4414,11 @@ static int bond_open(struct net_device *bond_dev)
 			static_branch_inc(&bond_bcast_neigh_enabled);
 	}
 
-	if (bond_mode_can_use_xmit_hash(bond))
-		bond_update_slave_arr(bond, NULL);
+	if (bond_mode_can_use_xmit_hash(bond) ||
+	    BOND_MODE(bond) == BOND_MODE_BROADCAST) {
+		if (bond_update_slave_arr(bond, NULL))
+			bond_slave_arr_work_rearm(bond, 1);
+	}
 
 	return 0;
 }
@@ -5068,35 +5086,26 @@ static netdev_tx_t bond_xmit_activebackup(struct sk_buff *skb,
 	return bond_tx_drop(bond_dev, skb);
 }
 
-/* Use this to update slave_array when (a) it's not appropriate to update
- * slave_array right away (note that update_slave_array() may sleep)
- * and / or (b) RTNL is not held.
+/* Retry array updates which failed, or defer them from atomic context.
+ * bond_update_slave_arr() may sleep but does not require RTNL.
  */
 void bond_slave_arr_work_rearm(struct bonding *bond, unsigned long delay)
 {
 	queue_delayed_work(bond->wq, &bond->slave_arr_work, delay);
 }
 
-/* Slave array work handler. Holds only RTNL */
+/* Retry failed array updates; the builder acquires slave_arr_lock. */
 static void bond_slave_arr_handler(struct work_struct *work)
 {
 	struct bonding *bond = container_of(work, struct bonding,
 					    slave_arr_work.work);
 	int ret;
 
-	if (!rtnl_trylock())
-		goto err;
-
 	ret = bond_update_slave_arr(bond, NULL);
-	rtnl_unlock();
 	if (ret) {
 		pr_warn_ratelimited("Failed to update slave array from WT\n");
-		goto err;
+		bond_slave_arr_work_rearm(bond, 1);
 	}
-	return;
-
-err:
-	bond_slave_arr_work_rearm(bond, 1);
 }
 
 static void bond_skip_slave(struct bond_up_slave *slaves,
@@ -5128,7 +5137,10 @@ static void bond_set_slave_arr(struct bonding *bond,
 {
 	struct bond_up_slave *usable, *all;
 
-	all = rtnl_dereference(bond->all_slaves);
+	lockdep_assert_held(&bond->slave_arr_lock);
+
+	all = rcu_dereference_protected(bond->all_slaves,
+					lockdep_is_held(&bond->slave_arr_lock));
 	rcu_assign_pointer(bond->all_slaves, all_slaves);
 	kfree_rcu(all, rcu);
 
@@ -5137,7 +5149,8 @@ static void bond_set_slave_arr(struct bonding *bond,
 		return;
 	}
 
-	usable = rtnl_dereference(bond->usable_slaves);
+	usable = rcu_dereference_protected(bond->usable_slaves,
+					   lockdep_is_held(&bond->slave_arr_lock));
 	rcu_assign_pointer(bond->usable_slaves, usable_slaves);
 	kfree_rcu(usable, rcu);
 }
@@ -5153,20 +5166,25 @@ static void bond_reset_slave_arr(struct bonding *bond)
  * (b) BOND_MODE_XOR
  * (c) (BOND_MODE_TLB || BOND_MODE_ALB) && tlb_dynamic_lb == 0
  *
- * The caller is expected to hold RTNL only and NO other lock!
+ * Acquires slave_arr_lock to stabilize membership and serialize writers.
+ * The caller must be in sleepable context without mode_lock or RCU held.
  */
 int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
 {
 	struct bond_up_slave *usable_slaves = NULL, *all_slaves = NULL;
 	struct slave *slave;
 	struct list_head *iter;
+	unsigned int count;
 	int agg_id = 0;
 	int ret = 0;
 
 	might_sleep();
+	mutex_lock(&bond->slave_arr_lock);
 
-	usable_slaves = kzalloc_flex(*usable_slaves, arr, bond->slave_cnt);
-	all_slaves = kzalloc_flex(*all_slaves, arr, bond->slave_cnt);
+	/* Enslave counts before committing; release unlinks before uncounting. */
+	count = READ_ONCE(bond->slave_cnt);
+	usable_slaves = kzalloc_flex(*usable_slaves, arr, count);
+	all_slaves = kzalloc_flex(*all_slaves, arr, count);
 	if (!usable_slaves || !all_slaves) {
 		ret = -ENOMEM;
 		goto out;
@@ -5189,7 +5207,7 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
 	}
 	rcu_read_lock();
 	bond_for_each_slave(bond, slave, iter) {
-		if (skipslave == slave)
+		if (skipslave == slave || !slave->array_ready)
 			continue;
 
 		all_slaves->arr[all_slaves->count++] = slave;
@@ -5211,17 +5229,21 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
 	rcu_read_unlock();
 
 	bond_set_slave_arr(bond, usable_slaves, all_slaves);
-	return ret;
+	goto out_unlock;
 out:
 	if (ret != 0 && skipslave) {
-		bond_skip_slave(rtnl_dereference(bond->all_slaves),
+		bond_skip_slave(rcu_dereference_protected(bond->all_slaves,
+							  lockdep_is_held(&bond->slave_arr_lock)),
 				skipslave);
-		bond_skip_slave(rtnl_dereference(bond->usable_slaves),
+		bond_skip_slave(rcu_dereference_protected(bond->usable_slaves,
+							  lockdep_is_held(&bond->slave_arr_lock)),
 				skipslave);
 	}
 	kfree_rcu(all_slaves, rcu);
 	kfree_rcu(usable_slaves, rcu);
 
+out_unlock:
+	mutex_unlock(&bond->slave_arr_lock);
 	return ret;
 }
 
@@ -5994,6 +6016,7 @@ void bond_setup(struct net_device *bond_dev)
 	struct bonding *bond = netdev_priv(bond_dev);
 
 	spin_lock_init(&bond->mode_lock);
+	mutex_init(&bond->slave_arr_lock);
 	bond->params = bonding_defaults;
 
 	/* Initialize pointers */
@@ -6073,7 +6096,9 @@ static void bond_uninit(struct net_device *bond_dev)
 	mutex_destroy(&bond->ipsec_lock);
 #endif /* CONFIG_XFRM_OFFLOAD */
 
+	mutex_lock(&bond->slave_arr_lock);
 	bond_set_slave_arr(bond, NULL, NULL);
+	mutex_unlock(&bond->slave_arr_lock);
 
 	list_del_rcu(&bond->bond_list);
 
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 598d56b1bc97..90f7ace7be59 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -23,6 +23,7 @@
 #include <linux/etherdevice.h>
 #include <linux/reciprocal_div.h>
 #include <linux/if_link.h>
+#include <linux/mutex.h>
 
 #include <net/bond_3ad.h>
 #include <net/bond_alb.h>
@@ -75,7 +76,7 @@
  * @pos:	current slave
  * @iter:	list_head * iterator
  *
- * Caller must hold RTNL
+ * Caller must hold RTNL or slave_arr_lock to stabilize the lower list.
  */
 #define bond_for_each_slave(bond, pos, iter) \
 	netdev_for_each_lower_private((bond)->dev, pos, iter)
@@ -177,6 +178,7 @@ struct slave {
 	u32    speed;
 	u16    queue_id;
 	u8     perm_hwaddr[MAX_ADDR_LEN];
+	bool   array_ready; /* Enslave complete; protected by slave_arr_lock. */
 	int    prio;
 	struct ad_slave_info *ad_info;
 	struct tlb_slave_info tlb_info;
@@ -210,8 +212,8 @@ struct bond_ipsec {
 };
 
 /*
- * Here are the locking policies for the two bonding locks:
- * Get rcu_read_lock when reading or RTNL when writing slave list.
+ * Slave list changes hold RTNL and slave_arr_lock. Readers hold RCU,
+ * RTNL, or slave_arr_lock when only stabilizing the lower list.
  */
 struct bonding {
 	struct   net_device *dev; /* first - useful for panic debug */
@@ -220,6 +222,8 @@ struct bonding {
 	struct   slave __rcu *primary_slave;
 	struct   bond_up_slave __rcu *usable_slaves;
 	struct   bond_up_slave __rcu *all_slaves;
+	/* Protects the lower list, array_ready and both TX arrays. */
+	struct mutex slave_arr_lock;
 	bool     force_primary;
 	bool     notifier_ctx;
 	s32      slave_cnt; /* never change this value outside the attach/detach wrappers */
-- 
2.43.0

Re: [PATCH net v4] bonding: 3ad: update transmit arrays without waiting for RTNL

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 (3) · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new per-bond slave_arr_lock mutex is held across all of
  netdev_master_upper_dev_link() in bond_master_upper_dev_link()…
- [Low] Several comments no longer match the code after this patch.
- [Low] This patch initialises slave_arr_lock with mutex_init() in
  bond_setup() (bond_main.c:6019) but never calls mutex_destroy() on it.

Pre-existing issues:
- [High] In 802.3ad mode, an enslave that fails after
  bond_master_upper_dev_link() can leave a use-after-free in the…
- [Medium] Only four call sites rearm slave_arr_work when
  bond_update_slave_arr() fails: bond_3ad_state_machine_handler,…
- [Medium] bond_enslave() increments bond->slave_cnt at bond_main.c:2295
  (`WRITE_ONCE(bond->slave_cnt, bond->slave_cnt + 1);`) before the XDP…
- [Medium] In broadcast mode, bond_set_slave_arr()
  (bond_main.c:5147-5150) frees only the newly passed usable_slaves and…

--

bonding: 3ad: update transmit arrays without waiting for RTNL

This commit makes the 802.3ad state machine rebuild the usable_slaves and
all_slaves transmit arrays itself. Before, it deferred the rebuild to a
worker that retried rtnl_trylock(). It adds a per-bond slave_arr_lock
mutex so array writers are serialized with lower-list link/unlink. It also
adds a per-slave array_ready flag that keeps half-enslaved slaves out of
the arrays.

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 7019e5d8fdc8e..883d0cac07a2f 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c

[ ... ]

> @@ -1714,19 +1719,22 @@ static int bond_master_upper_dev_link(struct bonding *bond, struct slave *slave,
>  	lag_upper_info.tx_type = type;
>  	lag_upper_info.hash_type = bond_lag_hash_type(bond, type);
>  
> +	mutex_lock(&bond->slave_arr_lock);
>  	err = netdev_master_upper_dev_link(slave->dev, bond->dev, slave,
>  					   &lag_upper_info, extack);
> -	if (err)
> -		return err;
> +	if (!err)
> +		slave->dev->flags |= IFF_SLAVE;
> +	mutex_unlock(&bond->slave_arr_lock);
>  
> -	slave->dev->flags |= IFF_SLAVE;
> -	return 0;
> +	return err;
>  }
>  
>  static void bond_upper_dev_unlink(struct bonding *bond, struct slave *slave)
>  {
> +	mutex_lock(&bond->slave_arr_lock);
>  	netdev_upper_dev_unlink(slave->dev, bond->dev);
>  	slave->dev->flags &= ~IFF_SLAVE;
> +	mutex_unlock(&bond->slave_arr_lock);
>  }

[Severity: Medium]
Is slave_arr_lock meant to be held across the whole of
netdev_master_upper_dev_link() and netdev_upper_dev_unlink()?

Those calls run the NETDEV_PRECHANGEUPPER and NETDEV_CHANGEUPPER notifier
chains (switchdev/LAG offload, bridge, RDMA handlers). They also create
and remove sysfs links and allocate memory. Every lock those subscribers
take now nests inside a bonding-private mutex.

This patch also makes bond_3ad_state_machine_handler() call the builder
directly instead of queueing slave_arr_work:

    if (update_slave_arr && bond_update_slave_arr(bond, NULL))
        bond_slave_arr_work_rearm(bond, 1);

bond_update_slave_arr() now does mutex_lock(&bond->slave_arr_lock) and
two GFP_KERNEL allocations.

bond->wq comes from alloc_ordered_workqueue(). While enslave or release is
inside the notifier chain, ad_work blocks on the mutex, and every other
bond work item waits behind it.

The new slave is already in the lower list before CHANGEUPPER fires.
ad_agg_selection_logic() sets update_slave_arr on most ticks when the
active aggregator has no partner. So this looks likely during enslave.

Won't this hold up the LACP tick and LACPDU transmission for as long as
the notifier handlers run?

There is also a theoretical self-deadlock on the bond_enslave()
err_upper_unlink path. There, IFF_SLAVE is still set and the rx_handler
is still registered:

bond_upper_dev_unlink()
    mutex_lock(&bond->slave_arr_lock)
    netdev_upper_dev_unlink()
        CHANGEUPPER subscriber raises NETDEV_UP/CHANGE/DOWN on lower dev
            bond_slave_netdev_event()
                bond_update_slave_arr()
                    mutex_lock(&bond->slave_arr_lock)

No in-tree subscriber appears to do this today.

The commit message doesn't discuss this trade-off. Only the lower-list
change itself, and its ordering against the rebuild after unlink, need
the lock. Would a narrower critical section, or a trylock-and-requeue in
the ad handler, keep the LACP worker from blocking?

[ ... ]

> @@ -2332,10 +2340,17 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  			bpf_prog_inc(bond->xdp_prog);
>  	}
>  
> +	/* Publish only after all fallible setup, including XDP, is complete. */
> +	mutex_lock(&bond->slave_arr_lock);
> +	new_slave->array_ready = true;
> +	mutex_unlock(&bond->slave_arr_lock);

[Severity: High]
This isn't a bug introduced by this patch, but can a failed 802.3ad
enslave leave a freed struct port linked into another slave's aggregator?

bond_enslave() calls bond_3ad_bind_slave(new_slave) before linking. Once
bond_master_upper_dev_link() succeeds, the new slave sits at the tail of
the RCU lower list. bond_3ad_state_machine_handler() runs under mode_lock
and RCU, not RTNL, so it will process that port.

The port is not AD_PORT_SELECTED, so ad_port_selection_logic() attaches
it to the first free aggregator:

    if (!aggregator->lag_ports) {
        if (!free_aggregator)
            free_aggregator = aggregator;
        continue;
    }
    ...
    free_aggregator->lag_ports = port;
    free_aggregator->num_of_ports++;

Suppose slaves A and B already share A's aggregator. B's aggregator is
free and is visited before C's, so a new port C joins B's aggregator.

A later enslave step can then fail: bond_sysfs_slave_add(),
dev_set_promiscuity()/dev_set_allmulti(), or the three XDP checks that
goto err_sysfs_del. The unwind runs bond_sysfs_slave_del(),
bond_upper_dev_unlink(), netdev_rx_handler_unregister(),
synchronize_rcu() and kobject_put(). None of those labels calls
bond_3ad_unbind_slave(). It is only reached from __bond_release_one().

slave_kobj_release() then frees the port:

    if (BOND_MODE(bond) == BOND_MODE_8023AD)
        kfree(SLAVE_AD_INFO(slave));

B's aggregator still points at C's port. On every later tick,
ad_agg_selection_logic() calls __agg_active_ports(agg) and walks
B.agg->lag_ports into freed memory. __disable_port() and __enable_port()
can also write through it.

One reliable way to hit the failure: attach an XDP program to the bond,
then enslave a device that lacks ndo_bpf, or one that already has an XDP
program loaded. The same error path also leaves slave_cnt incremented
(see below).

The new array_ready flag keeps this half-enslaved slave out of the TX
arrays, but the 3AD state machine still sees its port. Should the enslave
error path undo bond_3ad_bind_slave() and any aggregator attachment?

[ ... ]

> @@ -5068,35 +5086,26 @@ static netdev_tx_t bond_xmit_activebackup(struct sk_buff *skb,
>  	return bond_tx_drop(bond_dev, skb);
>  }
>  
> -/* Use this to update slave_array when (a) it's not appropriate to update
> - * slave_array right away (note that update_slave_array() may sleep)
> - * and / or (b) RTNL is not held.
> +/* Retry array updates which failed, or defer them from atomic context.
> + * bond_update_slave_arr() may sleep but does not require RTNL.
>   */
>  void bond_slave_arr_work_rearm(struct bonding *bond, unsigned long delay)
>  {
>  	queue_delayed_work(bond->wq, &bond->slave_arr_work, delay);
>  }

[Severity: Medium]
This is a pre-existing issue. The new comment says this work retries
array updates that failed, but only four callers rearm on failure:
bond_3ad_state_machine_handler(), bond_enslave() (when running),
bond_open() and bond_slave_arr_handler().

The other callers ignore the -ENOMEM return:

  bond_3ad_handle_link_change()
  __bond_release_one(), which only falls back to bond_skip_slave()
  bond_miimon_link_change() for XOR
  bond_loadbalance_arp_mon() for XOR
  bond_slave_netdev_event() on NETDEV_UP/CHANGE/DOWN
  bond_alb_handle_link_change(), which only logs with pr_err()

If the allocation fails at one of these sites, usable_slaves and
all_slaves stay stale and nothing retries.

For example, take an XOR bond with no miimon or arpmon whose only slave
lost carrier. Its array is empty. If the rebuild fails when carrier
returns, bond_3ad_xor_xmit() keeps dropping packets.

bond_slave_arr_handler() no longer needs RTNL. Would it make sense to
rearm slave_arr_work from these sites too?

[ ... ]

> @@ -5153,20 +5166,25 @@ static void bond_reset_slave_arr(struct bonding *bond)
>   * (b) BOND_MODE_XOR
>   * (c) (BOND_MODE_TLB || BOND_MODE_ALB) && tlb_dynamic_lb == 0
>   *
> - * The caller is expected to hold RTNL only and NO other lock!
> + * Acquires slave_arr_lock to stabilize membership and serialize writers.
> + * The caller must be in sleepable context without mode_lock or RCU held.
>   */

[Severity: Low]
This isn't a bug, but a few comments no longer match the code after this
change.

This doc block still says it builds "the usable slaves array" for
802.3ad, XOR, and TLB/ALB with tlb_dynamic_lb == 0. But:

  - the function also builds and publishes all_slaves
  - this patch adds a BOND_MODE_BROADCAST call from bond_open(), and
    broadcast calls already existed in bond_enslave() and
    __bond_release_one()
  - bond_mode_can_use_xmit_hash() is true for TLB and ALB whatever
    tlb_dynamic_lb is set to

The new comment above bond_slave_arr_work_rearm() says the work can
"defer them from atomic context". All four callers only use it to retry
after bond_update_slave_arr() fails.

bond_3ad_handle_link_change() still justifies its call with RTNL:

    /* RTNL is held and mode_lock is released so it's safe
     * to update slave_array here.
     */
    bond_update_slave_arr(slave->bond, NULL);

The new rule is sleepable context without mode_lock or RCU held; RTNL is
no longer part of it.

>  int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
>  {

[ ... ]

>  	might_sleep();
> +	mutex_lock(&bond->slave_arr_lock);
>  
> -	usable_slaves = kzalloc_flex(*usable_slaves, arr, bond->slave_cnt);
> -	all_slaves = kzalloc_flex(*all_slaves, arr, bond->slave_cnt);
> +	/* Enslave counts before committing; release unlinks before uncounting. */
> +	count = READ_ONCE(bond->slave_cnt);
> +	usable_slaves = kzalloc_flex(*usable_slaves, arr, count);
> +	all_slaves = kzalloc_flex(*all_slaves, arr, count);

[Severity: Medium]
This is a pre-existing issue, but this comment assumes slave_cnt follows
enslave and release, and bond_enslave() can leave it permanently too
high.

bond_enslave() does this before the XDP block:

    WRITE_ONCE(bond->slave_cnt, bond->slave_cnt + 1);

The XDP block has three goto err_sysfs_del exits:

  - the slave lacks ndo_bpf/ndo_xdp_xmit while bond->xdp_prog is set
  - dev_xdp_prog_count(slave_dev) > 0
  - dev_xdp_propagate() fails

None of the labels from err_sysfs_del down to err_undo_flags decrements
slave_cnt. The only decrement is in __bond_release_one().

Over-allocating here is harmless. But bond_xmit_roundrobin_slave_get()
does this:

    slave_id = bond_rr_gen_slave_id(bond) % slave_cnt;

After a failed enslave, an id past the real count makes
bond_get_slave_by_id() fall back to the first tx-capable slave. That
skews the round-robin distribution. bond_info_query() also reports the
wrong num_slaves.

Should the XDP error paths undo the slave_cnt increment?

[ ... ]

> @@ -6073,7 +6096,9 @@ static void bond_uninit(struct net_device *bond_dev)
>  	mutex_destroy(&bond->ipsec_lock);
>  #endif /* CONFIG_XFRM_OFFLOAD */
>  
> +	mutex_lock(&bond->slave_arr_lock);
>  	bond_set_slave_arr(bond, NULL, NULL);

[Severity: Medium]
This isn't a bug introduced by this patch, but does this leak the old
usable_slaves array if the bond is in broadcast mode at teardown?

bond_set_slave_arr() returns early for broadcast:

    if (BOND_MODE(bond) == BOND_MODE_BROADCAST) {
        kfree_rcu(usable_slaves, rcu);
        return;
    }

So the published bond->usable_slaves is never freed. One sequence that
triggers it:

  ip link add bond0 type bond mode xor
  ip link set bond0 up      (bond_open() publishes usable_slaves)
  ip link set bond0 down    (bond_close() doesn't reset the arrays)
  ip link set bond0 type bond mode broadcast
  ip link del bond0

bond_uninit() takes the broadcast early return, and the usable_slaves
array allocated in XOR mode is lost when the netdev is freed.

> +	mutex_unlock(&bond->slave_arr_lock);

[Severity: Low]
This isn't a bug, but bond_setup() initializes slave_arr_lock with
mutex_init() and nothing calls mutex_destroy() on it. ipsec_lock just
above does get destroyed.

Should a matching mutex_destroy() go here? It would need to come after
any work that can still call bond_update_slave_arr() has been cancelled.

>  
>  	list_del_rcu(&bond->bond_list);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029025411.1087287-1-luyun_611%40163.com