Netdev List
 help / color / mirror / Atom feed
* [PATCH net] bonding: do not clear curr_active_slave prematurely when releasing all slaves
@ 2026-08-31 20:30 Eric Dumazet
  2026-09-02  0:16 ` Jay Vosburgh
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Eric Dumazet @ 2026-08-31 20:30 UTC (permalink / raw)
  To: David S . Miller, Jakub Kicinski, Paolo Abeni
  Cc: Simon Horman, netdev, eric.dumazet, Eric Dumazet, Jay Vosburgh

When releasing all slaves during bond destruction (all == true),
__bond_release_one() unconditionally clears bond->curr_active_slave to
NULL in every iteration.

If a backup slave is released before the active slave,
bond_alb_deinit_slave() triggers rlb_teach_disabled_mac_on_primary(),
which increments the active slave dev promiscuity counter and sets
bond_info->primary_is_promisc = 1.

Because bond->curr_active_slave was prematurely cleared to NULL when
releasing the backup slave, the subsequent iteration releasing the active
slave evaluates oldcurrent as NULL, so bond_change_active_slave(bond, NULL)
is skipped. Consequently, bond_alb_handle_active_change() is never called
to decrement the promiscuity counter, permanently leaking promiscuous
mode on the physical device after bond teardown.

When oldcurrent == slave, bond_change_active_slave(bond, NULL) already sets
bond->curr_active_slave to NULL. We only need to avoid selecting a new
active slave when all == true. Replace the if (all) branch with
if (!all && oldcurrent == slave).

Fixes: 0896341a44bf ("bonding: fix bond_release_all inconsistencies")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
Cc: Jay Vosburgh <jv@jvosburgh.net>
---
 drivers/net/bonding/bond_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index c23cf18a996a..1e5ab3454872 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2517,9 +2517,7 @@ static int __bond_release_one(struct net_device *bond_dev,
 		bond_alb_deinit_slave(bond, slave);
 	}
 
-	if (all) {
-		RCU_INIT_POINTER(bond->curr_active_slave, NULL);
-	} else if (oldcurrent == slave) {
+	if (!all && oldcurrent == slave) {
 		/* Note that we hold RTNL over this sequence, so there
 		 * is no concern that another slave add/remove event
 		 * will interfere.
-- 
2.55.0.970.g62bdec98f9-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH net] bonding: do not clear curr_active_slave prematurely when releasing all slaves
  2026-08-31 20:30 [PATCH net] bonding: do not clear curr_active_slave prematurely when releasing all slaves Eric Dumazet
@ 2026-09-02  0:16 ` Jay Vosburgh
  2026-09-02  7:40 ` Nikolay Aleksandrov
  2026-09-03  1:30 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 4+ messages in thread
From: Jay Vosburgh @ 2026-09-02  0:16 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman,
	netdev, eric.dumazet

Eric Dumazet <edumazet@google.com> wrote:

>When releasing all slaves during bond destruction (all == true),
>__bond_release_one() unconditionally clears bond->curr_active_slave to
>NULL in every iteration.
>
>If a backup slave is released before the active slave,
>bond_alb_deinit_slave() triggers rlb_teach_disabled_mac_on_primary(),
>which increments the active slave dev promiscuity counter and sets
>bond_info->primary_is_promisc = 1.
>
>Because bond->curr_active_slave was prematurely cleared to NULL when
>releasing the backup slave, the subsequent iteration releasing the active
>slave evaluates oldcurrent as NULL, so bond_change_active_slave(bond, NULL)
>is skipped. Consequently, bond_alb_handle_active_change() is never called
>to decrement the promiscuity counter, permanently leaking promiscuous
>mode on the physical device after bond teardown.
>
>When oldcurrent == slave, bond_change_active_slave(bond, NULL) already sets
>bond->curr_active_slave to NULL. We only need to avoid selecting a new
>active slave when all == true. Replace the if (all) branch with
>if (!all && oldcurrent == slave).
>
>Fixes: 0896341a44bf ("bonding: fix bond_release_all inconsistencies")
>Signed-off-by: Eric Dumazet <edumazet@google.com>

	Complicated failure path, but looks correct.

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

	-J


>---
>Cc: Jay Vosburgh <jv@jvosburgh.net>
>---
> drivers/net/bonding/bond_main.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index c23cf18a996a..1e5ab3454872 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2517,9 +2517,7 @@ static int __bond_release_one(struct net_device *bond_dev,
> 		bond_alb_deinit_slave(bond, slave);
> 	}
> 
>-	if (all) {
>-		RCU_INIT_POINTER(bond->curr_active_slave, NULL);
>-	} else if (oldcurrent == slave) {
>+	if (!all && oldcurrent == slave) {
> 		/* Note that we hold RTNL over this sequence, so there
> 		 * is no concern that another slave add/remove event
> 		 * will interfere.
>-- 
>2.55.0.970.g62bdec98f9-goog
>

---
	-Jay Vosburgh, jv@jvosburgh.net

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net] bonding: do not clear curr_active_slave prematurely when releasing all slaves
  2026-08-31 20:30 [PATCH net] bonding: do not clear curr_active_slave prematurely when releasing all slaves Eric Dumazet
  2026-09-02  0:16 ` Jay Vosburgh
