Re: [PATCH net v4 2/2] bonding: fix u32 overflow in compute_gap()

Hi Nikolay,
On Fri, Aug 21, 2026 at 01:16:20PM +0300, Nikolay Aleksandrov wrote:

quoted

-static long long compute_gap(struct slave *slave)
+static u64 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 */
+	u64 slave_load = SLAVE_TLB_INFO(slave).load << 3; /* Bytes to bits */
+	u32 raw_speed = READ_ONCE(slave->speed);
+	u64 speed = (u64)raw_speed << 20; /* Convert to bits per sec */
+
+	/* It's meaningless to compare gap on unknown speed NIC */
+	if (raw_speed == (u32)SPEED_UNKNOWN)
+		return 0;
+
+	/* Skip slave which is over loaded */
+	if (speed <= slave_load)
+		return 0;
+
+	return speed - slave_load;
  }
  static struct slave *tlb_get_least_loaded_slave(struct bonding *bond)
  {
  	struct slave *slave, *least_loaded;
  	struct list_head *iter;
-	long long max_gap;
+	u64 max_gap = 0;
  	least_loaded = NULL;
-	max_gap = LLONG_MIN;
  	/* Find the slave with the largest gap */
  	bond_for_each_slave_rcu(bond, slave, iter) {
  		if (bond_slave_can_tx(slave)) {
-			long long gap = compute_gap(slave);
+			u64 gap = compute_gap(slave);
-			if (max_gap < gap) {
+			/* Make sure we have one available slave */
+			if (max_gap <= gap) {
  				least_loaded = slave;
  				max_gap = gap;

I think Sashiko's review has a point here:
"Does clamping the gap to 0 completely break load balancing when all interfaces
are overloaded?
When all slaves are overloaded, compute_gap() returns 0 for all of them. Since
max_gap is initialized to 0, max_gap <= gap will evaluate to 0 <= 0, which is
true.
This means tlb_get_least_loaded_slave() will continually update least_loaded to
the current slave, ultimately routing all traffic to the last slave in the list
instead of distributing it across the least overloaded interfaces."

Yes, I have thought about this question. Previous code set max_gap LLONG_MIN,
so there always has a slave assigned. Now we use u64. If we use (max_gap < gap),
there may return NULL pointer.

That is, compute_gap makes multiple different scenarios look the same:
 if speed is unknown           = 0
 if exactly equal capacity     = 0
 if overloaded by *any* amount = 0

Yes, if there is are 2 NICs with 1 Gbps and 10 Gbps, but both shows as
unknown. There is no meaning to compare the gaps. Because they use the same
speed value (u32)-1 << 20.

If two NICs both overloaded or equal capacity. There is also no mean to select
any devices.

So Sashiko's comment seems correct, it doesn't matter if a slave is overloaded
with 1 gbps or 100, they will look the same.

I've thought like:

  if (speed over load)
  	return 1;
  if (speed unknown)
  	return 2;

That sounds reasonable: we could select an unknown‑speed NIC that is not
overloaded. However, this does not work.

When performing the `speed <= slave_load` check, the speed value has already
been shifted. Unknown speed is converted to a very large value, so the
calculation will never report an overload condition — even though the actual
hardware may already be overloaded.

This is why I end up returning 0 for all such cases. Hope my explanation
is clear.

Thanks
Hangbin