Re: [PATCH] bonding: alb: Fix overflow in TLB gap calculation

[netdev-bot+sashiko@kernel.org]

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

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

- [High] compute_gap() (drivers/net/bonding/bond_alb.c:163) now widens
  slave->speed to 64 bits before shifting, but slave->speed is a u32…

Pre-existing issues:
- [Low] The comment retained on the rewritten line, `/* Convert to
  Megabit per sec */` (drivers/net/bonding/bond_alb.c:163), mislabels…

--

bonding: alb: Fix overflow in TLB gap calculation

This changes compute_gap() in the TLB/ALB balancer so that slave->speed and
the per-slave TLB load are cast to s64 before being shifted, rather than
being shifted in 32-bit width and cast afterwards. The stated goal is to
avoid overflow of the gap value on high speed devices.

> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index 43ac8e28e4182..b52004030a4b0 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -160,8 +160,8 @@ static void tlb_deinitialize(struct bonding *bond)
>  
>  static long long compute_gap(struct slave *slave)
>  {
> -	return (s64) (slave->speed << 20) - /* Convert to Megabit per sec */
> -	       (s64) (SLAVE_TLB_INFO(slave).load << 3); /* Bytes to bits */
> +	return ((s64)slave->speed << 20) - /* Convert to Megabit per sec */

[Severity: High]
What happens here when slave->speed holds SPEED_UNKNOWN?

slave->speed is a u32, and bond_update_speed_duplex() in bond_main.c stores
the sentinel on its failure path:

drivers/net/bonding/bond_main.c:bond_update_speed_duplex() {
	...
speed_duplex_unknown:
	slave->speed = SPEED_UNKNOWN;
	slave->duplex = DUPLEX_UNKNOWN;
}

That is (u32)-1, so after the new pre-shift cast the first term becomes
((s64)0xFFFFFFFF << 20), roughly 4.5e15. The load term can never offset
that: ((s64)U32_MAX << 3) is only about 3.4e10, and even a real 100 Gbit/s
slave reaches only about 1.05e11.

Does that make tlb_get_least_loaded_slave() pick the unknown-speed slave
permanently?

	bond_for_each_slave_rcu(bond, slave, iter) {
		if (bond_slave_can_tx(slave)) {
			long long gap = compute_gap(slave);

			if (max_gap < gap) {
				least_loaded = slave;
				max_gap = gap;
			}
		}
	}

bond_slave_can_tx() in include/net/bonding.h only checks up/link/active and
never looks at speed:

	return bond_slave_is_up(slave) && slave->link == BOND_LINK_UP &&
	       bond_is_active_slave(slave);

so the sentinel reaches the comparison and every newly hashed TLB client
lands on that slave. Since the periodic rebalance clears assignments, would
this end up steering effectively all clients onto it?

Before this change the sentinel was shifted in 32-bit width, giving
0xFFFFFFFF << 20 == 0xFFF00000 (about 4.29e9), which the load term could
grow past, so the imbalance corrected itself. Does widening the arithmetic
remove that accidental cap and make it permanent?

Some paths where a tx-eligible TLB slave can carry SPEED_UNKNOWN:

- bond_enslave() sets new_slave->link = BOND_LINK_DOWN when the speed query
  fails and bond_needs_speed_duplex(), but a few lines later does
  new_slave->link = BOND_LINK_NOCHANGE, and the final else branch sets
  BOND_LINK_UP when both miimon and arp_interval are 0.
- bond_slave_netdev_event() on NETDEV_UP/NETDEV_CHANGE only forces a
  down/fail state for BOND_MODE_8023AD:

	if (bond_update_speed_duplex(slave) &&
	    BOND_MODE(bond) == BOND_MODE_8023AD) {

- bond_ethtool_get_link_ksettings() re-runs bond_update_speed_duplex() on an
  already tx-capable slave and can latch SPEED_UNKNOWN without touching
  slave->link:

	bond_update_speed_duplex(slave);
	netdev_unlock_ops(slave->dev);
	if (slave->speed != SPEED_UNKNOWN) {

Would it make sense to also clamp or skip SPEED_UNKNOWN in compute_gap()
(treat it as 0, or exclude the slave) alongside the widening?

> +	       ((s64)SLAVE_TLB_INFO(slave).load << 3); /* Bytes to bits */
>  }
>  
>  static struct slave *tlb_get_least_loaded_slave(struct bonding *bond)

[Severity: Low]
This isn't a bug introduced by this patch, but since the line is being
rewritten anyway: is the retained comment /* Convert to Megabit per sec */
accurate?

slave->speed is already in Mbit/s. It is copied straight from ethtool in
bond_update_speed_duplex():

	slave->speed = ecmd.base.speed;

and bond_procfs.c prints it as:

		seq_printf(seq, "Speed: %d Mbps\n", slave->speed);

So the << 20 goes from Mbit/s to bit/s, matching the second term whose
comment already says /* Bytes to bits */ (load_history is documented in
include/net/bond_alb.h as being in Bps). As written, a reader auditing units
would conclude the two subtracted terms are in different units. Would
something like /* Mbit/s to bit/s */ be clearer?

Also, << 20 scales by 1048576 while a megabit is 10^6 bits, about a 4.9%
bias. Presumably that is intentional, but nothing in the code says so.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913125859.2349-1-lirongqing%40baidu.com