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

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

Hi Nikolay,
On Tue, Aug 18, 2026 at 02:51:31PM +0300, Nikolay Aleksandrov wrote:
> > > > @@ -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
> > > 
> > 
> > I meant - it doesn't improve on the current situation where it can also happen. :)

Ah, yes. I forgot this. The reset_unbalanced_load() could be called on any
CPU, which conflicts with other writers.

I re-checked the code. unbalanced_load is only called in two situations:
1. To rebalance the load in bond_alb_monitor(), which only executes once
   every 10 seconds.
2. !tx_slave in bond_do_alb_xmit(), which is only for multicast/broadcast
   traffic. This traffic shouldn't be significant.

So looks using spin_lock here is acceptable. What do you think?

> > 
> 
> Sorry for the multiple replies, but thinking about this - having multiple concurrent
> writers could cause write tearing for 32-bit architectures (the monitor is a writer
> and can write concurrently with tx) leading to invalid result.

Never mind. A detailed explanation is always welcome. I really appreciate
your review and comments.

Best Regards
Hangbin