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

[Hangbin Liu <hangbin.liu@linux.dev>]

On Wed, Sep 02, 2026 at 07:02:31AM +0000, sashiko-bot@kernel.org wrote:
> 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?

Thanks, I just find a tool iptables-translate and I will recheck each
translation.

> 
> [ ... ]
> 
> > @@ -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?

OK, I will add a regex to check if drop is number or not.

> 
> > +		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

Thanks, I will fix this.

Hangbin