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

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

[Xiang Mei (Microsoft)]

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@microsoft.com
Signed-off-by: Xiang Mei (Microsoft) <xmei5@asu.edu>
---
v2: keep vars' rev-x-mas tree order

 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..839f7482dc18 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -1534,8 +1534,8 @@ void bond_alb_monitor(struct work_struct *work)
 	struct bonding *bond = container_of(work, struct bonding,
 					    alb_work.work);
 	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
+	struct slave *slave, *curr;
 	struct list_head *iter;
-	struct slave *slave;
 
 	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();
-- 
2.43.0

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

[Nikolay Aleksandrov]

On Sat, Jul 25, 2026 at 11:39:30PM +0000, 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@microsoft.com
> Signed-off-by: Xiang Mei (Microsoft) <xmei5@asu.edu>
> ---
> v2: keep vars' rev-x-mas tree order
> 
>  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..839f7482dc18 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -1534,8 +1534,8 @@ void bond_alb_monitor(struct work_struct *work)
>  	struct bonding *bond = container_of(work, struct bonding,
>  					    alb_work.work);
>  	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
> +	struct slave *slave, *curr;
>  	struct list_head *iter;
> -	struct slave *slave;
>  
>  	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();
> -- 
> 2.43.0
> 

Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

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

[Jay Vosburgh]

Xiang Mei (Microsoft) <xmei5@asu.edu> 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@microsoft.com
>Signed-off-by: Xiang Mei (Microsoft) <xmei5@asu.edu>

Acked-by: Jay Vosburgh <jv@jvosburgh.net>


>---
>v2: keep vars' rev-x-mas tree order
>
> 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..839f7482dc18 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -1534,8 +1534,8 @@ void bond_alb_monitor(struct work_struct *work)
> 	struct bonding *bond = container_of(work, struct bonding,
> 					    alb_work.work);
> 	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>+	struct slave *slave, *curr;
> 	struct list_head *iter;
>-	struct slave *slave;
> 
> 	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();
>-- 
>2.43.0
>