Re: [PATCH net v2 2/2] selftests: net: check TCP IP IDs across tunnels after GRO and GSO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Low] selftests: net: check() fails instead of skipping on outer=incr
  without GRO
- [Low] selftests: net: missing nsim_del() in cleanup() trap and
  teardown order
- [Low] selftests: net: tcp_fixedid.sh duplicates lib.sh netdevsim and
  wait helpers
- [Low] selftests: net: do_rx() rejects all inner IDs when optional -c
  is omitted
--

commit 6de9851dc8b11c95f79b39bbfd7e7c1ba08870cd
Author: Julius Bairaktaris <julius@bairaktaris.de>

selftests: net: check TCP IP IDs across tunnels after GRO and GSO

This commit adds a selftest that sends TCP segments with fixed or
incrementing IPv4 IDs across veth GRO and tunnel software segmentation,
verifying that inner IP IDs are preserved and new outer IPv4 IDs are
unique or incrementing.

> diff --git a/tools/testing/selftests/net/tcp_fixedid.c b/tools/testing/selftests/net/tcp_fixedid.c
> new file mode 100644
> index 0000000000000..25c752beab4ed
> --- /dev/null
> +++ b/tools/testing/selftests/net/tcp_fixedid.c
> @@ -0,0 +1,411 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Send TCP segments with fixed or incrementing IPv4 IDs, optionally in
> + * VXLAN, and report the IP IDs of TCP segments received on a device,
> + * also inside IPIP, GRE, VXLAN or ESP.
> + *
> + * tx: tcp_fixedid -T -i dev -M dmac -s saddr -d daddr [-f] [-n segs]
> + *                 [-b bursts] [-l len] [-V osaddr,odaddr -m inner_dmac [-F]]
> + * rx: tcp_fixedid -R -i dev [-f] [-c segs] [-w ms]

[ ... ]

