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

[Nikolay Aleksandrov <razor@blackwall.org>]

On 21/08/2026 13:42, Hangbin Liu wrote:
> Hi Nikolay,
> On Fri, Aug 21, 2026 at 01:16:20PM +0300, Nikolay Aleksandrov wrote:
>>> -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.

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
have to mark unknown speed with S64_MIN but it will compute the correct numbers
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.

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

It doesn't to me, it still doesn't differentiate between NICs that are
overloaded differently.

> 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