Re: [PATCH net-next 15/15] selftests: netfilter: add UDP flowtable offload test

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 3

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 test runs the UDP flowtable checksum code but cannot
  detect a bug in it.
- [Low] The header comment in nft_flowtable_udp.sh claims coverage the
  test does not provide.
- [Low] The "IPv4 UDP masquerade" and "IPv6 UDP masquerade" cases (lines
  235-236) never check that NAT actually happened.
- [Low] listener_ready() is supposed to wait for both UDP listeners but
  passes as soon as either one is bound.

--

selftests: netfilter: add UDP flowtable offload test

This adds nft_flowtable_udp.sh, which sends a burst of UDP echoes through
an ingress flowtable for IPv4 and IPv6, with and without masquerade. It
checks that the payload comes back intact, that the forward chain sees
only the handshake, and that conntrack marks the flow OFFLOAD and counts
every datagram.

> diff --git a/tools/testing/selftests/net/netfilter/nft_flowtable_udp.sh b/tools/testing/selftests/net/netfilter/nft_flowtable_udp.sh
> new file mode 100755
> index 0000000000000..e0f05c76bd8ec
> --- /dev/null
> +++ b/tools/testing/selftests/net/netfilter/nft_flowtable_udp.sh
> @@ -0,0 +1,238 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# nft_flowtable.sh offloads TCP only. These UDP paths therefore stay at
> +# zero coverage in that run:
> +#
> +#   nf_flow_tuple_ip / nf_flow_tuple_ipv6          IPPROTO_UDP
> +#   nf_flow_nat_ip_udp / nf_flow_nat_port_udp      IPv4 NAT checksum
> +#   nf_flow_snat_ipv6 / nf_flow_dnat_ipv6          IPv6 NAT, including UDP
> +#   flow_offload_get_timeout / flow_offload_fixup_ct for UDP

[Severity: Low]
Is this list accurate for nf_flow_dnat_ipv6 and flow_offload_fixup_ct?

The only NAT rule the test adds is a postrouting masquerade, which is
source NAT only. flow_offload_alloc() sets NF_FLOW_DNAT only when the
conntrack has IPS_DST_NAT:

net/netfilter/nf_flow_table_core.c:flow_offload_alloc() {
    ...
	if (ct->status & IPS_DST_NAT)
		__set_bit(NF_FLOW_DNAT, &flow->flags);
    ...
}

