Re: [PATCH net] bonding: alb: re-check primary_is_promisc under RTNL in bond_alb_monitor

[Xiang Mei]

On Tue, Jul 21, 2026 at 1:22 AM Nikolay Aleksandrov <razor@xxxxxxxxxxxxx> wrote:
>
> On 21/07/2026 01:34, Xiang Mei (Microsoft) wrote:
> > bond_alb_monitor() reads primary_is_promisc under RCU, then drops RCU and
> > takes RTNL via rtnl_trylock() before undoing the promiscuity it set on the
> > active slave. In that window the active slave can change under RTNL
> > (RTM_DELLINK -> __bond_release_one() -> bond_alb_handle_active_change()),
> > which already drops the promiscuity and clears primary_is_promisc. The
> > monitor still acts on the stale decision: if the slave was removed with no
> > failover, curr_active_slave is now NULL and the deref faults; if it failed
> > over, the stale dev_set_promiscuity(-1) underflows the new slave's
> > promiscuity counter and pins it in IFF_PROMISC.
> >
> >    Oops: general protection fault, probably for non-canonical address ...
> >    KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
> >    Workqueue: b42 bond_alb_monitor
> >    RIP: 0010:bond_alb_monitor (drivers/net/bonding/bond_alb.c:1600)
> >     process_one_work (kernel/workqueue.c:3322)
> >     worker_thread (kernel/workqueue.c:3486)
> >     kthread (kernel/kthread.c:436)
> >     ret_from_fork (arch/x86/kernel/process.c:158)
> >    Kernel panic - not syncing: Fatal exception
> >
> > Re-check primary_is_promisc (and curr_active_slave) after taking RTNL so
> > the monitor only undoes an increment it still owns. The other bonding
> > monitors already re-read state under RTNL in their commit phase
> > (bond_miimon_commit/bond_ab_arp_commit); bond_alb_monitor() was the only
> > one acting on the pre-trylock decision.
> >
> > Fixes: d0e81b7e2246 ("bonding: Acquire correct locks in alb for promisc change")
> > Reported-by: AutonomousCodeSecurity@xxxxxxxxxxxxx
> > Signed-off-by: Xiang Mei (Microsoft) <xmei5@xxxxxxx>
> > ---
> >   drivers/net/bonding/bond_alb.c | 10 ++++++----
> >   1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> > index 2d37b07c8215..70458c5b23cc 100644
> > --- a/drivers/net/bonding/bond_alb.c
> > +++ b/drivers/net/bonding/bond_alb.c
> > @@ -1535,7 +1535,7 @@ void bond_alb_monitor(struct work_struct *work)
> >                                           alb_work.work);
> >       struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
> >       struct list_head *iter;
> > -     struct slave *slave;
> > +     struct slave *slave, *curr;
>
> please move this up to keep the reverse xmas tree ordering
>
Thanks for your review and reminder. I fixed the ordering issue and sent v2:

https://lore.kernel.org/all/20260725233930.2957317-1-xmei5%40asu.edu/

Best,
Xiang

> >
> >       if (!bond_has_slaves(bond)) {
> >               atomic_set(&bond_info->tx_rebalance_counter, 0);
> > @@ -1597,9 +1597,11 @@ void bond_alb_monitor(struct work_struct *work)
> >                        * because a slave was disabled then
> >                        * it can now leave promiscuous mode.
> >                        */
> > -                     dev_set_promiscuity(rtnl_dereference(bond->curr_active_slave)->dev,
> > -                                         -1);
> > -                     bond_info->primary_is_promisc = 0;
> > +                     curr = rtnl_dereference(bond->curr_active_slave);
> > +                     if (bond_info->primary_is_promisc && curr) {
> > +                             dev_set_promiscuity(curr->dev, -1);
> > +                             bond_info->primary_is_promisc = 0;
> > +                     }
> >
> >                       rtnl_unlock();
> >                       rcu_read_lock();
>