[PATCH net-next 1/2] bonding: 3ad: Receive packets in COLLECTING state

[PATCH net-next 1/2] bonding: 3ad: Receive packets in COLLECTING state

[Ido Schimmel]

When coupled control is disabled, a port in COLLECTING state is supposed
to receive packets, but it does not. The bond only delivers packets from
slaves whose 'inactive' flag is cleared, whereas
bond_set_slave_rx_enabled_flags() only clears 'rx_disabled'.

Fix by clearing 'inactive' as well.

Reviewed-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---

Notes:
    Targeting at net-next since this never worked.
    
    Known issues that this patch does not address:
    
    1. Changing 'all_slaves_active' from 1 to 0 while a port is in
       COLLECTING state sets its 'inactive' flag and the port stops
       receiving until the next mux state transition. This is not new,
       COLLECTING is a transient state and the option is off by default, so I
       do not think it is worth handling.
    
    2. The 'inactive' flag shares a bitfield with other slave flags. The
       802.3ad state machine writes them under the mode lock, while the
       'all_slaves_active' handler writes 'inactive' under RTNL only. This is
       not new.

 include/net/bonding.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/net/bonding.h b/include/net/bonding.h
index 598d56b1bc97..cb89aff437f4 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -594,6 +594,7 @@ static inline void bond_set_slave_active_flags(struct slave *slave,
 static inline void bond_set_slave_rx_enabled_flags(struct slave *slave,
 					       bool notify)
 {
+	slave->inactive = 0;
 	slave->rx_disabled = 0;
 }
 
-- 
2.56.0

[PATCH net-next 2/2] selftests: bonding: Add a test for 802.3ad with coupled control disabled

[Ido Schimmel]

Test that with coupled control disabled, a port in COLLECTING state
receives packets.

In this mode, a port only transitions from COLLECTING to DISTRIBUTING
state if its partner sets the Collecting bit in its LACPDUs. Therefore,
keep the port in COLLECTING state by clearing the Collecting bit in
received LACPDUs.

Also test that ping works once the port moves to DISTRIBUTING state.

Use VRFs rather than network namespaces so that the test could be run
with a veth pair or a pair of physically connected ports.

Output without "bonding: 3ad: Receive packets in COLLECTING state":

 # ./bond_lacp_independent_control.sh
 TEST: Receive in COLLECTING state                                   [FAIL]
         Packets not received in COLLECTING state
 TEST: Ping in DISTRIBUTING state                                    [ OK ]

Output with the fix:

 # ./bond_lacp_independent_control.sh
 TEST: Receive in COLLECTING state                                   [ OK ]
 TEST: Ping in DISTRIBUTING state                                    [ OK ]

Reviewed-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../selftests/drivers/net/bonding/Makefile    |   1 +
 .../bonding/bond_lacp_independent_control.sh  | 144 ++++++++++++++++++
 .../selftests/drivers/net/bonding/config      |   6 +
 3 files changed, 151 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/bonding/bond_lacp_independent_control.sh

diff --git a/tools/testing/selftests/drivers/net/bonding/Makefile b/tools/testing/selftests/drivers/net/bonding/Makefile
index e337c099fb40..46d9fede5918 100644
--- a/tools/testing/selftests/drivers/net/bonding/Makefile
+++ b/tools/testing/selftests/drivers/net/bonding/Makefile
@@ -7,6 +7,7 @@ TEST_PROGS := \
 	bond-eth-type-change.sh \
 	bond-lladdr-target.sh \
 	bond_ipsec_offload.sh \
+	bond_lacp_independent_control.sh \
 	bond_lacp_prio.sh \
 	bond_lacp_strict.sh \
 	bond_macvlan_ipvlan.sh \
diff --git a/tools/testing/selftests/drivers/net/bonding/bond_lacp_independent_control.sh b/tools/testing/selftests/drivers/net/bonding/bond_lacp_independent_control.sh
new file mode 100755
index 000000000000..c29712e01e49
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/bonding/bond_lacp_independent_control.sh
@@ -0,0 +1,144 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# +-------------------------+ +-------------------------+
+# | H1 (vrf)                | | H2 (vrf)                |
+# |  + bond1 (802.3ad)      | |  + bond2 (802.3ad)      |
+# |  | coupled_control off  | |  |                      |
+# |  | 192.0.2.1/28         | |  | 192.0.2.2/28         |
+# |  |                      | |  |                      |
+# |  + $h1                  | |  + $h2                  |
+# +--|----------------------+ +--|----------------------+
+#    |                           |
+#    +---------------------------+
+
+# shellcheck disable=SC2034,SC2329
+ALL_TESTS="
+	collecting
+	distributing
+"
+NUM_NETIFS=2
+REQUIRE_MZ=no
+lib_dir=$(dirname "$0")
+# shellcheck disable=SC1091
+source "$lib_dir"/../../../net/forwarding/lib.sh
+
+lacp_state_get()
+{
+	local dev=$1; shift
+
+	ip -d -j link show dev "$dev" | \
+		jq '.[].linkinfo.info_slave_data.ad_actor_oper_port_state'
+}
+
+# The actor state of a port in ATTACHED state is 0x0f: Activity (0x01),
+# Timeout (0x02), Aggregation (0x04) and Synchronization (0x08). The
+# Collecting (0x10) bit is added in COLLECTING state (0x1f) and the
+# Distributing (0x20) bit in DISTRIBUTING state (0x3f).
+lacp_state_wait()
+{
+	local dev=$1; shift
+	local state=$1; shift
+
+	slowwait "$WAIT_TIMEOUT" until_counter_is "== $state" \
+		lacp_state_get "$dev" > /dev/null
+}
+
+partner_state_clear()
+{
+	local bits=$1; shift
+
+	tc filter replace dev "$h1" ingress pref 1 handle 101 protocol 0x8809 \
+		flower skip_hw action pedit munge offset 18 u8 set 0 \
+		retain "$bits"
+}
+
+h1_create()
+{
+	adf_ip_link_add bond1 type bond mode 802.3ad lacp_rate fast \
+		coupled_control off
+	adf_ip_link_set_master "$h1" bond1
+	adf_simple_if_init bond1 192.0.2.1/28
+
+	tc qdisc add dev "$h1" clsact
+	defer tc qdisc del dev "$h1" clsact
+
+	tc qdisc add dev bond1 clsact
+	defer tc qdisc del dev bond1 clsact
+
+	tc filter add dev bond1 ingress pref 1 handle 101 proto ip \
+		flower skip_hw src_ip 192.0.2.2 dst_ip 192.0.2.1 \
+		ip_proto icmp type 8 action pass
+	defer tc filter del dev bond1 ingress pref 1 handle 101 flower
+}
+
+h2_create()
+{
+	adf_ip_link_add bond2 type bond mode 802.3ad lacp_rate fast
+	adf_ip_link_set_master "$h2" bond2
+	adf_simple_if_init bond2 192.0.2.2/28
+}
+
+setup_prepare()
+{
+	h1=${NETIFS[p1]}
+	h2=${NETIFS[p2]}
+
+	adf_vrf_prepare
+
+	h1_create
+	h2_create
+}
+
+collecting()
+{
+	local pkts
+
+	RET=0
+
+	# Clear the Synchronization (0x08) and Collecting (0x10) bits in the
+	# partner's state to move $h1 to ATTACHED state and then only clear
+	# the Collecting bit to move it to COLLECTING state.
+	partner_state_clear 0x18
+	defer tc filter del dev "$h1" ingress pref 1 handle 101 flower
+	lacp_state_wait "$h1" 0x0f
+	check_err $? "Port did not reach ATTACHED state"
+
+	partner_state_clear 0x10
+	lacp_state_wait "$h1" 0x1f
+	check_err $? "Port did not reach COLLECTING state"
+
+	# $h1 cannot reply to ARP requests in COLLECTING state.
+	ip neigh add 192.0.2.1 lladdr "$(mac_get bond1)" dev bond2 \
+		nud permanent
+	defer ip neigh del 192.0.2.1 dev bond2
+
+	ping_do bond2 192.0.2.1
+	pkts=$(tc_rule_handle_stats_get "dev bond1 ingress" 101)
+	((pkts >= PING_COUNT))
+	check_err $? "Packets not received in COLLECTING state"
+
+	log_test "Receive in COLLECTING state"
+}
+
+distributing()
+{
+	RET=0
+
+	lacp_state_wait "$h1" 0x3f
+	check_err $? "Port did not reach DISTRIBUTING state"
+
+	ping_do bond2 192.0.2.1
+	check_err $? "Ping failed"
+
+	log_test "Ping in DISTRIBUTING state"
+}
+
+trap cleanup EXIT
+
+setup_prepare
+setup_wait
+
+tests_run
+
+exit "$EXIT_STATUS"
diff --git a/tools/testing/selftests/drivers/net/bonding/config b/tools/testing/selftests/drivers/net/bonding/config
index b62c70715293..71c6d69d98a1 100644
--- a/tools/testing/selftests/drivers/net/bonding/config
+++ b/tools/testing/selftests/drivers/net/bonding/config
@@ -1,13 +1,17 @@
 CONFIG_BONDING=y
+CONFIG_BPF_SYSCALL=y
 CONFIG_BRIDGE=y
+CONFIG_CGROUP_BPF=y
 CONFIG_CONFIGFS_FS=y
 CONFIG_DUMMY=y
 CONFIG_INET_ESP=y
 CONFIG_INET_ESP_OFFLOAD=y
 CONFIG_IPV6=y
+CONFIG_IPV6_MULTIPLE_TABLES=y
 CONFIG_IPVLAN=y
 CONFIG_MACVLAN=y
 CONFIG_NET_ACT_GACT=y
+CONFIG_NET_ACT_PEDIT=m
 CONFIG_NET_CLS_FLOWER=y
 CONFIG_NET_CLS_MATCHALL=m
 CONFIG_NETCONSOLE=m
@@ -15,7 +19,9 @@ CONFIG_NETCONSOLE_DYNAMIC=y
 CONFIG_NETCONSOLE_EXTENDED_LOG=y
 CONFIG_NETDEVSIM=m
 CONFIG_NET_IPGRE=y
+CONFIG_NET_L3_MASTER_DEV=y
 CONFIG_NET_SCH_INGRESS=y
+CONFIG_NET_VRF=y
 CONFIG_NLMON=y
 CONFIG_VETH=y
 CONFIG_VLAN_8021Q=m
-- 
2.56.0

Re: [PATCH net-next 2/2] selftests: bonding: Add a test for 802.3ad with coupled control disabled

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The bonding selftest config fragment
  (tools/testing/selftests/drivers/net/bonding/config) now asks for…
- [Low] In bond_lacp_independent_control.sh, collecting() reads the total
  packet count of the bond1 ingress ICMP echo-request filter (handle…

--

selftests: bonding: Add a test for 802.3ad with coupled control disabled

This adds a VRF based selftest which checks that a bond port with
coupled_control off receives packets in COLLECTING state. It keeps the
port in COLLECTING state by clearing the Collecting bit in received
LACPDUs, and then checks that ping works in DISTRIBUTING state. The
bonding selftest config fragment gains the VRF, pedit and policy routing
options the test needs.

> diff --git a/tools/testing/selftests/drivers/net/bonding/bond_lacp_independent_control.sh b/tools/testing/selftests/drivers/net/bonding/bond_lacp_independent_control.sh
> new file mode 100755
> index 0000000000000..c29712e01e49e
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/bonding/bond_lacp_independent_control.sh

[ ... ]

> +h1_create()
> +{
> +	adf_ip_link_add bond1 type bond mode 802.3ad lacp_rate fast \
> +		coupled_control off
> +	adf_ip_link_set_master "$h1" bond1
> +	adf_simple_if_init bond1 192.0.2.1/28
> +
> +	tc qdisc add dev "$h1" clsact
> +	defer tc qdisc del dev "$h1" clsact
> +
> +	tc qdisc add dev bond1 clsact
> +	defer tc qdisc del dev bond1 clsact
> +
> +	tc filter add dev bond1 ingress pref 1 handle 101 proto ip \
> +		flower skip_hw src_ip 192.0.2.2 dst_ip 192.0.2.1 \
> +		ip_proto icmp type 8 action pass
> +	defer tc filter del dev bond1 ingress pref 1 handle 101 flower
> +}

[ ... ]

> +collecting()
> +{

[ ... ]

> +	ping_do bond2 192.0.2.1
> +	pkts=$(tc_rule_handle_stats_get "dev bond1 ingress" 101)
> +	((pkts >= PING_COUNT))
> +	check_err $? "Packets not received in COLLECTING state"

[Severity: Low]
Can this check pass when no packets were received in COLLECTING state?

The bond1 ingress filter with handle 101 is installed once by
h1_create() from setup_prepare(), outside the per-test defer scopes, so
its counter keeps growing across tests. collecting() compares the
absolute counter with PING_COUNT and does not take a baseline before
ping_do.

tests_run() in net/lib.sh iterates over ${TESTS:-$ALL_TESTS}. Take
TESTS="distributing collecting" as an example:

distributing()
    ping_do bond2 192.0.2.1    <- PING_COUNT echo requests hit filter 101
collecting()
    port moved to COLLECTING state
    ping_do bond2 192.0.2.1    <- nothing received without the fix
    pkts >= PING_COUNT         <- still true from the earlier test

The same thing happens if collecting is listed twice. The default
ALL_TESTS order runs collecting first, so the default run is not
affected.

Since the ping_do return value is ignored here, this counter is the only
check of the fix. Would it make sense to read the counter before ping_do
and check the delta, or to create the filter inside collecting() with a
matching defer?

> +
> +	log_test "Receive in COLLECTING state"
> +}

[ ... ]

> diff --git a/tools/testing/selftests/drivers/net/bonding/config b/tools/testing/selftests/drivers/net/bonding/config
> index b62c707152930..71c6d69d98a18 100644
> --- a/tools/testing/selftests/drivers/net/bonding/config
> +++ b/tools/testing/selftests/drivers/net/bonding/config

[ ... ]

> @@ -15,7 +19,9 @@ CONFIG_NETCONSOLE_DYNAMIC=y
>  CONFIG_NETCONSOLE_EXTENDED_LOG=y
>  CONFIG_NETDEVSIM=m
>  CONFIG_NET_IPGRE=y
> +CONFIG_NET_L3_MASTER_DEV=y
>  CONFIG_NET_SCH_INGRESS=y
> +CONFIG_NET_VRF=y

[Severity: Low]
Should this fragment also enable CONFIG_IP_ADVANCED_ROUTER=y and
CONFIG_IP_MULTIPLE_TABLES=y?

NET_VRF depends on IP_MULTIPLE_TABLES in drivers/net/Kconfig:

config NET_VRF
	tristate "Virtual Routing and Forwarding (Lite)"
	depends on IP_MULTIPLE_TABLES
	depends on NET_L3_MASTER_DEV
	depends on IPV6_MULTIPLE_TABLES || IPV6=n

and in net/ipv4/Kconfig, IP_MULTIPLE_TABLES depends on IP_ADVANCED_ROUTER
and has no default:

config IP_MULTIPLE_TABLES
	bool "IP: policy routing"
	depends on IP_ADVANCED_ROUTER

The patch adds the NET_L3_MASTER_DEV and IPV6_MULTIPLE_TABLES
dependencies but not the IPv4 ones. If the fragment is merged onto a base
config that lacks them, Kconfig silently drops NET_VRF. arm64 defconfig
is one example: it sets neither symbol, while x86_64_defconfig sets both.

In that case setup_prepare()->adf_vrf_prepare()->vrf_prepare() runs:

	ip -4 rule add pref 32765 table local

which needs IPv4 policy routing, and adf_simple_if_init() cannot create
the VRF devices. The test then fails during setup and never exercises the
bonding change.

Several other net selftest fragments have the same gap, while
net/mptcp/config lists IP_MULTIPLE_TABLES explicitly.

>  CONFIG_NLMON=y
>  CONFIG_VETH=y
>  CONFIG_VLAN_8021Q=m

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111171947.2816258-1-idosch%40nvidia.com

Re: [PATCH net-next 1/2] bonding: 3ad: Receive packets in COLLECTING state

[netdev-bot+sashiko]

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 fix clears 'inactive' only when ad_enable_collecting()
  runs.
- [Low] The patch fixes a real, user-visible bug but has no Fixes: tag.

--

bonding: 3ad: Receive packets in COLLECTING state

With coupled control disabled, an 802.3ad port in COLLECTING state should
receive packets, but the RX path only delivers from slaves whose inactive
flag is cleared. This patch makes bond_set_slave_rx_enabled_flags() clear
slave->inactive in addition to slave->rx_disabled.

> When coupled control is disabled, a port in COLLECTING state is supposed
> to receive packets, but it does not. The bond only delivers packets from
> slaves whose 'inactive' flag is cleared, whereas
> bond_set_slave_rx_enabled_flags() only clears 'rx_disabled'.
>
> Fix by clearing 'inactive' as well.

[Severity: Low]
Should this carry a Fixes: tag?

bond_set_slave_rx_enabled_flags() was added by commit 240fd405528b
("bonding: Add independent control state machine"), and that commit only
cleared rx_disabled. The RX gate in bond_should_deliver_exact_match()
checks only bond_is_slave_inactive(), so the bug goes back to that commit.

The notes say this targets net-next because it never worked, and that
makes sense. A tag would still let stable and distro maintainers link the
fix to the original commit:

Fixes: 240fd405528b ("bonding: Add independent control state machine")

> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 598d56b1bc970..cb89aff437f44 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -594,6 +594,7 @@ static inline void bond_set_slave_active_flags(struct slave *slave,
>  static inline void bond_set_slave_rx_enabled_flags(struct slave *slave,
>  					       bool notify)
>  {
> +	slave->inactive = 0;
>  	slave->rx_disabled = 0;
>  }

[Severity: Medium]
With this change, inactive is only cleared when ad_enable_collecting()
runs. Can a COLLECTING port still be left with inactive=1 and
rx_disabled=0?

bond_option_all_slaves_active_set() can still set inactive without
touching rx_disabled when all_slaves_active goes from 1 to 0:

drivers/net/bonding/bond_options.c:bond_option_all_slaves_active_set() {
    ...
	bond_for_each_slave(bond, slave, iter) {
		if (!bond_is_active_slave(slave)) {
			if (newval->value)
				slave->inactive = 0;
			else
				slave->inactive = 1;
		}
	}
    ...
}

A COLLECTING port matches !bond_is_active_slave(). This is because
ad_disable_distributing()->__disable_distributing_port()->
bond_set_slave_tx_disabled_flags() left it in BOND_STATE_BACKUP.

The steady-state COLLECTING recheck in ad_mux_machine() only looks at
rx_disabled:

drivers/net/bonding/bond_3ad.c:ad_mux_machine() {
    ...
					if (aggregator->is_active &&
					    bond_is_slave_rx_disabled(slave)) {
						ad_enable_collecting(port);
						*update_slave_arr = true;
					}
    ...
}

Since rx_disabled stays 0, the recheck never fires. Until the next mux
transition, bond_should_deliver_exact_match() keeps returning true, and
every non-LACPDU frame on that port gets RX_HANDLER_EXACT.

The notes list this as known issue 1 and describe COLLECTING as
transient. However, the companion selftest ("selftests: bonding: Add a
test for 802.3ad with coupled control disabled") keeps a port in
COLLECTING indefinitely by clearing the partner's Collecting bit. So a
partner that never sets LACP_STATE_COLLECTING would keep the port
dropping traffic.

Would it work to also check bond_is_slave_inactive(slave) in the recheck,
for example:

    bond_is_slave_rx_disabled(slave) || bond_is_slave_inactive(slave)

That would let the port recover on the next state machine tick.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111171947.2816258-1-idosch%40nvidia.com