Re: [PATCH mptcp-next 1/2] selftests: mptcp: convert iptables to nftables for mptcp_sockopt.sh

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Replacing iptables TCP flag matching with nftables exact matching (`tcp flags rst` and `tcp flags syn`) fails to match packets with multiple flags set, causing spurious test failures.
- [Medium] An empty `$drop` variable causes a Bash syntax error during numeric evaluation, which evaluates to false and silently masks test failures.
- [Low] Typo in string interpolation for the error message (`IPv{$af}` instead of `IPv${af}`).
--

commit af165781f3fb777bc40bc4de19c7d79cd01f51cc
Author: Hangbin Liu <liuhangbin@kylinos.cn>

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

This patch converts the mptcp_sockopt.sh selftest to use nftables instead of
iptables. It replaces per-address-family iptables mark rules with an inet
table containing drop counting rules for IPv4 and IPv6.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index e850a87429b61..4d0af2bf94840 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh

[ ... ]

> @@ -49,17 +47,23 @@ 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

[ ... ]

> +	# just to debug: check we have multiple subflows connection requests
> +	ip netns exec "$ns" nft add rule inet msock_table output \
> +		 meta mark "$m" tcp flags syn accept
> +	# RST packets might be handled by a internal dummy socket
> +	ip netns exec "$ns" nft add rule inet msock_table output \
> +		meta mark 0 tcp flags rst accept

[Severity: Medium]
In add_mark_rules(), the original iptables rules --tcp-flags RST RST and
--syn used bitmasks to allow additional flags like ACK for RST or ECN for
SYN. The new nftables rules tcp flags rst and tcp flags syn enforce an
exact match on the entire TCP flags byte.

Will these exact matches reject packets with multiple flags set, such as
RST+ACK or SYN+ECN, causing them to fall through to the drop rule and fail the
test?

[ ... ]

> @@ -105,33 +109,23 @@ cleanup()
>  
>  mptcp_lib_check_mptcp
>  mptcp_lib_check_kallsyms
> -mptcp_lib_check_tools ip "${iptables}" "${ip6tables}"
> +mptcp_lib_check_tools ip nft
>  
>  check_mark()
>  {
>  	local ns=$1
>  	local af=$2
>  
> -	local tables=${iptables}
> +	drop=$(ip netns exec "$ns" nft list table inet msock_table | \
> +		grep "ipv$af.*packets.*drop" | awk '{print $(NF-3)}')
>  
> -	if [ $af -eq 6 ];then
> -		tables=${ip6tables}
> +	if [ "$drop" -ne 0 ]; then

[Severity: Medium]
If the command extraction for drop fails, or if grep finds no matches,
the $drop variable will be empty.

When bash evaluates [ "$drop" -ne 0 ] with an empty string, it triggers a
syntax error. This error evaluates to false and skips the failure block.

Does this risk silently masking test failures if the extraction logic ever
fails to produce a number?

> +		mptcp_lib_pr_fail "got $drop pkt drops in ns $ns IPv{$af} tables," \

[Severity: Low]
Is the string interpolation IPv{$af} a typo?

Bash interpolates the $af variable but leaves the literal braces, printing
IPv{4} instead of IPv4. Should this be IPv${af} or IPv$af instead?

> +				  "not 0 - not all expected packets marked"
> +		ret=${KSFT_FAIL}
> +		return 1
>  	fi

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-mptcp_nft-v1-0-559caa16f410@kylinos.cn?part=1