nf_flow_nat_ipv6() calls the DNAT helpers only when that bit is set:

	if (test_bit(NF_FLOW_DNAT, &flow->flags)) {
		nf_flow_dnat_port(flow, skb, thoff, ip6h->nexthdr, dir);
		nf_flow_dnat_ipv6(flow, skb, ip6h, thoff, dir);

So nf_flow_dnat_ipv6() never seems to run in this test.

The only caller of flow_offload_fixup_ct() is flow_offload_teardown().
The UDP offload timeout is 30 seconds, the test finishes in a few seconds,
and no route or device changes. Teardown only happens when cleanup_all_ns
runs from the EXIT trap, after PASS/FAIL has been printed. Nothing checks
the conntrack state or timeout after that point.

A minor point: tools/testing/selftests/bpf/prog_tests/xdp_flowtable.c
already adds UDP flows to a flowtable. The comment does limit its "zero
coverage" claim to the nft_flowtable.sh run, though.

Could the comment be corrected, or could a DNAT case and a teardown/fixup
check be added?

[ ... ]

> +listener_ready() {
> +	ss -N "$ns2" -uln -H "sport = :$port" | grep -q "$port"
> +}
> +
> +ip netns exec "$ns2" socat -T 60 \
> +	UDP4-LISTEN:"$port",reuseaddr,fork EXEC:cat >/dev/null 2>&1 &
> +ip netns exec "$ns2" socat -T 60 \
> +	UDP6-LISTEN:"$port",reuseaddr,ipv6only=1,fork EXEC:cat >/dev/null 2>&1 &
> +
> +if ! busywait 3000 listener_ready; then

[Severity: Low]
Can listener_ready() succeed when only one of the two listeners is bound?

The two socat processes start in parallel, and grep -q succeeds on the
first matching socket line. So busywait can return once 0.0.0.0:12345 is
bound, before the UDP6 socat has bound [::]:12345.

On a heavily loaded machine, the "IPv6 UDP" case could then send its
first datagram before the IPv6 listener exists. It would get ICMPv6 port
unreachable and fail with "FAIL: packet 0 timeout", which has nothing to
do with the flowtable. The 32 IPv4 round trips that run first make this
unlikely in practice.

Would counting the matching sockets (at least two), or checking ss -4 and
ss -6 separately, close this gap?

> +
> +udp_exchange() {

[ ... ]

> +	i=0
> +	while [ "$i" -lt "$count" ]; do
> +		msg=$(printf '%04d' "$i")
> +		printf '%s' "$msg" >&"${SO[1]}"
> +		if ! IFS= read -r -n 4 -t 2 got <&"${SO[0]}"; then
> +			echo "FAIL: packet $i timeout" >&2
> +			kill "$SO_PID" 2>/dev/null || true
> +			wait "$SO_PID" 2>/dev/null || true
> +			return 1
> +		fi
> +		if [ "$got" != "$msg" ]; then

[Severity: Medium]
Can this payload comparison catch a broken UDP checksum update in the
flowtable NAT code?

The commit message lists "the UDP checksum update" as newly covered. The
header comment labels nf_flow_nat_ip_udp / nf_flow_nat_port_udp as "IPv4
NAT checksum".

All links are veth pairs with default features, and VETH_FEATURES
includes NETIF_F_HW_CSUM. So the datagrams socat sends stay
CHECKSUM_PARTIAL from end to end.

On the router, nf_flow_nat_ip_udp() takes this branch, which only adjusts
the pseudo-header seed:

net/netfilter/nf_flow_table_ip.c:nf_flow_nat_ip_udp() {
	if (udph->check || skb->ip_summed == CHECKSUM_PARTIAL) {
		inet_proto_csum_replace4(&udph->check, skb, addr,
					 new_addr, true);
    ...
}

nf_flow_nat_port_udp() calls inet_proto_csum_replace2(..., false). For
CHECKSUM_PARTIAL skbs, that leaves uh->check untouched.

The receiver in ns2 (or ns1 for replies) treats CHECKSUM_PARTIAL as
already valid in skb_csum_unnecessary():

	(skb->ip_summed == CHECKSUM_PARTIAL &&
	 skb_checksum_start_offset(skb) >= 0));

A wrong udph->check written by the flowtable would still be delivered.
The 4-byte payload would still match, and the test would still print
PASS. The CHECKSUM_NONE full-checksum branch never runs at all.

Would turning off tx checksum offload on the eth0 devices in ns1 and ns2
(for example ethtool -K eth0 tx off) force a real checksum that the
receiver verifies after the flowtable rewrites it?

[ ... ]

> +if ! ip netns exec "$nsr" nft add rule inet nat postrouting \
> +		oifname "veth1" masquerade; then
> +	echo "FAIL: could not add masquerade rule" >&2
> +	exit 1
> +fi
> +
> +run_case "IPv4 UDP masquerade" 4 10.0.2.99 5403
> +run_case "IPv6 UDP masquerade" 6 dead:2::99 5404

[Severity: Low]
Do these masquerade cases check that NAT actually happened?

run_case() checks three things: [OFFLOAD] in the conntrack line, the slow
counter, and the orig/reply packet counts. None of these differ between a
NATed flow and a plain one. ns2 has a default route back through nsr, so
the echo works with or without SNAT.

If the conntrack entry were not NATed, these cases would pass just like
the plain ones, without ever reaching nf_flow_snat_ip() or
nf_flow_snat_ipv6().

There is one partial safeguard. If the flowtable failed to apply an
existing SNAT binding, the slow counter would probably go over 8. A
missing binding would still go unnoticed.

Could run_case() check the conntrack line for a reply tuple that points
at the router, such as dst=10.0.2.1 or dst=dead:2::1?

> +
> +exit $ret

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113031251.2437074-2-pablo%40netfilter.org