[Devel] [PATCH hci-8.1 v4 1/1] selftests: net: skip the veth GRO checks where they cannot run

[no-reply at sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] selftests: veth.sh udp_encap_in_use() probes only some UDP encap users
- [Low] selftests: veth.sh ss port regex is not anchored to local encap sockets
- [Low] selftests: veth.sh UDP_ENCAP flag is inverted and skips are not reported as skips
--

commit f9959728bd7c821cbe897be9079c649853692e5a
Author: Eva Kurchatova <eva.kurchatova at virtuozzo.com>

selftests: net: skip the veth GRO checks where they cannot run

This patch adds udp_encap_in_use() to veth.sh. It detects UDP
encapsulation users such as vxlan, geneve, bareudp, fou, l2tp and sockets
on ports 4789 and 6081. A new chk_gro_fwd() wrapper then skips the three
rx-udp-gro-forwarding aggregation checks when any of them is found.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-139651

> diff --git a/tools/testing/selftests/net/veth.sh b/tools/testing/selftests/net/veth.sh
> --- a/tools/testing/selftests/net/veth.sh
> +++ b/tools/testing/selftests/net/veth.sh

[ ... ]

> +udp_encap_in_use() {
> +	local ns cmd t
> +
> +	for ns in "" `ip netns list 2>/dev/null | awk '{print $1}'`; do

[Severity: Medium]
udp_encap_enable() does static_branch_inc(&udp_encap_needed_key), and
udp_gro_receive() tests that global key. The set of callers is much wider
than what this function probes. WireGuard (drivers/net/wireguard/socket.c),
GTP (drivers/net/gtp.c) and the UDP_ENCAP setsockopt path, which covers
ESP-in-UDP and NAT-T sockets, all reach it.

Consider a host with only one of those. None of the vxlan, geneve,
bareudp, fou, l2tp or port 4789/6081 probes match, so udp_encap_in_use()
returns 1. chk_gro_fwd() then calls chk_gro(), and the check still fails
with 10 packets where 1 is expected. Doesn't that leave the failure this
patch is meant to remove?

Only named namespaces from "ip netns list" are visited. Anonymous and
container-runtime namespaces are missed, even though the key is global.
Can the commit message wording "in any namespace" hold with this approach?

Errors from "ip fou show", "ip l2tp show tunnel" and "ss" are discarded
with 2>/dev/null. Doesn't a missing tool or an unloaded module then read
as "no encapsulation"?

The probe also runs only once, before the tests start. Tunnels created
later are not seen.

The commit message says "Any UDP encapsulation socket on the machine".
The comment above chk_gro_fwd() says "Only meaningful where nothing else
has turned udp_encap_needed_key on". Could both be reworded to match the
subset of users that is actually detected?

> +		$cmd ss -uapn 2>/dev/null | \
> +			grep -qE ':(4789|6081)([^0-9]|$)' && return 0

[Severity: Low]
ss -uapn prints both the local and the peer address:port columns. This
regex is not anchored to the local column. Can a plain UDP socket bound to
4789 or 6081, or a client connected to a peer on those ports, match here?
Such sockets do not enable udp_encap_needed_key, so the checks would be
skipped on hosts where they could have run.

Conversely, encap sockets on non-default ports are only caught through the
link type checks.

[ ... ]

> +# Only meaningful where nothing else has turned udp_encap_needed_key on
> +chk_gro_fwd() {
> +	local msg="$1"
> +	local expected=$2
> +
> +	if [ $UDP_ENCAP -eq 0 ]; then
> +		printf "%-60s" "$msg"
> +		echo " skip - UDP encapsulation in use"
> +		return
> +	fi

[Severity: Low]
This isn't a bug, but UDP_ENCAP=0 meaning "encapsulation in use" comes
from storing the exit status of udp_encap_in_use() with UDP_ENCAP=$?. It
is easy to misread.

The skip path only prints text. It does not set the kselftest skip exit
code (4). Can the script then exit 0 when all three gated checks were
skipped?

> +
> +	chk_gro "$msg" $expected
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261005112332.1989594-1-eva.kurchatova@virtuozzo.com?part=1