Re: [PATCH net v4 1/2] bonding: fix initial last_rx vs ARP-monitor slack window

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new DOWN-slave seed in bond_enslave()
  (drivers/net/bonding/bond_main.c:2160-2162, `last_rx = jiffies - (2 *…
- [Low] The new comment in bond_enslave() (bond_main.c:2156-2159) says
  the DOWN seed keeps the slave DOWN 'until a real reply arrives'.

--

bonding: fix initial last_rx vs ARP-monitor slack window

This patch moves the last_rx, target_last_arp_rx[] and last_tx seeding in
bond_enslave() so it runs after the initial link state is set. A slave
that starts DOWN is now seeded two arp_intervals in the past, which puts
it outside the arp_interval/2 slack that bond_time_in_interval() adds.
Slaves that start UP keep the old seed.

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 7019e5d8fdc8e..bfa4cb97e647c 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c

[ ... ]

> @@ -2159,6 +2152,22 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  
>  	if (new_slave->link != BOND_LINK_DOWN)
>  		new_slave->last_link_up = jiffies;
> +
> +	/* The ARP monitor's up test includes arp_interval/2 of slack, see
> +	 * bond_time_in_interval(). Seed a slave that starts DOWN outside
> +	 * that window, so it stays DOWN until a real reply arrives.
> +	 */

[Severity: Low]
This isn't a bug, but is "until a real reply arrives" accurate? More than
ARP replies can move last_rx forward.

With arp_validate=none (the default), bond_rcv_validate() updates last_rx
for any received frame:

drivers/net/bonding/bond_main.c:bond_rcv_validate() {
    ...
	if (!slave_do_arp_validate(bond, slave)) {
		if ((slave_do_arp_validate_only(bond) && is_arp) ||
    ...
		    !slave_do_arp_validate_only(bond))
			WRITE_ONCE(slave->last_rx, jiffies);
    ...
}

With backup validation, bond_arp_rcv() also accepts broadcast ARP requests
on backup slaves, with sip and tip swapped:

drivers/net/bonding/bond_main.c:bond_arp_rcv() {
    ...
	else if (curr_active_slave &&
		 time_after(slave_last_rx(bond, curr_active_slave),
			    curr_active_slave->last_link_up))
		bond_validate_arp(bond, slave, tip, sip);
    ...
}

Would wording like "until it receives qualifying traffic" fit better? The
commit message makes the same claim and may need the same change.

> +	if (new_slave->link == BOND_LINK_DOWN)
> +		new_slave->last_rx = jiffies -
> +			(2 * msecs_to_jiffies(bond->params.arp_interval) + 1);

[Severity: Medium]
This seed uses the arp_interval in effect at enslave time. What happens if
ARP monitoring is turned on later, or the interval is raised?

Here are two ways the seed can end up inside the up window.

If the slave is enslaved under miimon (arp_interval == 0) with no carrier,
it starts BOND_LINK_DOWN and the seed is just jiffies - 1.

If the slave is enslaved with, say, arp_interval=100 and the interval is
then raised to 1000, the seed (t0 - (2 * d_old + 1)) still falls inside
the window that bond_time_in_interval() builds from the new interval:

drivers/net/bonding/bond_main.c:bond_time_in_interval() {
	int delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);

	return time_in_range(jiffies,
			     last_act - delta_in_ticks,
			     last_act + mod * delta_in_ticks + delta_in_ticks/2);
}

In both cases bond_option_arp_interval_set() writes the new interval and
starts arp_work right away. It does not reseed last_rx,
target_last_arp_rx[] or last_tx on existing slaves:

drivers/net/bonding/bond_options.c:bond_option_arp_interval_set() {
    ...
			WRITE_ONCE(bond->recv_probe, bond_rcv_validate);
			cancel_delayed_work_sync(&bond->mii_work);
			queue_delayed_work(bond->wq, &bond->arp_work, 0);
    ...
}

The path then runs bond_activebackup_arp_mon() -> bond_ab_arp_inspect(),
and bond_time_in_interval(bond, last_rx, 1) returns true. That proposes
BOND_LINK_UP for a slave with no carrier that has received nothing.

If that slave is the configured primary, force_primary may still be
pending. Under miimon the primary was DOWN, so
bond_choose_primary_or_current() never used up force_primary. The primary
then becomes active, fails ARP validation, and with
primary_reselect=failure the bond stays on the backup. That is the same
result as the reproducer in the commit message.

arp_interval has no BOND_OPTFLAG_NOSLAVES, and bond_open() does not reseed
either. The follow-up patch in this series ("bonding: give a slave that
starts UP a full arp_missed_max window") only changes the UP branch.

Should bond_option_arp_interval_set() also reseed slaves that are DOWN?

> +	else
> +		new_slave->last_rx = jiffies -
> +			(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;
> +
> +	new_slave->last_tx = new_slave->last_rx;
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028-bonding-last-rx-v4-0-4d8e4cd47467%40well-founded.dev