Re: [PATCH net] bonding: 3ad: select a port when the TX array is empty

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Lock re-entrancy: bond_3ad_xmit_fallback() adds
  spin_lock_bh(&bond->mode_lock) to the ndo_start_xmit path…
- [Medium] New per-packet mode_lock acquisition plus O(nr_slaves) list
  walks on the 802.3ad transmit path whenever usable_slaves is empty.
- [Medium] Divergent semantics for 'usable_slaves is empty in 802.3ad
  mode' across peer call sites.
- [Medium] The patch's stated motivation is that 'ARP requests still fail
  to transmit' during 802.3ad bring-up, but with the broadcast_neighbor…
- [Low] bond_3ad_xmit_fallback() counts eligible ports in one pass and
  re-walks the list in a second pass to pick the target-th eligible…
- [Low] Undescribed hunk: the patch also reorders bond_xmit_hash() to
  after the count check inside bond_xmit_3ad_xor_slave_get()…

--

bonding: 3ad: select a port when the TX array is empty

This adds a fallback in bond_3ad_xor_xmit() so that, in 802.3ad mode, an
empty usable_slaves array no longer means an automatic drop: a port is
picked directly from the active aggregator under bond->mode_lock.  A few
questions below about the new locking on the transmit path and about the
peer call sites that were left with the old semantics.

No Link: tags in the commit header.

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index a9bff7663eeca..2cf280bf134f2 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5233,11 +5233,11 @@ static struct slave *bond_xmit_3ad_xor_slave_get(struct bonding *bond,
>  	unsigned int count;
>  	u32 hash;
>  
> -	hash = bond_xmit_hash(bond, skb);
>  	count = slaves ? READ_ONCE(slaves->count) : 0;
>  	if (unlikely(!count))
>  		return NULL;
>  
> +	hash = bond_xmit_hash(bond, skb);
>  	slave = slaves->arr[hash % count];
>  	return slave;
>  }

[Severity: Low]
This isn't a bug, but could the changelog mention this hunk?  It moves
bond_xmit_hash() below the count check in bond_xmit_3ad_xor_slave_get(),
which is also called from bond_xmit_get_slave().

The reorder looks like a prerequisite of the new design, so the hash is
computed once (in the helper for the array path, in bond_3ad_xor_xmit()
for the fallback path), and it appears functionally neutral because
bond_xmit_hash()/__bond_xmit_hash() only read the skb.  Right now a
reader has to work that out on their own.

