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

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

[Ramses de Norre via B4 Relay]

From: Ramses de Norre <ramses@well-founded.dev>

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.

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.

Fixes: da210f559019 ("bonding: add some slack to arp monitoring time limits")
Signed-off-by: Ramses de Norre <ramses@well-founded.dev>
---
Changes in v2:
- No code changes. Resend with my personal name in the From and
  Signed-off-by lines instead of my handle, as requested for the DCO.
- Link to v1: https://patch.msgid.link/20260817-bonding-last-rx-v1-1-9da6f7fdf812@well-founded.dev
---
 drivers/net/bonding/bond_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 522eab060f9ed..23e1544faf39f 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2123,7 +2123,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 		new_slave->link = BOND_LINK_DOWN;
 
 	new_slave->last_rx = jiffies -
-		(msecs_to_jiffies(bond->params.arp_interval) + 1);
+		(2 * msecs_to_jiffies(bond->params.arp_interval) + 1);
 	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++)
 		new_slave->target_last_arp_rx[i] = new_slave->last_rx;
 

---
base-commit: 24ef02f934eeb48830cff6b739abc3c62b1d107b
change-id: 20260817-bonding-last-rx-0303fa048f9d

Best regards,
--  
Ramses de Norre <ramses@well-founded.dev>

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

[Hangbin Liu]

On Thu, Aug 27, 2026 at 01:44:42PM +0200, Ramses de Norre via B4 Relay wrote:
> From: Ramses de Norre <ramses@well-founded.dev>
> 
> 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.

> 
> 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?

Thanks
Hangbin

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

[Ramses]

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

> On Thu, Aug 27, 2026 at 01:44:42PM +0200, Ramses de Norre via B4 Relay wrote:
>
>> From: Ramses de Norre <ramses@well-founded.dev>
>>
>> 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
>

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

[Hangbin Liu]

On Sun, Sep 06, 2026 at 11:02:37AM +0200, Ramses wrote:
> Aug 28, 2026, 04:50 by hangbin.liu@linux.dev:
> 
> > On Thu, Aug 27, 2026 at 01:44:42PM +0200, Ramses de Norre via B4 Relay wrote:
> >
> >> From: Ramses de Norre <ramses@well-founded.dev>
> >>
> >> 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.

Ah, I got what you mean. The primary slave is set "UP" but no arp reply yet,
which cause a false UP and then goes down. Your fix is similar with
f31c7937c254 ("bonding: start slaves with link down for ARP monitor"), the
difference is your version is based on the extra arp_interval/2 delta.

The code looks good to me.

Reviewed-by: Hangbin Liu <liuhangbin@kylinos.cn>

But it would be good if someone else also could help review in case I missed
anything.

Thanks
Hangbin