> +static int do_rx(int fd)
> +{
> +	static uint8_t outer_map[8192], inner_map[8192];
> +	unsigned int frames = 0, outer4 = 0, outer_ids = 0, inner_ids = 0;
> +	unsigned int inner_bad = 0;
> +	unsigned char buf[65536];
> +	struct pollfd pfd = { .fd = fd, .events = POLLIN };
> +
> +	printf("ready\n");
> +	fflush(stdout);
> +
> +	/* wait longer for the first segment than between segments */
> +	while (poll(&pfd, 1, frames ? wait_ms : 10 * wait_ms) > 0) {
> +		const struct iphdr *iph, *oiph;
> +		struct sockaddr_ll from;
> +		socklen_t fl = sizeof(from);
> +		uint16_t id;
> +		int n;
> +
> +		n = recvfrom(fd, buf, sizeof(buf), 0, (struct sockaddr *)&from,
> +			     &fl);
> +		if (n < 0)
> +			error(1, errno, "recvfrom");
> +		if (from.sll_pkttype == PACKET_OUTGOING)
> +			continue;
> +		iph = find_inner(buf, n, &oiph);
> +		if (!iph)
> +			continue;
> +		frames++;
> +		id = ntohs(iph->id);
> +		count_id(inner_map, &inner_ids, id);
> +		if (fixed_id ? id != FIXED_ID : (uint16_t)(id - INCR_ID) >= stop)
> +			inner_bad++;
> +		if (oiph) {
> +			outer4++;
> +			count_id(outer_map, &outer_ids, ntohs(oiph->id));
> +		}
> +		if (stop && frames == (unsigned int)stop)
> +			break;
> +	}

[Severity: Low]
Does do_rx() count every segment as inner_bad when the optional -c flag is
omitted in incrementing-ID mode?  With stop left at 0 and fixed_id false,
(uint16_t)(id - INCR_ID) >= stop is always true.

[ ... ]

> diff --git a/tools/testing/selftests/net/tcp_fixedid.sh b/tools/testing/selftests/net/tcp_fixedid.sh
> new file mode 100755
> index 0000000000000..05ef7ec6b08e7
> --- /dev/null
> +++ b/tools/testing/selftests/net/tcp_fixedid.sh
> @@ -0,0 +1,400 @@

[ ... ]

> +cleanup() {
> +	cleanup_all_ns
> +	rm -f "$RESULT"
> +}
> +
> +trap cleanup EXIT

[Severity: Low]
Should cleanup() also call nsim_del() before cleanup_all_ns()?  If the
script exits via the trap during run_esp() (for example on SIGINT or
PAUSE_ON_FAIL=yes), netdevsim$NSIM_ID and netdevsim$((NSIM_ID + 1)) are
left behind in /sys/bus/netdevsim and their interfaces move back into the
initial network namespace when the namespaces are removed.  At the end of
run_esp(), cleanup_all_ns is also called before nsim_del.

[ ... ]

> +# check <name> <result> <want: fixed|incr> <outer: 0|unique|incr> <merged>
> +check() {
> +	local name=$1 res=$2 want=$3 outer=$4 sent=$5
> +	local frames outer_ids inner_ids inner_bad
> +
> +	read -r _ frames _ _ _ outer_ids _ inner_ids _ inner_bad <<< "$res"
> +	RET=0
> +	if [ -z "$inner_bad" ]; then
> +		check_err 1 "no result from the receiver"
> +		log_test "$name"
> +		return
> +	fi
> +	check_err $((frames != TOTAL)) "$frames of $TOTAL segments received"
> +	check_err "$inner_bad" "$inner_bad inner IDs not sent"
> +	if [ "$want" = fixed ]; then
> +		check_err $((inner_ids != 1)) "$inner_ids inner IDs, want 1"
> +	else
> +		check_err $((inner_ids != TOTAL)) \
> +			"$inner_ids inner IDs, want $TOTAL"
> +	fi
> +	if [ "$outer" = unique ]; then
> +		check_err $((outer_ids != frames)) \
> +			"$outer_ids outer IDs for $frames segments"
> +	elif [ "$outer" = incr ]; then
> +		check_err $((outer_ids < 2)) "fixed outer IDs"
> +	fi
> +	if [ "$RET" -eq 0 ] && [ \"$sent\" -ge \"$TOTAL\" ]; then
> +		ret_set_ksft_status \"$ksft_skip\" \"GRO did not merge\"
> +	fi
> +	log_test "$name"
> +}

[Severity: Low]
When GRO does not merge in run_esp() (sent >= TOTAL), does check() fail
with "fixed outer IDs" instead of skipping?  Each unmerged packet has DF
set, so ESP tunnel encapsulation gives every packet outer ID 0
(outer_ids == 1); check_err $((outer_ids < 2)) then sets RET=1 before the
sent >= TOTAL check can mark the test as skipped.

> +
> +# capture <ns> <dev> <want: fixed|incr>: start the receiver in the
> +# background, pid in CAP_PID, and wait until it is bound
> +capture() {
> +	local f="" i
> +
> +	[ "$3" = fixed ] && f=-f
> +	: > "$RESULT"
> +	ip netns exec "$1" "$TX" -R -i "$2" -c "$TOTAL" -w 3000 $f > "$RESULT" &
> +	CAP_PID=$!
> +	for i in $(seq 100); do
> +		grep -q ready "$RESULT" && return
> +		sleep 0.1
> +	done
> +}

[ ... ]

> +NSIM_ID=$((RANDOM % 1024 + 1024))
> +NSIM_BUS=/sys/bus/netdevsim
> +
> +# nsim_add <id> <ns>: create a netdevsim port in <ns>, print its name
> +nsim_add() {
> +	local dev
> +
> +	echo "$1 1" > "$NSIM_BUS/new_device" || return 1
> +	for dev in "$NSIM_BUS/devices/netdevsim$1/net/"*; do
> +		dev=${dev##*/}
> +		ip link set "$dev" netns "$2" || return 1
> +		echo "$dev"
> +		return 0
> +	done
> +	return 1
> +}
> +
> +nsim_del() {
> +	echo "$NSIM_ID" > "$NSIM_BUS/del_device" 2>/dev/null
> +	echo $((NSIM_ID + 1)) > "$NSIM_BUS/del_device" 2>/dev/null
> +}

[Severity: Low]
Could this script reuse create_netdevsim(), cleanup_netdevsim(),
slowwait() and check_gen_prog() from lib.sh instead of open-coding
nsim_add(), nsim_del(), the readiness loop in capture() and the [ ! -x
"$TX" ] check?  In particular, create_netdevsim() creates the device
inside the target netns after udevadm settle, avoiding a race with host
udevd renaming the interface before ip link set moves it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794139176.git.julius@bairaktaris.de?part=2