Re: [PATCH net-next 4/5] selftests: mptcp: convert iptables to nftables for mptcp_sockopt.sh

[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 nft/jq tool check added to mptcp_lib_check_tools()
  (tools/testing/selftests/net/mptcp/mptcp_lib.sh:531-532) only runs…
- [Low] In add_mark_rules()
  (tools/testing/selftests/net/mptcp/mptcp_sockopt.sh:57-60), the…

--

selftests: mptcp: convert iptables to nftables for mptcp_sockopt.sh

This commit makes mptcp_sockopt.sh use nftables instead of
iptables/ip6tables. It creates ip/ip6 filter tables with an OUTPUT chain,
and check_mark() now reads the drop counter through nft -j and jq. It also
extends mptcp_lib_check_tools() to probe for nft and jq.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index b9d14647f4019..e65b4ebee06a6 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -528,7 +528,7 @@ mptcp_lib_check_tools() {
>  				exit ${KSFT_SKIP}
>  			fi
>  			;;
> -		"iptables"* | "ip6tables"*)
> +		"iptables"* | "ip6tables"* | "nft" | "jq")
>  			if ! "${tool}" -V &> /dev/null; then

[Severity: Medium]
Is nft -V the right probe for nft here?

For nft, -V prints extended build information, and only newer nftables
releases have it. The portable version flag is -v or --version, which is
what the other selftests use. For example, the netfilter/*.sh scripts call
nft --version.

If an older nft rejects -V, would mptcp_lib_check_tools() exit with
KSFT_SKIP from the top-level call in mptcp_sockopt.sh?

    mptcp_lib_check_tools ip nft jq

That would also skip the transfer, sockopt and TCP_INQ subtests, which
don't use nft at all.

The opposite case is also a concern. check_mark() needs an nft that was
built with JSON support:

    drops=$(ip netns exec "$ns" nft -j list table "$tables" filter | \
    ...
    if [ -z "$drops" ] || [ "$drops" -ne 0 ]; then
            mptcp_lib_pr_fail "got $tables $drops in ns $ns," \
                              "not 0 - not all expected packets marked"
            ret=${KSFT_FAIL}

On an nftables build without libjansson, nft -V still exits 0 and just
reports json: no. add_mark_rules() works, because it only uses nft -f.

nft -j then fails with "JSON support not compiled-in", so drops is empty.
The test then reports "not all expected packets marked" and sets
KSFT_FAIL. Should a missing userspace feature give a skip here instead?

The later patch in the series, "selftests: mptcp: convert iptables to
nftables for mptcp_join.sh", keeps the same "nft" | "jq" case with the -V
probe. So it looks like this is still present at the end of the series.

Would a separate nft check match the real dependency better? For example,
nft --version plus a JSON check such as nft -j list ruleset.

>  				mptcp_lib_pr_skip "Could not run all tests without ${tool}"
>  				exit ${KSFT_SKIP}
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index e850a87429b61..b4e3eddc9cf21 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh

[ ... ]

> @@ -49,16 +47,27 @@ add_mark_rules()
>  	local ns=$1
>  	local m=$2
>  
> -	local t
> -	for t in ${iptables} ${ip6tables}; do
> -		# just to debug: check we have multiple subflows connection requests
> -		ip netns exec $ns $t -A OUTPUT -p tcp --syn -m mark --mark $m -j ACCEPT
> -
> -		# RST packets might be handled by a internal dummy socket
> -		ip netns exec $ns $t -A OUTPUT -p tcp --tcp-flags RST RST -m mark --mark 0 -j ACCEPT
> -
> -		ip netns exec $ns $t -A OUTPUT -p tcp -m mark --mark $m -j ACCEPT
> -		ip netns exec $ns $t -A OUTPUT -p tcp -m mark --mark 0 -j DROP
> +	local table
> +	for table in ip ip6; do
> +		ip netns exec "$ns" nft -f - <<-EOF
> +			add table $table filter
> +			add chain $table filter OUTPUT \
> +				{ type filter hook output priority 0; policy accept; }
> +
> +			# just to debug: check we have multiple subflows connection requests
> +			add rule $table filter OUTPUT \
> +				tcp flags & (fin | syn | rst | ack) == syn \
> +				meta mark $m accept

[Severity: Low]
This isn't a bug, but is this comment still accurate after the conversion?

With iptables every rule had implicit counters, so iptables -v -L could
show how many subflow SYNs were sent. In nftables a rule only counts
packets if it has an explicit counter statement, and this rule has none.

Every packet this rule matches is also accepted by the later "meta
l4proto tcp meta mark $m accept" rule, so the rule now does nothing.

The commit message says:

  "Counters are not added to accept rules because the test does not
  inspect them."

However, this rule was meant for manual debugging rather than for the test
itself. Should it get a counter, or should the rule and its comment be
removed?

> +
> +			# RST packets might be handled by a internal dummy socket
> +			add rule $table filter OUTPUT \
> +				tcp flags & rst == rst meta mark 0x0 accept

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260926-net-next-mptcp-misc-feat-7-4-v1-0-67af4ab37406%40kernel.org