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

[netdev-bot+sashiko@kernel.org]

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