Hi Hangbin, On 02/09/2026 08:52, Hangbin Liu wrote: > From: Hangbin Liu <[email protected]> > > Replace the per-address-family iptables rules with a single inet table > (mjoin_table) that handles both IPv4 and IPv6. The BPF bytecode for > matching MPTCP add-addr and remove-addr suboptions is replaced with > native nft matching via "tcp option mptcp subtype". Rule handles are
Good idea! > captured via "nft -e --handle" so that rules can be selectively removed > during tests. > > The config file adds CONFIG_NFT_NUMGEN (replaces iptables statistic nth), > CONFIG_NFT_REJECT and CONFIG_NFT_REJECT_IPV4 for reject‑related rules. > > The iptables/ip6tables check inside mptcp_lib.sh is kept in case any > one still need them. Please remove them, not to be tempted to use them. > Signed-off-by: Hangbin Liu <[email protected]> > --- > tools/testing/selftests/net/mptcp/config | 3 + > tools/testing/selftests/net/mptcp/mptcp_join.sh | 136 > +++++++++--------------- > 2 files changed, 51 insertions(+), 88 deletions(-) > > diff --git a/tools/testing/selftests/net/mptcp/config > b/tools/testing/selftests/net/mptcp/config > index 59051ee2a986..0d0a744c4ca8 100644 > --- a/tools/testing/selftests/net/mptcp/config > +++ b/tools/testing/selftests/net/mptcp/config > @@ -30,6 +30,9 @@ CONFIG_NET_SCH_NETEM=m > CONFIG_NF_TABLES=m > CONFIG_NF_TABLES_INET=y > CONFIG_NFT_COMPAT=m > +CONFIG_NFT_NUMGEN=y > +CONFIG_NFT_REJECT=m > +CONFIG_NFT_REJECT_IPV4=m Even if we currently don't need the v6 version, I wonder if we shouldn't add it here. Up to you, when we will need it, we can also add it here, fine. I wonder if we shouldn't remove the ones linked to IPTables. I was thinking that maybe we could keep them for debug purposes, but same as the reject v6, we can add them when required instead of guessing which ones would be useful, "just in case". So yes, do you mind removing the ones that are no longer needed, please? > 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 18ce7136a2b0..05cbaddb8261 100755 > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh > @@ -26,8 +26,6 @@ capout="" > cappid="" > ns1="" > ns2="" > -iptables="iptables" > -ip6tables="ip6tables" > timeout_poll=30 > timeout_test=$((timeout_poll * 2 + 1)) > capture=false > @@ -50,6 +48,7 @@ declare -A failed_tests > MPTCP_LIB_TEST_FORMAT="%03u %s\n" > TEST_NAME="" > nr_blank=6 > +nft_handle="" > > # These var are used only in some tests, make sure they are not already set > unset FAILING_LINKS > @@ -99,42 +98,6 @@ unset add_addr_tx_nr > unset add_addr_echo_tx_nr > unset add_addr_drop_tx_nr > > -# generated using "nfbpf_compile '(ip && (ip[54] & 0xf0) == 0x30) || > -# (ip6 && (ip6[74] & 0xf0) == 0x30)'" > -CBPF_MPTCP_SUBOPTION_ADD_ADDR="14, > - 48 0 0 0, > - 84 0 0 240, > - 21 0 3 64, > - 48 0 0 54, > - 84 0 0 240, > - 21 6 7 48, > - 48 0 0 0, > - 84 0 0 240, > - 21 0 4 96, > - 48 0 0 74, > - 84 0 0 240, > - 21 0 1 48, > - 6 0 0 65535, > - 6 0 0 0" > - > -# IPv4: TCP hdr of 48B, a first suboption of 12B (DACK8), the RM_ADDR > suboption > -# generated using "nfbpf_compile '(ip[32] & 0xf0) == 0xc0 && ip[53] == 0x0c > && > -# (ip[66] & 0xf0) == 0x40'" > -CBPF_MPTCP_SUBOPTION_RM_ADDR="13, > - 48 0 0 0, > - 84 0 0 240, > - 21 0 9 64, > - 48 0 0 32, > - 84 0 0 240, > - 21 0 6 192, > - 48 0 0 53, > - 21 0 4 12, > - 48 0 0 66, > - 84 0 0 240, > - 21 0 1 64, > - 6 0 0 65535, > - 6 0 0 0" Good to get rid of that. Also not to have get_maintainer.pl cc'ing the BPF ML just for that :) > init_partial() > { > capout=$(mktemp) > @@ -147,6 +110,14 @@ init_partial() > if $checksum; then > ip netns exec $netns sysctl -q > net.mptcp.checksum_enabled=1 > fi > + > + ip netns exec "$netns" nft add table inet mjoin_table > + ip netns exec "$netns" nft add chain inet mjoin_table input \ > + '{ type filter hook input priority filter; policy > accept; }' > + ip netns exec "$netns" nft add chain inet mjoin_table output \ > + '{ type filter hook output priority filter; policy > accept; }' > + ip netns exec "$netns" nft add chain inet mjoin_table mangle \ > + '{ type filter hook output priority mangle; policy > accept; }' I hope having this done by default for all subtests will not have a big impact at the end when using a debug kernel. Do you mind checking the impact, please? Just not to add a few seconds for each of the 130+ subtest if it is only needed in some of them. If it is, we could move that to a new helper and call it when 'nft' is required, it shouldn't be in many places I guess. This new helper could also be used to add new rules, or this could be a "reset_" helper, I didn't check what would be best. > done > > check_invert=0 > @@ -196,7 +167,7 @@ init() { > > mptcp_lib_check_mptcp > mptcp_lib_check_kallsyms > - mptcp_lib_check_tools ip tc ss "${iptables}" "${ip6tables}" > + mptcp_lib_check_tools ip tc ss nft > > sin=$(mktemp) > sout=$(mktemp) > @@ -381,23 +352,18 @@ reset_with_cookies() > reset_with_add_addr_timeout() > { > local ip="${2:-4}" > - local tables > > reset "${1}" || return 1 > > - tables="${iptables}" > - if [ $ip -eq 6 ]; then > - tables="${ip6tables}" > - fi > - > # set a maximum, to avoid too long timeout with exponential backoff > ip netns exec $ns1 sysctl -q net.mptcp.add_addr_timeout=1 > > - if ! ip netns exec $ns2 $tables -A OUTPUT -p tcp \ > - -m tcp --tcp-option 30 \ > - -m bpf --bytecode \ > - "$CBPF_MPTCP_SUBOPTION_ADD_ADDR" \ > - -j DROP; then > + > + nft_handle=$(ip netns exec "$ns2" nft -e --handle add rule \ > + inet mjoin_table output meta nfproto ipv${ip} \ > + tcp option mptcp subtype add-addr \ That's clearer, nice! I just hope devs and CIs will use a recent enough version for nft (>= 1.1.2 from Apr. 25) to support mptcp subtypes. (Fine to use them, no need to have a fallback mechanism.) > + drop | head -n1 | awk '{print $NF}') Why do you need "head -n1 | awk '{print $NF}'"? Can we not look at the ret code like we did with IPTables? Same below with the RM_ADDR subtype, but for the reject ones, you do check the ret code. EDIT: mmh, I see you are using "nft_handle" below, but not the one set here, right?. That's not very clear when it is set in the function and used later. Plus this field is not reset before/after each subtest. Is this really needed? I guess you used it for others because it is easier remove rules, right? If you don't need this one (or any set in helpers), don't set it/them, and don't use a global variable. Or reset it in init_partial, but prefer using local variable with a limited scope. Also, maybe clearer to use 'nft -j' with 'jq' to get that (if possible)? One last note: for new features linked to MPTCP that might take multiple versions to get ready, it might be better to send these patches only to the MPTCP ML (no need to add anybody else in cc). Then we will apply them in our tree and send them to netdev when we consider them as "ready" (and hope for Clashiko not to get back to them days/weeks later, but that should be a temporally issue :) ). Cheers, Matt -- Sponsored by the NGI0 Core fund.