> @@ -5289,9 +5289,65 @@ static bool bond_should_broadcast_neighbor(struct sk_buff *skb,
>  	return false;
>  }
>  
> -/* Use this Xmit function for 3AD as well as XOR modes. The current
> - * usable slave array is formed in the control path. The xmit function
> - * just calculates hash and sends the packet out.
> +/* Called with RCU and bond->mode_lock held. */
> +static bool bond_3ad_slave_is_eligible(struct slave *slave)
> +{
> +	const struct aggregator *agg;
> +
> +	agg = rcu_dereference(SLAVE_AD_INFO(slave)->port.aggregator);
> +	return agg && agg->is_active && bond_slave_can_tx(slave);
> +}
> +
> +/* Called with RCU held when the transmit array has not caught up with
> + * the 802.3ad state machine. Do not enable ports here: use the same
> + * eligibility checks as bond_update_slave_arr().
> + */
> +static struct slave *bond_3ad_xmit_fallback(struct bonding *bond, u32 hash)
> +{
> +	struct slave *selected = NULL;
> +	unsigned int eligible = 0;
> +	unsigned int target;
> +	struct list_head *iter;
> +	struct slave *slave;
> +
> +	/* Netpoll can re-enter TX while the state machine holds mode_lock.
> +	 * Keep the existing empty-array drop behavior in that context.
> +	 */
> +	if (unlikely(netpoll_tx_running(bond->dev)))
> +		return NULL;
> +
> +	/* Aggregator selection temporarily clears all is_active flags. Keep
> +	 * both loops under mode_lock to avoid observing that intermediate
> +	 * state. The caller's RCU read lock protects the selected slave.
> +	 */
> +	spin_lock_bh(&bond->mode_lock);

[Severity: High]
Is the netpoll check enough to cover all the ways TX can be entered with
a mode_lock already held?  The 802.3ad state machine holds mode_lock
across its whole sweep, and LACPDU transmission happens inside that
section:

bond_3ad_state_machine_handler()
    spin_lock_bh(&bond->mode_lock);
    ...
    ad_tx_machine(port);
        ad_lacpdu_send(port);
            skb->dev = slave->dev;
            dev_queue_xmit(skb);

bond_3ad_unbind_slave() does the same thing, calling ad_lacpdu_send()
with mode_lock held.

Bond devices set IFF_NO_QUEUE, so dev_queue_xmit() calls the lower
device's ndo_start_xmit synchronously in the same context.  If the lower
device is itself an 802.3ad bond (bond_enslave() only rejects non-bond
masters, so bond-over-bond is allowed) whose usable array is still empty
- the very bring-up window this patch targets - then:

bond_3ad_state_machine_handler()      /* outer bond mode_lock held */
  -> ad_lacpdu_send() -> dev_queue_xmit()
    -> inner bond bond_start_xmit() -> __bond_start_xmit()
      -> bond_3ad_xor_xmit()         /* empty usable_slaves */
        -> bond_3ad_xmit_fallback()
          -> spin_lock_bh(&inner->mode_lock);

Since mode_lock has a single lockdep class (one spin_lock_init() in
bond_setup(), no subclass or nest_lock annotation), wouldn't lockdep
report possible recursive locking here and then disable itself?

There is a worse variant: if egress processing on the lower device
redirects the frame back into the same bond (tc mirred egress redirect or
a bpf redirect run from sch_handle_egress in the same call), doesn't the
same CPU re-acquire the same non-recursive mode_lock with softirqs
disabled and spin forever?  netpoll_tx_running() does not cover that
case, since these paths run with interrupts enabled.

Would it be possible to avoid taking mode_lock on the transmit path
altogether, or to move LACPDU transmission out of the mode_lock section?

> +	bond_for_each_slave_rcu(bond, slave, iter)
> +		if (bond_3ad_slave_is_eligible(slave))
> +			eligible++;
> +
> +	if (!eligible)
> +		goto out;

[Severity: Medium]
How expensive does this become when the array is empty for a long time?
Every skb that finds an empty array now takes a bond-wide spinlock and
walks the full lower-device list (once when nothing is eligible, twice
otherwise), and is then dropped anyway.

The empty-array state is not only the transient rebuild window described
in the changelog.  bond_update_slave_arr() publishes an empty array
whenever bond_3ad_get_active_agg_info() fails:

	/* No active aggragator means it's not safe to use
	 * the previous array.
	 */
	bond_reset_slave_arr(bond);

That covers the whole duration of LACP non-convergence (peer switch not
running LACP, all members down), and bond devices are IFF_NO_QUEUE so
skbs still reach ndo_start_xmit with carrier down.

The same mode_lock is held by bond_3ad_state_machine_handler() across its
entire sweep and by bond_3ad_rx_indication() on LACPDU receive, so
sustained TX bounces the lock against the very state machine that would
end the fallback state.  Would an "update pending" latch, or making the
fallback one-shot, bound this?

> +
> +	target = hash % eligible;
> +	bond_for_each_slave_rcu(bond, slave, iter) {
> +		if (!bond_3ad_slave_is_eligible(slave))
> +			continue;
> +
> +		if (!target) {
> +			selected = slave;
> +			break;
> +		}
> +		target--;
> +	}

[Severity: Low]
Does mode_lock really keep the two passes consistent?  It serialises the
802.3ad state (agg->is_active), but the other inputs of
bond_3ad_slave_is_eligible() are not written under it:

include/net/bonding.h:
static inline bool bond_slave_can_tx(struct slave *slave)
{
	return bond_slave_is_up(slave) && slave->link == BOND_LINK_UP &&
	       bond_is_active_slave(slave);
}

bond_slave_is_up() reads netif_running()/netif_carrier_ok(), which the
lower driver can flip via netif_carrier_off() with no bond lock at all,
and slave->link / slave->backup are written by the monitors
(bond_miimon_commit()) under RTNL only.

So if the hash picked the last eligible index (target == eligible - 1)
and one slave becomes ineligible between the two passes, doesn't the
second walk decrement target for each remaining eligible slave, never
reach target == 0, and return NULL - dropping the packet the fallback was
supposed to send?

In the opposite direction, a slave becoming eligible between the passes
skews the hash distribution relative to the array path, which does its
single walk under RTNL.  Could the changelog's claim that "hash selection
remains equivalent" be narrowed to agg->is_active?

> +
> +out:
> +	spin_unlock_bh(&bond->mode_lock);
> +	return selected;
> +}
> +
> +/* Use this Xmit function for 3AD as well as XOR modes. The usable slave
> + * array is formed in the control path.  In 3AD mode, fall back to the
> + * current port state while an empty array awaits an update.
>   */
>  static netdev_tx_t bond_3ad_xor_xmit(struct sk_buff *skb,
>  				     struct net_device *dev)
> @@ -5302,6 +5358,8 @@ static netdev_tx_t bond_3ad_xor_xmit(struct sk_buff *skb,
>  
>  	slaves = rcu_dereference(bond->usable_slaves);
>  	slave = bond_xmit_3ad_xor_slave_get(bond, skb, slaves);
> +	if (unlikely(!slave) && BOND_MODE(bond) == BOND_MODE_8023AD)
> +		slave = bond_3ad_xmit_fallback(bond, bond_xmit_hash(bond, skb));

[Severity: Medium]
Should the fallback live in the shared helper
bond_xmit_3ad_xor_slave_get() instead?  Several other readers of the same
"usable_slaves is empty in 802.3ad mode" condition were left with the old
meaning:

bond_xdp_xmit_3ad_xor_slave_get() open-codes the same lookup and still
returns NULL:

	count = slaves ? READ_ONCE(slaves->count) : 0;
	if (unlikely(!count))
		return NULL;

so XDP_TX/XDP_REDIRECT frames keep getting dropped in exactly the window
this patch closes for skbs.

bond_xmit_get_slave(), the ndo_get_xmit_slave callback, also still
returns NULL.  Its documented contract in include/linux/netdevice.h is to
return the slave the master will use for that skb, and
netdev_get_xmit_slave() is consumed by drivers/infiniband/core/lag.c.
Doesn't that now tell an offload consumer "no egress slave" for an skb
the bond does in fact transmit?

Two control-path readers still encode the old invariant as well:

bond_should_notify_peers():
	usable = rcu_dereference_rtnl(bond->usable_slaves);
	if (!usable || !READ_ONCE(usable->count))
		return false;

bond_miimon_inspect():
	if (usable_slaves && usable_slaves->count == 0)
		ignore_updelay = true;

Is peer notification meant to stay suppressed, and updelay ignored, for a
bond that can now transmit?

[Severity: Medium]
The changelog says "ARP requests still fail to transmit".  With the
broadcast_neighbor option enabled, do ARP and IPv6 neighbour-discovery
packets ever reach this fallback?

__bond_start_xmit() short-circuits before bond_3ad_xor_xmit():

	case BOND_MODE_8023AD:
		if (bond_should_broadcast_neighbor(skb, dev))
			return bond_xmit_broadcast(skb, dev, false);

and bond_xmit_broadcast() reads the same asynchronously updated array:

	slaves_count = slaves ? READ_ONCE(slaves->count) : 0;
	for (i = 0; i < slaves_count; i++) {

With an empty or NULL array the loop body never runs, the skb is freed
and NET_XMIT_DROP is returned, so the symptom described in the commit
message survives for bonds configured with broadcast_neighbor=1 even
though the active aggregator has eligible ports.  Should that path get
the same treatment, or is the residual gap intentional?

>  	if (likely(slave))
>  		return bond_dev_queue_xmit(bond, skb, slave->dev);
>

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