[PATCH net] bonding: fix multicast MAC address synchronization

[PATCH net] bonding: fix multicast MAC address synchronization

[Hangbin Liu]

There is a corner case where the NS (Neighbor Solicitation) target is set to
an invalid or unreachable address. In such cases, all the slave links are
marked as down and set to backup. This causes the bond to add multicast MAC
addresses to all slaves.

However, bond_ab_arp_probe() later tries to activate a carrier on slave and
sets it as active. If we subsequently change or clear the NS targets, the
call to bond_slave_ns_maddrs_del() on this interface will fail because it
is still marked active, and the multicast MAC address will remain.

To fix this issue, move the NS multicast address add/remove logic into
bond_set_slave_state() to ensure multicast MAC addresses are updated
synchronously whenever the slave state changes.

Note: The call to bond_slave_ns_maddrs_del() in __bond_release_one() is
kept, as it is still required to clean up multicast MAC addresses when
a slave is removed.

Fixes: 8eb36164d1a6 ("bonding: add ns target multicast address to slave device")
Reported-by: Liang Li <liali@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 9 ---------
 include/net/bonding.h           | 7 +++++++
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 8ea183da8d53..6dde6f870ee2 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1004,8 +1004,6 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
 
 		if (bond->dev->flags & IFF_UP)
 			bond_hw_addr_flush(bond->dev, old_active->dev);
-
-		bond_slave_ns_maddrs_add(bond, old_active);
 	}
 
 	if (new_active) {
@@ -1022,8 +1020,6 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
 			dev_mc_sync(new_active->dev, bond->dev);
 			netif_addr_unlock_bh(bond->dev);
 		}
-
-		bond_slave_ns_maddrs_del(bond, new_active);
 	}
 }
 
@@ -2350,11 +2346,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	bond_compute_features(bond);
 	bond_set_carrier(bond);
 