@ 2026-09-02  7:40 ` Nikolay Aleksandrov
  2026-09-03  1:30 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 4+ messages in thread
From: Nikolay Aleksandrov @ 2026-09-02  7:40 UTC (permalink / raw)
  To: Eric Dumazet, David S . Miller, Jakub Kicinski, Paolo Abeni
  Cc: Simon Horman, netdev, eric.dumazet, Jay Vosburgh

On 31/08/2026 23:30, Eric Dumazet wrote:
> When releasing all slaves during bond destruction (all == true),
> __bond_release_one() unconditionally clears bond->curr_active_slave to
> NULL in every iteration.
> 
> If a backup slave is released before the active slave,
> bond_alb_deinit_slave() triggers rlb_teach_disabled_mac_on_primary(),
> which increments the active slave dev promiscuity counter and sets
> bond_info->primary_is_promisc = 1.
> 
> Because bond->curr_active_slave was prematurely cleared to NULL when
> releasing the backup slave, the subsequent iteration releasing the active
> slave evaluates oldcurrent as NULL, so bond_change_active_slave(bond, NULL)
> is skipped. Consequently, bond_alb_handle_active_change() is never called
> to decrement the promiscuity counter, permanently leaking promiscuous
> mode on the physical device after bond teardown.
> 
> When oldcurrent == slave, bond_change_active_slave(bond, NULL) already sets
> bond->curr_active_slave to NULL. We only need to avoid selecting a new
> active slave when all == true. Replace the if (all) branch with
> if (!all && oldcurrent == slave).
> 
> Fixes: 0896341a44bf ("bonding: fix bond_release_all inconsistencies")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> Cc: Jay Vosburgh <jv@jvosburgh.net>
> ---
>   drivers/net/bonding/bond_main.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index c23cf18a996a..1e5ab3454872 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2517,9 +2517,7 @@ static int __bond_release_one(struct net_device *bond_dev,
>   		bond_alb_deinit_slave(bond, slave);
>   	}
>   
> -	if (all) {
> -		RCU_INIT_POINTER(bond->curr_active_slave, NULL);
> -	} else if (oldcurrent == slave) {
> +	if (!all && oldcurrent == slave) {
>   		/* Note that we hold RTNL over this sequence, so there
>   		 * is no concern that another slave add/remove event
>   		 * will interfere.

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net] bonding: do not clear curr_active_slave prematurely when releasing all slaves
  2026-08-31 20:30 [PATCH net] bonding: do not clear curr_active_slave prematurely when releasing all slaves Eric Dumazet
  2026-09-02  0:16 ` Jay Vosburgh
  2026-09-02  7:40 ` Nikolay Aleksandrov
@ 2026-09-03  1:30 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2026-09-03  1:30 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: davem, kuba, pabeni, horms, netdev, eric.dumazet, jv

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 31 Aug 2026 20:30:42 +0000 you wrote:
> When releasing all slaves during bond destruction (all == true),
> __bond_release_one() unconditionally clears bond->curr_active_slave to
> NULL in every iteration.
> 
> If a backup slave is released before the active slave,
> bond_alb_deinit_slave() triggers rlb_teach_disabled_mac_on_primary(),
> which increments the active slave dev promiscuity counter and sets
> bond_info->primary_is_promisc = 1.
> 
> [...]

Here is the summary with links:
  - [net] bonding: do not clear curr_active_slave prematurely when releasing all slaves
    https://git.kernel.org/netdev/net/c/af602c7aa5fe

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-09-03  1:31 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-31 20:30 [PATCH net] bonding: do not clear curr_active_slave prematurely when releasing all slaves Eric Dumazet
2026-09-02  0:16 ` Jay Vosburgh
2026-09-02  7:40 ` Nikolay Aleksandrov
2026-09-03  1:30 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox