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

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

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.

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