-	/* Needs to be called before bond_select_active_slave(), which will
-	 * remove the maddrs if the slave is selected as active slave.
-	 */
-	bond_slave_ns_maddrs_add(bond, new_slave);
-
 	if (bond_uses_primary(bond)) {
 		block_netpoll_tx();
 		bond_select_active_slave(bond);
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 95f67b308c19..0041f7a2bd18 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -385,7 +385,14 @@ static inline void bond_set_slave_state(struct slave *slave,
 	if (slave->backup == slave_state)
 		return;
 
+	if (slave_state == BOND_STATE_ACTIVE)
+		bond_slave_ns_maddrs_del(slave->bond, slave);
+
 	slave->backup = slave_state;
+
+	if (slave_state == BOND_STATE_BACKUP)
+		bond_slave_ns_maddrs_add(slave->bond, slave);
+
 	if (notify) {
 		bond_lower_state_changed(slave);
 		bond_queue_slave_event(slave);
-- 
2.46.0

Re: [PATCH net] bonding: fix multicast MAC address synchronization

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>There is a corner case where the NS (Neighbor Solicitation) target is set to
>an invalid or unreachable address. In such cases, all the slave links are
>marked as down and set to backup. This causes the bond to add multicast MAC
>addresses to all slaves.
>
>However, bond_ab_arp_probe() later tries to activate a carrier on slave and
>sets it as active. If we subsequently change or clear the NS targets, the
>call to bond_slave_ns_maddrs_del() on this interface will fail because it
>is still marked active, and the multicast MAC address will remain.

	This seems complicated, so, just to make sure I'm clear, the bug
being fixed here happens when:

(a) ARP monitor is running with NS target(s), all of which do not
solicit a reply (invalid address or unreachable), resulting in all
interfaces in the bond being marked down

(b) while in the above state, the ARP monitor will cycle through each
interface, making them "active" (active-ish, really, just enough for the
ARP mon stuff to work) in turn to check for a response to a probe

(c) while the cycling from (b) is occurring, attempts to change a NS
target will fail on the interface that happens to be quasi-"active" at
the moment.

	Is my summary correct?

	Doesn't the failure scenario also require that arp_validate be
enabled?  Looking at bond_slave_ns_maddrs_{add,del}, they do nothing if
arp_validate is off.

>To fix this issue, move the NS multicast address add/remove logic into
>bond_set_slave_state() to ensure multicast MAC addresses are updated
>synchronously whenever the slave state changes.

	Ok, but state change calls happen in a lot more places than the
existing bond_hw_addr_swap(), which is only called during change of
active for active-backup, balance-alb, and balance-tlb.  Are you sure
that something goofy like setting arp_validate and an NS target with the
ARP monitor disabled (or in a mode that disallows it) will behave
rationally?

>Note: The call to bond_slave_ns_maddrs_del() in __bond_release_one() is
>kept, as it is still required to clean up multicast MAC addresses when
>a slave is removed.
>
>Fixes: 8eb36164d1a6 ("bonding: add ns target multicast address to slave device")
>Reported-by: Liang Li <liali@redhat.com>
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>---
> drivers/net/bonding/bond_main.c | 9 ---------
> include/net/bonding.h           | 7 +++++++
> 2 files changed, 7 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 8ea183da8d53..6dde6f870ee2 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1004,8 +1004,6 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
> 
> 		if (bond->dev->flags & IFF_UP)
> 			bond_hw_addr_flush(bond->dev, old_active->dev);
>-
>-		bond_slave_ns_maddrs_add(bond, old_active);
> 	}
> 
> 	if (new_active) {
>@@ -1022,8 +1020,6 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
> 			dev_mc_sync(new_active->dev, bond->dev);
> 			netif_addr_unlock_bh(bond->dev);
> 		}
>-
>-		bond_slave_ns_maddrs_del(bond, new_active);
> 	}
> }
> 
>@@ -2350,11 +2346,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> 	bond_compute_features(bond);
> 	bond_set_carrier(bond);
> 
>-	/* Needs to be called before bond_select_active_slave(), which will
>-	 * remove the maddrs if the slave is selected as active slave.
>-	 */
>-	bond_slave_ns_maddrs_add(bond, new_slave);
>-
> 	if (bond_uses_primary(bond)) {
> 		block_netpoll_tx();
> 		bond_select_active_slave(bond);
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index 95f67b308c19..0041f7a2bd18 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -385,7 +385,14 @@ static inline void bond_set_slave_state(struct slave *slave,
> 	if (slave->backup == slave_state)
> 		return;
> 
>+	if (slave_state == BOND_STATE_ACTIVE)
>+		bond_slave_ns_maddrs_del(slave->bond, slave);
>+
> 	slave->backup = slave_state;
>+
>+	if (slave_state == BOND_STATE_BACKUP)
>+		bond_slave_ns_maddrs_add(slave->bond, slave);

	This code pattern kind of makes it look like the slave->backup
assignment must happen between the two new if blocks.  I don't think
that's true, and things would work correctly if the slave->backup
assignment happened first (or last).

	Assuming I'm correct, could you move the assignment so it's not
in the middle?  If, however, it does need to be in the middle, that
deserves a comment explaining why.

	-J

>+
> 	if (notify) {
> 		bond_lower_state_changed(slave);
> 		bond_queue_slave_event(slave);
>-- 
>2.46.0
>

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

Re: [PATCH net] bonding: fix multicast MAC address synchronization

[Hangbin Liu]

On Fri, May 23, 2025 at 02:03:47PM -0700, Jay Vosburgh wrote:
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
> >There is a corner case where the NS (Neighbor Solicitation) target is set to
> >an invalid or unreachable address. In such cases, all the slave links are
> >marked as down and set to backup. This causes the bond to add multicast MAC
> >addresses to all slaves.
> >
> >However, bond_ab_arp_probe() later tries to activate a carrier on slave and
> >sets it as active. If we subsequently change or clear the NS targets, the
> >call to bond_slave_ns_maddrs_del() on this interface will fail because it
> >is still marked active, and the multicast MAC address will remain.
> 
> 	This seems complicated, so, just to make sure I'm clear, the bug
> being fixed here happens when:
> 
> (a) ARP monitor is running with NS target(s), all of which do not
> solicit a reply (invalid address or unreachable), resulting in all
> interfaces in the bond being marked down
> 
> (b) while in the above state, the ARP monitor will cycle through each
> interface, making them "active" (active-ish, really, just enough for the
> ARP mon stuff to work) in turn to check for a response to a probe

Yes

> 
> (c) while the cycling from (b) is occurring, attempts to change a NS
> target will fail on the interface that happens to be quasi-"active" at
> the moment.

Yes, this is because bond_slave_ns_maddrs_del() must ensure the deletion
happens on a backup slave only. However, during ARP monitor, it set one of
the slaves to active, this causes the deletion of multicast MAC addresses to
be skipped on that interface.

> 	Is my summary correct?
> 
> 	Doesn't the failure scenario also require that arp_validate be
> enabled?  Looking at bond_slave_ns_maddrs_{add,del}, they do nothing if
> arp_validate is off.

Yes, it need.

> 
> >To fix this issue, move the NS multicast address add/remove logic into
> >bond_set_slave_state() to ensure multicast MAC addresses are updated
> >synchronously whenever the slave state changes.
> 
> 	Ok, but state change calls happen in a lot more places than the
> existing bond_hw_addr_swap(), which is only called during change of
> active for active-backup, balance-alb, and balance-tlb.  Are you sure
> that something goofy like setting arp_validate and an NS target with the
> ARP monitor disabled (or in a mode that disallows it) will behave
> rationally?

The slave_can_set_ns_maddr() in slave_set_ns_maddrs could check the bond mode
and if the slave is active. But no arp_interval checking. I can add it in the
checking to avoid the miss-config. e.g.

diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 91893c29b899..21116362cc24 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -1241,6 +1241,7 @@ static int bond_option_arp_ip_targets_set(struct bonding *bond,
 static bool slave_can_set_ns_maddr(const struct bonding *bond, struct slave *slave)
 {
        return BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP &&
+              bond->params.arp_interval &&
               !bond_is_active_slave(slave) &&
               slave->dev->flags & IFF_MULTICAST;
 }

> 
> >Note: The call to bond_slave_ns_maddrs_del() in __bond_release_one() is
> >kept, as it is still required to clean up multicast MAC addresses when
> >a slave is removed.
> >
> >Fixes: 8eb36164d1a6 ("bonding: add ns target multicast address to slave device")
> >Reported-by: Liang Li <liali@redhat.com>
> >Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> >---
> > drivers/net/bonding/bond_main.c | 9 ---------
> > include/net/bonding.h           | 7 +++++++
> > 2 files changed, 7 insertions(+), 9 deletions(-)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index 8ea183da8d53..6dde6f870ee2 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -1004,8 +1004,6 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
> > 
> > 		if (bond->dev->flags & IFF_UP)
> > 			bond_hw_addr_flush(bond->dev, old_active->dev);
> >-
> >-		bond_slave_ns_maddrs_add(bond, old_active);
> > 	}
> > 
> > 	if (new_active) {
> >@@ -1022,8 +1020,6 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
> > 			dev_mc_sync(new_active->dev, bond->dev);
> > 			netif_addr_unlock_bh(bond->dev);
> > 		}
> >-
> >-		bond_slave_ns_maddrs_del(bond, new_active);
> > 	}
> > }
> > 
> >@@ -2350,11 +2346,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> > 	bond_compute_features(bond);
> > 	bond_set_carrier(bond);
> > 
> >-	/* Needs to be called before bond_select_active_slave(), which will
> >-	 * remove the maddrs if the slave is selected as active slave.
> >-	 */
> >-	bond_slave_ns_maddrs_add(bond, new_slave);
> >-
> > 	if (bond_uses_primary(bond)) {
> > 		block_netpoll_tx();
> > 		bond_select_active_slave(bond);
> >diff --git a/include/net/bonding.h b/include/net/bonding.h
> >index 95f67b308c19..0041f7a2bd18 100644
> >--- a/include/net/bonding.h
> >+++ b/include/net/bonding.h
> >@@ -385,7 +385,14 @@ static inline void bond_set_slave_state(struct slave *slave,
> > 	if (slave->backup == slave_state)
> > 		return;
> > 
> >+	if (slave_state == BOND_STATE_ACTIVE)
> >+		bond_slave_ns_maddrs_del(slave->bond, slave);
> >+
> > 	slave->backup = slave_state;
> >+
> >+	if (slave_state == BOND_STATE_BACKUP)
> >+		bond_slave_ns_maddrs_add(slave->bond, slave);
> 
> 	This code pattern kind of makes it look like the slave->backup
> assignment must happen between the two new if blocks.  I don't think
> that's true, and things would work correctly if the slave->backup
> assignment happened first (or last).

The slave->backup assignment must happen between the two if blocks, because

bond_slave_ns_maddrs_add/del only do the operation on backup slave.
So if a interface become active, we need to call maddrs_del before it set
backup state to active. If a interface become backup. We need to call
maddrs_add after the backup state set to backup.

I will add a comment in the code.

Thanks
Hangbin
> 
> 	Assuming I'm correct, could you move the assignment so it's not
> in the middle?  If, however, it does need to be in the middle, that
> deserves a comment explaining why.
> 
> 	-J
> 
> >+
> > 	if (notify) {
> > 		bond_lower_state_changed(slave);
> > 		bond_queue_slave_event(slave);
> >-- 
> >2.46.0
> >
> 
> ---
> 	-Jay Vosburgh, jv@jvosburgh.net

Re: [PATCH net] bonding: fix multicast MAC address synchronization

[Hangbin Liu]

Hi Jay,

Any comments?

Hangbin
On Mon, May 26, 2025 at 08:51:52AM +0000, Hangbin Liu wrote:
> On Fri, May 23, 2025 at 02:03:47PM -0700, Jay Vosburgh wrote:
> > Hangbin Liu <liuhangbin@gmail.com> wrote:
> > 
> > >There is a corner case where the NS (Neighbor Solicitation) target is set to
> > >an invalid or unreachable address. In such cases, all the slave links are
> > >marked as down and set to backup. This causes the bond to add multicast MAC
> > >addresses to all slaves.
> > >
> > >However, bond_ab_arp_probe() later tries to activate a carrier on slave and
> > >sets it as active. If we subsequently change or clear the NS targets, the
> > >call to bond_slave_ns_maddrs_del() on this interface will fail because it
> > >is still marked active, and the multicast MAC address will remain.
> > 
> > 	This seems complicated, so, just to make sure I'm clear, the bug
> > being fixed here happens when:
> > 
> > (a) ARP monitor is running with NS target(s), all of which do not
> > solicit a reply (invalid address or unreachable), resulting in all
> > interfaces in the bond being marked down
> > 
> > (b) while in the above state, the ARP monitor will cycle through each
> > interface, making them "active" (active-ish, really, just enough for the
> > ARP mon stuff to work) in turn to check for a response to a probe
> 
> Yes
> 
> > 
> > (c) while the cycling from (b) is occurring, attempts to change a NS
> > target will fail on the interface that happens to be quasi-"active" at
> > the moment.
> 
> Yes, this is because bond_slave_ns_maddrs_del() must ensure the deletion
> happens on a backup slave only. However, during ARP monitor, it set one of
> the slaves to active, this causes the deletion of multicast MAC addresses to
> be skipped on that interface.
> 
> > 	Is my summary correct?
> > 
> > 	Doesn't the failure scenario also require that arp_validate be
> > enabled?  Looking at bond_slave_ns_maddrs_{add,del}, they do nothing if
> > arp_validate is off.
> 
> Yes, it need.
> 
> > 
> > >To fix this issue, move the NS multicast address add/remove logic into
> > >bond_set_slave_state() to ensure multicast MAC addresses are updated
> > >synchronously whenever the slave state changes.
> > 
> > 	Ok, but state change calls happen in a lot more places than the
> > existing bond_hw_addr_swap(), which is only called during change of
> > active for active-backup, balance-alb, and balance-tlb.  Are you sure
> > that something goofy like setting arp_validate and an NS target with the
> > ARP monitor disabled (or in a mode that disallows it) will behave
> > rationally?
> 
> The slave_can_set_ns_maddr() in slave_set_ns_maddrs could check the bond mode
> and if the slave is active. But no arp_interval checking. I can add it in the
> checking to avoid the miss-config. e.g.
> 
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 91893c29b899..21116362cc24 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -1241,6 +1241,7 @@ static int bond_option_arp_ip_targets_set(struct bonding *bond,
>  static bool slave_can_set_ns_maddr(const struct bonding *bond, struct slave *slave)
>  {
>         return BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP &&
> +              bond->params.arp_interval &&
>                !bond_is_active_slave(slave) &&
>                slave->dev->flags & IFF_MULTICAST;
>  }
> 
> > 
> > >Note: The call to bond_slave_ns_maddrs_del() in __bond_release_one() is
> > >kept, as it is still required to clean up multicast MAC addresses when
> > >a slave is removed.
> > >
> > >Fixes: 8eb36164d1a6 ("bonding: add ns target multicast address to slave device")
> > >Reported-by: Liang Li <liali@redhat.com>
> > >Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > >---
> > > drivers/net/bonding/bond_main.c | 9 ---------
> > > include/net/bonding.h           | 7 +++++++
> > > 2 files changed, 7 insertions(+), 9 deletions(-)
> > >
> > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > >index 8ea183da8d53..6dde6f870ee2 100644
> > >--- a/drivers/net/bonding/bond_main.c
> > >+++ b/drivers/net/bonding/bond_main.c
> > >@@ -1004,8 +1004,6 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
> > > 
> > > 		if (bond->dev->flags & IFF_UP)
> > > 			bond_hw_addr_flush(bond->dev, old_active->dev);
> > >-
> > >-		bond_slave_ns_maddrs_add(bond, old_active);
> > > 	}
> > > 
> > > 	if (new_active) {
> > >@@ -1022,8 +1020,6 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
> > > 			dev_mc_sync(new_active->dev, bond->dev);
> > > 			netif_addr_unlock_bh(bond->dev);
> > > 		}
> > >-
> > >-		bond_slave_ns_maddrs_del(bond, new_active);
> > > 	}
> > > }
> > > 
> > >@@ -2350,11 +2346,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> > > 	bond_compute_features(bond);
> > > 	bond_set_carrier(bond);
> > > 
> > >-	/* Needs to be called before bond_select_active_slave(), which will
> > >-	 * remove the maddrs if the slave is selected as active slave.
> > >-	 */
> > >-	bond_slave_ns_maddrs_add(bond, new_slave);
> > >-
> > > 	if (bond_uses_primary(bond)) {
> > > 		block_netpoll_tx();
> > > 		bond_select_active_slave(bond);
> > >diff --git a/include/net/bonding.h b/include/net/bonding.h
> > >index 95f67b308c19..0041f7a2bd18 100644
> > >--- a/include/net/bonding.h
> > >+++ b/include/net/bonding.h
> > >@@ -385,7 +385,14 @@ static inline void bond_set_slave_state(struct slave *slave,
> > > 	if (slave->backup == slave_state)
> > > 		return;
> > > 
> > >+	if (slave_state == BOND_STATE_ACTIVE)
> > >+		bond_slave_ns_maddrs_del(slave->bond, slave);
> > >+
> > > 	slave->backup = slave_state;
> > >+
> > >+	if (slave_state == BOND_STATE_BACKUP)
> > >+		bond_slave_ns_maddrs_add(slave->bond, slave);
> > 
> > 	This code pattern kind of makes it look like the slave->backup
> > assignment must happen between the two new if blocks.  I don't think
> > that's true, and things would work correctly if the slave->backup
> > assignment happened first (or last).
> 
> The slave->backup assignment must happen between the two if blocks, because
> 
> bond_slave_ns_maddrs_add/del only do the operation on backup slave.
> So if a interface become active, we need to call maddrs_del before it set
> backup state to active. If a interface become backup. We need to call
> maddrs_add after the backup state set to backup.
> 
> I will add a comment in the code.
> 
> Thanks
> Hangbin
> > 
> > 	Assuming I'm correct, could you move the assignment so it's not
> > in the middle?  If, however, it does need to be in the middle, that
> > deserves a comment explaining why.
> > 
> > 	-J
> > 
> > >+
> > > 	if (notify) {
> > > 		bond_lower_state_changed(slave);
> > > 		bond_queue_slave_event(slave);
> > >-- 
> > >2.46.0
> > >
> > 
> > ---
> > 	-Jay Vosburgh, jv@jvosburgh.net

Re: [PATCH net] bonding: fix multicast MAC address synchronization

[Hangbin Liu]

Hi Jay,

Any comments?

Thanks
Hangbin
On Mon, May 26, 2025 at 08:51:52AM +0000, Hangbin Liu wrote:
> On Fri, May 23, 2025 at 02:03:47PM -0700, Jay Vosburgh wrote:
> > Hangbin Liu <liuhangbin@gmail.com> wrote:
> > 
> > >There is a corner case where the NS (Neighbor Solicitation) target is set to
> > >an invalid or unreachable address. In such cases, all the slave links are
> > >marked as down and set to backup. This causes the bond to add multicast MAC
> > >addresses to all slaves.
> > >
> > >However, bond_ab_arp_probe() later tries to activate a carrier on slave and
> > >sets it as active. If we subsequently change or clear the NS targets, the
> > >call to bond_slave_ns_maddrs_del() on this interface will fail because it
> > >is still marked active, and the multicast MAC address will remain.
> > 
> > 	This seems complicated, so, just to make sure I'm clear, the bug
> > being fixed here happens when:
> > 
> > (a) ARP monitor is running with NS target(s), all of which do not
> > solicit a reply (invalid address or unreachable), resulting in all
> > interfaces in the bond being marked down
> > 
> > (b) while in the above state, the ARP monitor will cycle through each
> > interface, making them "active" (active-ish, really, just enough for the
> > ARP mon stuff to work) in turn to check for a response to a probe
> 
> Yes
> 
> > 
> > (c) while the cycling from (b) is occurring, attempts to change a NS
> > target will fail on the interface that happens to be quasi-"active" at
> > the moment.
> 
> Yes, this is because bond_slave_ns_maddrs_del() must ensure the deletion
> happens on a backup slave only. However, during ARP monitor, it set one of
> the slaves to active, this causes the deletion of multicast MAC addresses to
> be skipped on that interface.
> 
> > 	Is my summary correct?
> > 
> > 	Doesn't the failure scenario also require that arp_validate be
> > enabled?  Looking at bond_slave_ns_maddrs_{add,del}, they do nothing if
> > arp_validate is off.
> 
> Yes, it need.
> 
> > 
> > >To fix this issue, move the NS multicast address add/remove logic into
> > >bond_set_slave_state() to ensure multicast MAC addresses are updated
> > >synchronously whenever the slave state changes.
> > 
> > 	Ok, but state change calls happen in a lot more places than the
> > existing bond_hw_addr_swap(), which is only called during change of
> > active for active-backup, balance-alb, and balance-tlb.  Are you sure
> > that something goofy like setting arp_validate and an NS target with the
> > ARP monitor disabled (or in a mode that disallows it) will behave
> > rationally?
> 
> The slave_can_set_ns_maddr() in slave_set_ns_maddrs could check the bond mode
> and if the slave is active. But no arp_interval checking. I can add it in the
> checking to avoid the miss-config. e.g.
> 
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 91893c29b899..21116362cc24 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -1241,6 +1241,7 @@ static int bond_option_arp_ip_targets_set(struct bonding *bond,
>  static bool slave_can_set_ns_maddr(const struct bonding *bond, struct slave *slave)
>  {
>         return BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP &&
> +              bond->params.arp_interval &&
>                !bond_is_active_slave(slave) &&
>                slave->dev->flags & IFF_MULTICAST;
>  }
> 
> > 
> > >Note: The call to bond_slave_ns_maddrs_del() in __bond_release_one() is
> > >kept, as it is still required to clean up multicast MAC addresses when
> > >a slave is removed.
> > >
> > >Fixes: 8eb36164d1a6 ("bonding: add ns target multicast address to slave device")
> > >Reported-by: Liang Li <liali@redhat.com>
> > >Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > >---
> > > drivers/net/bonding/bond_main.c | 9 ---------
> > > include/net/bonding.h           | 7 +++++++
> > > 2 files changed, 7 insertions(+), 9 deletions(-)
> > >
> > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > >index 8ea183da8d53..6dde6f870ee2 100644
> > >--- a/drivers/net/bonding/bond_main.c
> > >+++ b/drivers/net/bonding/bond_main.c
> > >@@ -1004,8 +1004,6 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
> > > 
> > > 		if (bond->dev->flags & IFF_UP)
> > > 			bond_hw_addr_flush(bond->dev, old_active->dev);
> > >-
> > >-		bond_slave_ns_maddrs_add(bond, old_active);
> > > 	}
> > > 
> > > 	if (new_active) {
> > >@@ -1022,8 +1020,6 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
> > > 			dev_mc_sync(new_active->dev, bond->dev);
> > > 			netif_addr_unlock_bh(bond->dev);
> > > 		}
> > >-
> > >-		bond_slave_ns_maddrs_del(bond, new_active);
> > > 	}
> > > }
> > > 
> > >@@ -2350,11 +2346,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> > > 	bond_compute_features(bond);
> > > 	bond_set_carrier(bond);
> > > 
> > >-	/* Needs to be called before bond_select_active_slave(), which will
> > >-	 * remove the maddrs if the slave is selected as active slave.
> > >-	 */
> > >-	bond_slave_ns_maddrs_add(bond, new_slave);
> > >-
> > > 	if (bond_uses_primary(bond)) {
> > > 		block_netpoll_tx();
> > > 		bond_select_active_slave(bond);
> > >diff --git a/include/net/bonding.h b/include/net/bonding.h
> > >index 95f67b308c19..0041f7a2bd18 100644
> > >--- a/include/net/bonding.h
> > >+++ b/include/net/bonding.h
> > >@@ -385,7 +385,14 @@ static inline void bond_set_slave_state(struct slave *slave,
> > > 	if (slave->backup == slave_state)
> > > 		return;
> > > 
> > >+	if (slave_state == BOND_STATE_ACTIVE)
> > >+		bond_slave_ns_maddrs_del(slave->bond, slave);
> > >+
> > > 	slave->backup = slave_state;
> > >+
> > >+	if (slave_state == BOND_STATE_BACKUP)
> > >+		bond_slave_ns_maddrs_add(slave->bond, slave);
> > 
> > 	This code pattern kind of makes it look like the slave->backup
> > assignment must happen between the two new if blocks.  I don't think
> > that's true, and things would work correctly if the slave->backup
> > assignment happened first (or last).
> 
> The slave->backup assignment must happen between the two if blocks, because
> 
> bond_slave_ns_maddrs_add/del only do the operation on backup slave.
> So if a interface become active, we need to call maddrs_del before it set
> backup state to active. If a interface become backup. We need to call
> maddrs_add after the backup state set to backup.
> 
> I will add a comment in the code.
> 
> Thanks
> Hangbin
> > 
> > 	Assuming I'm correct, could you move the assignment so it's not
> > in the middle?  If, however, it does need to be in the middle, that
> > deserves a comment explaining why.
> > 
> > 	-J
> > 
> > >+
> > > 	if (notify) {
> > > 		bond_lower_state_changed(slave);
> > > 		bond_queue_slave_event(slave);
> > >-- 
> > >2.46.0
> > >
> > 
> > ---
> > 	-Jay Vosburgh, jv@jvosburgh.net