Re: [PATCH net-next 5/5] selftests: mptcp: convert iptables to nftables for mptcp_join.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] In endpoint_tests()
  (tools/testing/selftests/net/mptcp/mptcp_join.sh), the new rule `tcp…
- [Low] The patch removes CONFIG_NFT_COMPAT from
  tools/testing/selftests/net/mptcp/config, "since we don't need it now".

--

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

This converts the iptables/ip6tables rules in mptcp_join.sh to nftables.
It keeps the same table and chain names and adds init_nftables() to set
up the tables only when a test needs them. The cBPF bytecode that matched
the ADD_ADDR and RM_ADDR suboptions is replaced with "tcp option mptcp
subtype" matching. In the config fragment, CONFIG_NFT_COMPAT is swapped
for the nft modules that are now needed.

> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> index bb9c4c97c6206..61057a0667192 100644
> --- a/tools/testing/selftests/net/mptcp/config
> +++ b/tools/testing/selftests/net/mptcp/config
> @@ -29,7 +29,9 @@ CONFIG_NET_SCH_INGRESS=m
>  CONFIG_NET_SCH_NETEM=m
>  CONFIG_NF_TABLES=m
>  CONFIG_NF_TABLES_INET=y
> -CONFIG_NFT_COMPAT=m
> +CONFIG_NFT_NUMGEN=m
> +CONFIG_NFT_REJECT=m
> +CONFIG_NFT_REJECT_INET=m

[Severity: Low]
This isn't a bug, but CONFIG_NFT_COMPAT is removed here "since we don't
need it now". Should the other xtables-only options in this fragment be
removed too?

After this patch the config still has:

CONFIG_IP6_NF_FILTER=m
CONFIG_IP6_NF_TARGET_REJECT=m
CONFIG_IP_NF_FILTER=m
CONFIG_IP_NF_MANGLE=m
CONFIG_IP_NF_TARGET_REJECT=m
CONFIG_NETFILTER_XTABLES=m
CONFIG_NETFILTER_XTABLES_LEGACY=y
CONFIG_NETFILTER_XT_MATCH_BPF=m
CONFIG_NETFILTER_XT_MATCH_LENGTH=m
CONFIG_NETFILTER_XT_MATCH_STATISTIC=m
CONFIG_NETFILTER_XT_TARGET_MARK=m

Only the removed -m bpf, -m length, -m statistic, -j MARK and REJECT
rules needed these. Nothing under tools/testing/selftests/net/mptcp/
uses iptables or ip6tables anymore.

>  CONFIG_NFT_SOCKET=m
>  CONFIG_NFT_TPROXY=m
>  CONFIG_SYN_COOKIES=y
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 18ce7136a2b0e..b16e24418e737 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh

[ ... ]

> @@ -4402,12 +4372,10 @@ endpoint_tests()
>  
>  		# To make sure RM_ADDR are sent over a different subflow, but
>  		# allow the rest to quickly and cleanly close the subflow
> -		local ipt=1
> -		ip netns exec "${ns2}" ${iptables} -I OUTPUT -s "10.0.1.2" \
> -			-p tcp -m tcp --tcp-option 30 \
> -			-m bpf --bytecode \
> -			"$CBPF_MPTCP_SUBOPTION_RM_ADDR" \
> -			-j DROP || ipt=0
> +		local nft=1
> +		ip netns exec "${ns2}" nft insert rule ip filter OUTPUT \
> +			ip saddr 10.0.1.2 meta l4proto tcp \
> +			tcp option mptcp subtype remove-addr drop || nft=0

[Severity: Medium]
Can this rule ever match an RM_ADDR packet on an established connection?

nft_exthdr_tcp_eval() stops at the first TCP option whose kind matches
(30 for MPTCP). The subtype check therefore only ever sees the first
MPTCP suboption:

net/netfilter/nft_exthdr.c:nft_exthdr_tcp_eval() {
    ...
	for (i = sizeof(*tcph); i < tcphdr_len - 1; i += optl) {
		optl = optlen(opt, i);

		if (priv->type != opt[i])
			continue;
		...
		return;
	}
    ...
}

Once can_ack is set, mptcp_established_options_dss() always selects a
DSS ack:

net/mptcp/options.c:mptcp_established_options_dss() {
    ...
	opts->ext_copy.use_ack = 1;
	opts->suboptions = OPTION_MPTCP_DSS;
    ...
}

mptcp_established_options_rm_addr() then ORs OPTION_MPTCP_RM_ADDR into
suboptions, so the DSS is kept. mptcp_write_options() writes the DSS
first and writes RM_ADDR later, after the mp_capable_done label:

net/mptcp/options.c:mptcp_write_options() {
    ...
	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
    ...
mp_capable_done:
	if (OPTION_MPTCP_RM_ADDR & opts->suboptions) {
    ...
}

On these packets the first kind 30 option is the DSS (subtype 0x2), so
wouldn't "subtype remove-addr" (0x4) always fail to match?

The removed CBPF_MPTCP_SUBOPTION_RM_ADDR bytecode was written for this
layout. Its comment said "a first suboption of 12B (DACK8), the RM_ADDR
suboption". It checked ip[53] == 0x0c for the length of the first
suboption and (ip[66] & 0xf0) == 0x40 for the subtype of the second one.

The same problem affects the ns1 rule on 10.0.1.1 later in
endpoint_tests(), which is installed right before
"pm_nl_del_endpoint $ns1 42 10.0.1.1".

In both places the rule installs without error, so the "|| nft=0"
fallback never triggers. The tests keep passing, but RM_ADDR is no
longer forced onto a different subflow.

The commit message says the bytecode "is replaced with native nft
matching using "tcp option mptcp subtype"". That is true for ADD_ADDR,
because mptcp_write_options() emits ADD_ADDR in an else-if branch with
the DSS. Is it also true for RM_ADDR?

Would a raw payload match on the second MPTCP option (for example with
@th,...) be needed here instead?

>  		local i
>  		for i in $(seq 3); do
>  			pm_nl_del_endpoint $ns2 1 10.0.1.2

[ ... ]

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