Re: [PATCH net v2] bonding: fix initial last_rx vs ARP-monitor slack window

[Ramses]

Aug 28, 2026, 04:50 by hangbin.liu@xxxxxxxxx:

> On Thu, Aug 27, 2026 at 01:44:42PM +0200, Ramses de Norre via B4 Relay wrote:
>
>> From: Ramses de Norre <ramses@xxxxxxxxxxxxxxxx>
>>
>> Commit f31c7937c254 ("bonding: start slaves with link down for ARP
>> monitor") initialises a freshly enslaved port's last_rx to
>> jiffies - (arp_interval + 1) so that it does not "immediately cause
>> fake detection of 'up' state". At the time, the comparison was a plain
>> <= arp_interval and the value was just stale enough.
>>
>> Commit da210f559019 ("bonding: add some slack to arp monitoring time
>> limits"), four months later, added a +arp_interval/2 slack term to
>> every comparison (now bond_time_in_interval()) but did not widen the
>> init to match. Since then, bond_time_in_interval(bond, last_rx, 1) is
>> true for the first ~arp_interval/2 after enslavement even though no
>> packet has been received: the upper bound is last_rx + 1.5*delta and
>> last_rx was set to jiffies - delta - 1.
>>
>> If the ARP monitor tick lands in that window, bond_ab_arp_inspect()
>> proposes the slave UP. If the slave is the configured primary,
>> bond_ab_arp_commit() sets do_failover and the still-armed
>> force_primary in bond_choose_primary_or_current() makes it the active
>> slave regardless of primary_reselect. ARP validation as the active
>> slave then fails (the link has not actually received anything; on
>> SFP+ ports the PHY is often still negotiating) and the bond falls back
>> to the backup. With primary_reselect=failure, force_primary has now
>> been spent and the bond stays on the backup until something else
>> triggers a reselect.
>>
>
> Can we set primary_reselect to always or better to avoid this? If you prefer
> to using the primary slave.
>
That hides the symptom, but the wrong link state happens nonetheless, in bond_ab_arp_inspect(), independent of any selection policy:

if (slave->link != BOND_LINK_UP) {
  if (bond_time_in_interval(bond, last_rx, 1)) {
    bond_propose_link_state(slave, BOND_LINK_UP);

bond_enslave() needs to seed last_rx with a timestamp old enough that this test fails until a real ARP reply arrives. That is what the seed is for, see f31c7937c254 ("bonding: start slaves with link down for ARP monitor").

The seed is one arp_interval old, which was just old enough back when the test was "jiffies <= last_rx + delta". da210f559019 then added delta/2 of slack to every such comparison without adjusting the seed accordingly, so the seed now sits inside the window instead of outside it. So for the first half ARP interval after enslavement the test indicates "received recently" for a slave that hasn't actually received anything, and that is true for any freshly enslaved slave, primary or not, whatever primary_reselect is set to.

With the always policy the bond does recover once the port really comes up, so it is not permanent, but until then traffic goes to a port whose PHY has not finished negotiating and is dropped.
So in my case, I use the failure policy precisely to avoid the bond flapping back to the primary while it is not actually up, which would cause dropped packets, but it would be much nicer if the link state was just detected correctly and this workaround wasn't needed.

>>
>> Reproducer:
>>
>> ip link add bond0 type bond mode active-backup arp_interval 1000 \
>> arp_validate all arp_ip_target 192.0.2.1 \
>> primary eth0 primary_reselect failure
>> # eth0: SFP+ (slow link-up), eth1: RJ45 (fast link-up)
>> ip link set eth0 master bond0
>> ip link set eth1 master bond0
>> ip link set bond0 up
>> # bond0 lands on eth0 via force_primary, ARP-fails it before the
>> # SFP+ has carrier, falls to eth1, and stays there.
>>
>> Initialise last_rx (and the per-target array, and last_tx) to two full
>> intervals in the past so it is outside the slack window from the
>> start.
>>
>
> Is this trying to init the backup slave down by default?
>
No, the link state init is untouched, it still comes from netif_carrier_ok() a few lines further down. Only the last_rx seed changes, from one interval to two, so that it is outside the window bond_time_in_interval() checks.

>
> Thanks
> Hangbin
>