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

[Hangbin Liu <hangbin.liu@linux.dev>]

On Fri, Aug 21, 2026 at 02:33:39PM +0300, Nikolay Aleksandrov wrote:
> > > 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.
> 
> Well that is debatable, to be correct you'd like to choose the NIC that is
> least overloaded, one could be at capacity and the other could be 10Gbps above
> capacity and you can still choose the second with this.
> 
> If you make it a signed comparison then you can choose the least loaded, you'd

Oh, do you want to fallback to use s64 (long long) in compute_gap? Then
all the counters need to using s64. The same with unbalanced_load, and we
can't using the "delta" anymore. Do we need to change back to using spin_lock
to protect the unbalanced_load writing.

> have to mark unknown speed with S64_MIN but it will compute the correct numbers

Here do you mean
	if (raw_speed == (u32)SPEED_UNKNOWN)
		s64 speed = S64_MIN

? Then the 's64 gap = speed - load' will overflow, which means a 1Gbps NIC
(shown as unknown) will have more gaps then 10Gbps NIC (correctly shown speed)

> and you can choose the least overloaded NIC, which the current code actually
> does correctly.
> 
> And most importantly - you definitely want to differentiate between unknown speed
> and overload, these should not be the same.

If we use s64 and all slaves are overloaded, we can compute the difference.
But once there is an unknown speed NIC, we lose visibility into the real difference.
Such a NIC could be 1G, 10G, or 100G, yet we set its speed to `(u32)-1`.

That is why I believe comparing gaps for NICs with unknown speed is meaningless.

Regarding overload scenarios: do you think this is a common‑case situation?
Because in practice, we rarely hit the theoretical maximum link speed.
For example, a 10Gbps NIC typically peaks at around ~950 Mbps.

Thanks
Hangbin