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

[Nikolay Aleksandrov <razor@blackwall.org>]

On 21/08/2026 15:58, Hangbin Liu wrote:
> 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.
> 

why? see more below

>> 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)
> 

oh that is easily fixed, it should not be a problem

>> 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`.

no, we use signed and set it at S64_MIN, it is never chosen.

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

Right and they shouldn't be considered or rather should be last.

> 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

Completely untested, but something like:

-static u64 compute_gap(struct slave *slave)
+static s64 compute_gap(struct slave *slave)
  {
         u64 slave_load = SLAVE_TLB_INFO(slave).load << 3;
         u32 raw_speed = READ_ONCE(slave->speed);
         u64 speed = (u64)raw_speed << 20;

         if (raw_speed == (u32)SPEED_UNKNOWN)
-               return 0;
-
-       if (speed <= slave_load)
-               return 0;
+               return S64_MIN;

-       return speed - slave_load;
+       return (s64)speed - (s64)slave_load;
  }

Then change the selection variables and comparison:

-       u64 max_gap = 0;
+       s64 max_gap = S64_MIN;

...

-                       u64 gap = compute_gap(slave);
+                       s64 gap = compute_gap(slave);

-                       if (max_gap <= gap) {
+                       if (!least_loaded || max_gap < gap) {

This should choose the slave with smallest gap and put the unknown speed behind all
slaves with known speeds.