Re: [PATCH net] bonding: do not clear curr_active_slave prematurely when releasing all slaves

[Jay Vosburgh <jv@jvosburgh.net>]

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