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

[Hangbin Liu <liuhangbin@gmail.com>]

On Tue, Sep 09, 2025 at 03:42:30AM +0000, Hangbin Liu wrote:
> Hi Jay,
> On Wed, Aug 13, 2025 at 04:04:54AM +0000, Hangbin Liu wrote:
> > On Tue, Aug 12, 2025 at 10:42:22AM +0200, Paolo Abeni wrote:
> > > On 8/5/25 10:09 AM, Hangbin Liu 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. The ARP monitor then cycles through each slave to
> > > > probe them, temporarily marking as *active*.
> > > > 
> > > > Later, if the NS target is changed or cleared during this probe cycle, the
> > > > *active* slave will fail to remove its NS multicast address because
> > > > bond_slave_ns_maddrs_del() only removes addresses from backup slaves.
> > > > This leaves stale multicast MACs on the interface.
> > > > 
> > > > To fix this, we move the NS multicast MAC address handling into
> > > > bond_set_slave_state(), so every slave state transition consistently
> > > > adds/removes NS multicast addresses as needed.
> > > > 
> > > > We also ensure this logic is only active when arp_interval is configured,
> > > > to prevent misconfiguration or accidental behavior in unsupported modes.
> > > 
> > > As noted by Jay in the previous revision, moving the handling into
> > > bond_set_slave_state() could possibly impact a lot of scenarios, and
> > > it's not obvious to me that restricting to arp_interval != 0 would be
> > > sufficient.
> > 
> > I understand your concern. The bond_set_slave_state() function is called by:
> >   - bond_set_slave_inactive_flags
> >   - bond_set_slave_tx_disabled_flags
> >   - bond_set_slave_active_flags
> > 
> > These functions are mainly invoked via bond_change_active_slave, bond_enslave,
> > bond_ab_arp_commit, and bond_miimon_commit.
> > 
> > To avoid misconfiguration, in slave_can_set_ns_maddr() I tried to limit
> > changes to the backup slave when operating in active-backup mode with
> > arp_interval enabled. I also ensured that the multicast address is only
> > modified when the NS target is set.
> > 
> > > 
> > > I'm wondering if the issue could/should instead addressed explicitly
> > > handling the mac swap for the active slave at NS target change time. WDYT?
> > 
> > The problem is that bond_hw_addr_swap() is only called in bond_ab_arp_commit()
> > during ARP monitoring, while the bond sets active/inactive flags in
> > bond_ab_arp_probe(). These operations are called partially.
> > 
> > bond_activebackup_arp_mon
> >  - bond_ab_arp_commit
> >    - bond_select_active_slave
> >      - bond_change_active_slave
> >        - bond_hw_addr_swap
> >  - bond_ab_arp_probe
> >    - bond_set_slave_{active/inactive}_flags
> > 
> > On the other hand, we need to set the multicast address on the *temporary*
> > active interface to ensure we can receive the replied NA message. The MAC
> > swap only happens when the *actual* active interface is chosen.
> > 
> > This is why I chose to place the multicast address configuration in
> > bond_set_slave_state().
> 
> Do you have any comments?

Hi Jay,

Could you add some comments?

Thanks
Hangbin