Re: [PATCH net v2 2/2] selftests/net/openvswitch: check that a failed set keeps the features

[netdev-bot+sashiko@kernel.org]

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

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

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] In test_dp_features
  (tools/testing/selftests/net/openvswitch/openvswitch.sh), the…
- [Low] The patch adds CONFIG_NET_CLS_ACT=y and CONFIG_NET_TC_SKB_EXT=y
  to tools/testing/selftests/net/openvswitch/config but leaves out…
- [Low] selftests/net/openvswitch: unsorted CONFIG_NET_TC_SKB_EXT in
  config

--

selftests/net/openvswitch: check that a failed set keeps the features

This adds a set-dp command to ovs-dpctl.py and a new dp_features test to
openvswitch.sh. The test checks that a rejected OVS_DP_CMD_SET leaves the
datapath user features unchanged. It also adds CONFIG_NET_CLS_ACT and
CONFIG_NET_TC_SKB_EXT to the selftest config fragment so the recirc round
can run.

> diff --git a/tools/testing/selftests/net/openvswitch/config b/tools/testing/selftests/net/openvswitch/config
> index 15685d2428139..d93eac0f08f7b 100644
> --- a/tools/testing/selftests/net/openvswitch/config
> +++ b/tools/testing/selftests/net/openvswitch/config
> @@ -2,6 +2,8 @@ CONFIG_GENEVE=m
>  CONFIG_INET_DIAG=y
>  CONFIG_IP_SCTP=y
>  CONFIG_IPV6=y
> +CONFIG_NET_CLS_ACT=y
> +CONFIG_NET_TC_SKB_EXT=y
>  CONFIG_NETFILTER=y
>  CONFIG_NETFILTER_ADVANCED=y
>  CONFIG_NET_IPGRE=m

[Severity: Low]
This isn't in the right place in the sort order.

The fragment is kept sorted with underscores ignored, which is the
order the netdev selftest config checks expect. In that order
CONFIG_NET_CLS_ACT fits where you put it, before CONFIG_NETFILTER.

CONFIG_NET_TC_SKB_EXT does not. Compared without underscores,
NETTCSKBEXT sorts after NETFILTER, NETFILTERADVANCED, NETIPGRE and
NETIPGREDEMUX. It should go after CONFIG_NET_IPGRE_DEMUX=m and before
the CONFIG_NF_* entries, which is also where
selftests/net/forwarding/config has it.

Putting it straight after CONFIG_NET_CLS_ACT=y breaks the ordering
for the rest of the file. Could it be moved down in the next version?

[Severity: Low]
Should CONFIG_NET_SCHED=y be added to the fragment too?

Both new symbols sit inside the if NET_SCHED block in net/sched/Kconfig.
Nothing in the tree selects NET_SCHED, and OPENVSWITCH doesn't either:

net/sched/Kconfig:
    if NET_SCHED
    ...
    config NET_CLS_ACT
    	bool "Actions"
    ...
    config NET_TC_SKB_EXT
    	bool "TC recirculation support"
    	depends on NET_CLS_ACT
    ...
    endif # NET_SCHED

When the fragment is merged onto a base without NET_SCHED (for example
tinyconfig or allnoconfig plus the fragment), merge_config drops both
symbols. ovs_dp_change() then still returns -EOPNOTSUPP for
OVS_DP_F_TC_RECIRC_SHARING, and test_dp_features skips the recirc round.
That is the case the commit message says this change fixes.

hid/config.common and tc-testing/config both list CONFIG_NET_SCHED=y next
to these symbols.

Also, the commit message says:

    Select the key and the classful action support it depends on

The Kconfig prompt for NET_CLS_ACT is "Actions" (tc actions), not
classful actions. Could the wording be adjusted?

> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index 21444657a0404..e2aba1e40d559 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh

[ ... ]

> @@ -1409,6 +1410,72 @@ test_netlink_checks () {
>  	return 0
>  }
>  
> +test_dp_features() {

[ ... ]

> +	sbx_add "test_dp_features" || return $?
> +	ovs_add_dp "test_dp_features" dpfeat || return 1
> +

[ ... ]

> +	ovs_sbx "test_dp_features" dp_features_set dpfeat --features 4 \
> +	    &> /dev/null
> +	rc=$?
> +	if [ $rc -eq 95 ]; then
> +		info "tc skb extension is not compiled in, skipping recirc"
> +		return $ksft_skip

[Severity: Medium]
Does this leave the dpfeat datapath behind on kernels without
CONFIG_NET_TC_SKB_EXT?

ovs_add_dp() creates the datapath but only queues its removal:

ovs_add_dp () {
	...
	ovs_sbx "$sbxname" python3 $ovs_base/ovs-dpctl.py add-dp $*
	on_exit "ovs_sbx $sbxname python3 $ovs_base/ovs-dpctl.py del-dp $1;"
}

The queued del-dp only runs from ovs_exit_sig. run_test() calls
ovs_exit_sig for OK and FAIL, but not for a skip:

	elif [ $ret -eq $ksft_skip ]; then
		printf "TEST: %-60s  [SKIP]\n" "${tdesc}"

The script-level EXIT trap doesn't catch this either. run_test runs its
body in a subshell, and ovs_dir is only set inside that subshell.

Every other test that skips after setting up the sandbox (dec_ttl,
icmpv6, psample, drop_reason, ct/nat) calls ovs_exit_sig right before
return $ksft_skip.

Without that call, the dpfeat datapath and its netdev stay on the host
after the run. Later runs then reuse it without any warning, because
add-dp reports "already exists" and exits 0.

Should ovs_exit_sig be called here before returning $ksft_skip?

> +	elif [ $rc -ne 0 ]; then
> +		info "the recirc set failed with $rc"
> +		return 1
> +	fi

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108123424.2825403-1-houminxi%40gmail.com