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

[Nikolay Aleksandrov]

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.