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

[Nikolay Aleksandrov <razor@blackwall.org>]

On 18/08/2026 11:47, Hangbin Liu wrote:
> From: Hangbin Liu <liuhangbin@kylinos.cn>
> 
> The TLB load-tracking fields tx_bytes, load_history, load, and
> unbalanced_load are all u32. At sustained throughput above ~3.2 Gbit/s
> over the 10-second rebalance interval the byte counters wrap, causing
> compute_gap() to produce incorrect gap values and mis-select slaves.
> Such speeds are common on modern NICs under heavy traffic.
> 
> Widen these fields to u64. Use u64_stats_sync to protect the per-cpu
> unbalanced_load_stats against tearing on 32-bit architectures, and
> div_u64() for the 64-bit divisions. The tx_bytes, load, and load_history
> are protected in spin_lock.
> 
> Rework compute_gap() to use u64 arithmetic throughout. Return 0 when the
> speed is unknown or the slave is already overloaded.
> 
> Detected by AI code review.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Hangbin Liu <liuhangbin@kylinos.cn>
> ---
>   drivers/net/bonding/bond_alb.c  | 56 ++++++++++++++++++++++++++++++-----------
>   drivers/net/bonding/bond_main.c |  2 +-
>   include/net/bond_alb.h          |  9 ++++---
>   3 files changed, 47 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index d54d834cf72b..659a77323444 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -6,6 +6,7 @@
>   #include <linux/skbuff.h>
>   #include <linux/netdevice.h>
>   #include <linux/etherdevice.h>
> +#include <linux/ethtool.h>
>   #include <linux/pkt_sched.h>
>   #include <linux/spinlock.h>
>   #include <linux/slab.h>
> @@ -74,8 +75,8 @@ static inline u8 _simple_hash(const u8 *hash_start, int hash_size)
>   static inline void tlb_init_table_entry(struct tlb_client_info *entry, int save_load)
>   {
>   	if (save_load) {
> -		entry->load_history = 1 + entry->tx_bytes /
> -				      BOND_TLB_REBALANCE_INTERVAL;
> +		entry->load_history = 1 + div_u64(entry->tx_bytes,
> +				      BOND_TLB_REBALANCE_INTERVAL);
>   		entry->tx_bytes = 0;
>   	}
>   
> @@ -158,25 +159,35 @@ static void tlb_deinitialize(struct bonding *bond)
>   	spin_unlock_bh(&bond->mode_lock);
>   }
>   
> -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 */
> +	u32 raw_speed = READ_ONCE(slave->speed);
> +	u64 speed = (u64)raw_speed;
> +
> +	/* 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 << 20) <= (SLAVE_TLB_INFO(slave).load << 3))
> +		return 0;
> +
> +	return (speed << 20) - /* Convert to Megabit per sec */
> +	       (SLAVE_TLB_INFO(slave).load << 3); /* Bytes to bits */
>   }
>   
>   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) {
>   				least_loaded = slave;
> @@ -1344,8 +1355,14 @@ static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
>   	if (!tx_slave) {
>   		/* unbalanced or unassigned, send through primary */
>   		tx_slave = rcu_dereference(bond->curr_active_slave);
> -		if (bond->params.tlb_dynamic_lb)
> -			this_cpu_add(bond_info->unbalanced_load->tx_bytes, skb->len);
> +		if (bond->params.tlb_dynamic_lb) {
> +			struct unbalanced_load_stats *pcpu_load;
> +
> +			pcpu_load = this_cpu_ptr(bond_info->unbalanced_load);
> +			u64_stats_update_begin(&pcpu_load->syncp);
> +			u64_stats_add(&pcpu_load->tx_bytes, skb->len);
> +			u64_stats_update_end(&pcpu_load->syncp);

this still races with...

> +		}
>   	}
>   
>   	if (tx_slave && bond_slave_can_tx(tx_slave)) {
> @@ -1529,19 +1546,28 @@ netdev_tx_t bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>   	return bond_do_alb_xmit(skb, bond, tx_slave);
>   }
>   
> -static u32 reset_unbalanced_load(struct alb_bond_info *bond_info)
> +static u64 reset_unbalanced_load(struct alb_bond_info *bond_info)
>   {
>   	struct unbalanced_load_stats *p;
> -	u32 total_bytes = 0;
> +	u64 tx_bytes, total_bytes = 0;
> +	unsigned int start;
>   	int i;
>   
>   	for_each_possible_cpu(i) {
>   		p = per_cpu_ptr(bond_info->unbalanced_load, i);
> -		total_bytes += READ_ONCE(p->tx_bytes);
> -		WRITE_ONCE(p->tx_bytes, 0);
> +		do {
> +			start = u64_stats_fetch_begin(&p->syncp);
> +			tx_bytes = u64_stats_read(&p->tx_bytes);
> +		} while (u64_stats_fetch_retry(&p->syncp, start));
> +
> +		u64_stats_update_begin(&p->syncp);
> +		u64_stats_set(&p->tx_bytes, 0);
> +		u64_stats_update_end(&p->syncp);

... this here, as u64_stats_update_begin doesn't provide exclusive access, so writers
must do that themselves, so you can't be sure what value will end up, the zeroing
might not work at all and can get overwritten

> +
> +		total_bytes += tx_bytes;
>   	}
>   
> -	return total_bytes / BOND_TLB_REBALANCE_INTERVAL;
> +	return div_u64(total_bytes, BOND_TLB_REBALANCE_INTERVAL);
>   }
>   
>   void bond_alb_monitor(struct work_struct *work)
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 9fb44e0031c8..4c4d9bf71e0c 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -6495,7 +6495,7 @@ static int bond_init(struct net_device *bond_dev)
>   	if (!bond->wq)
>   		return -ENOMEM;
>   
> -	bond->alb_info.unbalanced_load = alloc_percpu(struct unbalanced_load_stats);
> +	bond->alb_info.unbalanced_load = netdev_alloc_pcpu_stats(struct unbalanced_load_stats);
>   	if (!bond->alb_info.unbalanced_load)
>   		goto wq_out;
>   
> diff --git a/include/net/bond_alb.h b/include/net/bond_alb.h
> index 3fabf4714dec..51c083c76115 100644
> --- a/include/net/bond_alb.h
> +++ b/include/net/bond_alb.h
> @@ -57,12 +57,12 @@ struct tlb_client_info {
>   				 * packets to a Client that the Hash function
>   				 * gave this entry index.
>   				 */
> -	u32 tx_bytes;		/* Each Client accumulates the BytesTx that
> +	u64 tx_bytes;		/* Each Client accumulates the BytesTx that
>   				 * were transmitted to it, and after each
>   				 * CallBack the LoadHistory is divided
>   				 * by the balance interval
>   				 */
> -	u32 load_history;	/* This field contains the amount of Bytes
> +	u64 load_history;	/* This field contains the amount of Bytes
>   				 * that were transmitted to this client by
>   				 * the server on the previous balance
>   				 * interval in Bps.
> @@ -118,13 +118,14 @@ struct tlb_slave_info {
>   			 * are the entries that were assigned to use this
>   			 * slave for transmit.
>   			 */
> -	u32 load;	/* Each slave sums the loadHistory of all clients
> +	u64 load;	/* Each slave sums the loadHistory of all clients
>   			 * assigned to it
>   			 */
>   };
>   
>   struct unbalanced_load_stats {
> -	u32			tx_bytes;
> +	u64_stats_t		tx_bytes;
> +	struct u64_stats_sync	syncp;
>   };
>   
>   struct alb_bond_info {
>