[PATCH] selftests: net: veth: check UDP GRO forwarding on forwarded traffic

[PATCH] selftests: net: veth: check UDP GRO forwarding on forwarded traffic

[Eva Kurchatova]

The rx-udp-gro-forwarding checks fail with "got 10 packets, expected 1"
on a host where something has a UDP tunnel socket, a vxlan say, and
pass on a host where nothing does. Since commit 36707061d6ba ("udp:
allow forwarding of plain (non-fraglisted) UDP GRO packets") the
feature aggregates only datagrams GRO finds no socket for, and GRO
looks the socket up only once a tunnel socket exists on the host; the
checks send to a socket in the receiving namespace, so they pass only
while nothing looks it up.

Let the destination forward the datagrams to a receiver in a namespace
behind it: no local socket is found there, tunnel or not, and the
checks count what the destination receives as before.

Fixes: 1c3cadbe0242 ("self-tests: add veth tests")
Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Link: https://lore.kernel.org/netdev/CAF=yD-LZSec9g_0bBgrcQvq2OO1BrxMSRHH=uU7eamCmWcTO_A@mail.gmail.com/
Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
---
 tools/testing/selftests/net/veth.sh | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/net/veth.sh b/tools/testing/selftests/net/veth.sh
index 8b867e0675a7..be7c6b9213a9 100755
--- a/tools/testing/selftests/net/veth.sh
+++ b/tools/testing/selftests/net/veth.sh
@@ -9,12 +9,17 @@ readonly DST=1
 readonly DST_NAT=100
 readonly NS_SRC=$BASE$SRC
 readonly NS_DST=$BASE$DST
+# The receiver of the GRO checks, behind the destination
+readonly RCV=3
+readonly NS_RCV=$BASE$RCV
 # shellcheck disable=SC2155 # prefer RO variable over return value from cmd
 readonly YNL="$(dirname "$(readlink -f "$0")")/../../../net/ynl/pyynl/cli.py"
 
 # "baremetal" network used for raw UDP traffic
 readonly BM_NET_V4=192.168.1.
 readonly BM_NET_V6=2001:db8::
+# Network between the destination and the receiver
+readonly RCV_NET_V4=192.168.2.
 
 readonly CPUS=`nproc`
 ret=0
@@ -26,7 +31,7 @@ cleanup() {
 	[ -n "${jobs}" ] && kill -1 ${jobs} 2>/dev/null
 	rm -f $STATS
 
-	for ns in $NS_SRC $NS_DST; do
+	for ns in $NS_SRC $NS_DST $NS_RCV; do
 		ip netns del $ns 2>/dev/null
 	done
 }
@@ -48,6 +53,17 @@ create_ns() {
 		ip -n $BASE$ns addr add dev veth$ns $BM_NET_V4$ns/24
 		ip -n $BASE$ns addr add dev veth$ns $BM_NET_V6$ns/64 nodad
 	done
+
+	# Forwarded, the datagrams find no local socket, whatever tunnel the host has
+	ip netns add $NS_RCV
+	ip -n $NS_RCV link set dev lo up
+	ip link add name fwd$DST type veth peer name fwd$RCV
+	ip link set dev fwd$DST netns $NS_DST up
+	ip link set dev fwd$RCV netns $NS_RCV up
+	ip -n $NS_DST addr add dev fwd$DST $RCV_NET_V4$DST/24
+	ip -n $NS_RCV addr add dev fwd$RCV $RCV_NET_V4$RCV/24
+	ip netns exec $NS_DST sysctl -wq net.ipv4.ip_forward=1
+	ip -n $NS_RCV route add default via $RCV_NET_V4$DST
 }
 
 __chk_flag() {
@@ -134,15 +150,17 @@ chk_gro() {
 	local msg="$1"
 	local expected=$2
 
-	ip netns exec $BASE$SRC ping -qc 1 $BM_NET_V4$DST >/dev/null
+	# Taking veth$SRC down drops the route, so set it for every check
+	ip -n $NS_SRC route replace ${RCV_NET_V4}0/24 via $BM_NET_V4$DST
+	ip netns exec $BASE$SRC ping -qc 1 $RCV_NET_V4$RCV >/dev/null
 	NSTAT_HISTORY=$STATS ip netns exec $NS_DST nstat -n
 
 	printf "%-60s" "$msg"
-	ip netns exec $BASE$DST ./udpgso_bench_rx -C 1000 -R 10 &
+	ip netns exec $NS_RCV ./udpgso_bench_rx -C 1000 -R 10 &
 	local spid=$!
 	sleep 0.1
 
-	ip netns exec $NS_SRC ./udpgso_bench_tx -4 -s 13000 -S 1300 -M 1 -D $BM_NET_V4$DST
+	ip netns exec $NS_SRC ./udpgso_bench_tx -4 -s 13000 -S 1300 -M 1 -D $RCV_NET_V4$RCV
 	local retc=$?
 	wait $spid
 	local rets=$?
-- 
2.56.0

Re: [PATCH] selftests: net: veth: check UDP GRO forwarding on forwarded traffic

[Willem de Bruijn]

Eva Kurchatova wrote:
> The rx-udp-gro-forwarding checks fail with "got 10 packets, expected 1"
> on a host where something has a UDP tunnel socket, a vxlan say, and
> pass on a host where nothing does. Since commit 36707061d6ba ("udp:
> allow forwarding of plain (non-fraglisted) UDP GRO packets") the
> feature aggregates only datagrams GRO finds no socket for, and GRO
> looks the socket up only once a tunnel socket exists on the host; the
> checks send to a socket in the receiving namespace, so they pass only
> while nothing looks it up.
> 
> Let the destination forward the datagrams to a receiver in a namespace
> behind it: no local socket is found there, tunnel or not, and the
> checks count what the destination receives as before.
> 
> Fixes: 1c3cadbe0242 ("self-tests: add veth tests")
> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Link: https://lore.kernel.org/netdev/CAF=yD-LZSec9g_0bBgrcQvq2OO1BrxMSRHH=uU7eamCmWcTO_A@mail.gmail.com/
> Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
> ---
>  tools/testing/selftests/net/veth.sh | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/veth.sh b/tools/testing/selftests/net/veth.sh
> index 8b867e0675a7..be7c6b9213a9 100755
> --- a/tools/testing/selftests/net/veth.sh
> +++ b/tools/testing/selftests/net/veth.sh
> @@ -9,12 +9,17 @@ readonly DST=1
>  readonly DST_NAT=100
>  readonly NS_SRC=$BASE$SRC
>  readonly NS_DST=$BASE$DST
> +# The receiver of the GRO checks, behind the destination
> +readonly RCV=3
> +readonly NS_RCV=$BASE$RCV
>  # shellcheck disable=SC2155 # prefer RO variable over return value from cmd
>  readonly YNL="$(dirname "$(readlink -f "$0")")/../../../net/ynl/pyynl/cli.py"
>  
>  # "baremetal" network used for raw UDP traffic
>  readonly BM_NET_V4=192.168.1.
>  readonly BM_NET_V6=2001:db8::
> +# Network between the destination and the receiver
> +readonly RCV_NET_V4=192.168.2.
>  
>  readonly CPUS=`nproc`
>  ret=0
> @@ -26,7 +31,7 @@ cleanup() {
>  	[ -n "${jobs}" ] && kill -1 ${jobs} 2>/dev/null
>  	rm -f $STATS
>  
> -	for ns in $NS_SRC $NS_DST; do
> +	for ns in $NS_SRC $NS_DST $NS_RCV; do
>  		ip netns del $ns 2>/dev/null
>  	done
>  }
> @@ -48,6 +53,17 @@ create_ns() {
>  		ip -n $BASE$ns addr add dev veth$ns $BM_NET_V4$ns/24
>  		ip -n $BASE$ns addr add dev veth$ns $BM_NET_V6$ns/64 nodad
>  	done
> +
> +	# Forwarded, the datagrams find no local socket, whatever tunnel the host has
> +	ip netns add $NS_RCV
> +	ip -n $NS_RCV link set dev lo up
> +	ip link add name fwd$DST type veth peer name fwd$RCV
> +	ip link set dev fwd$DST netns $NS_DST up
> +	ip link set dev fwd$RCV netns $NS_RCV up
> +	ip -n $NS_DST addr add dev fwd$DST $RCV_NET_V4$DST/24
> +	ip -n $NS_RCV addr add dev fwd$RCV $RCV_NET_V4$RCV/24

Similar to the cleanup code above, this can probably reuse the loop in
create_ns?

Also a bit confusing that NS_DST is no longer the dst, but keeps its name
and now there is an additional NS_RCV.

> +	ip netns exec $NS_DST sysctl -wq net.ipv4.ip_forward=1
> +	ip -n $NS_RCV route add default via $RCV_NET_V4$DST
>  }
>  
>  __chk_flag() {
> @@ -134,15 +150,17 @@ chk_gro() {
>  	local msg="$1"
>  	local expected=$2
>  
> -	ip netns exec $BASE$SRC ping -qc 1 $BM_NET_V4$DST >/dev/null
> +	# Taking veth$SRC down drops the route, so set it for every check

What takes this down?

> +	ip -n $NS_SRC route replace ${RCV_NET_V4}0/24 via $BM_NET_V4$DST
> +	ip netns exec $BASE$SRC ping -qc 1 $RCV_NET_V4$RCV >/dev/null
>  	NSTAT_HISTORY=$STATS ip netns exec $NS_DST nstat -n
>  
>  	printf "%-60s" "$msg"
> -	ip netns exec $BASE$DST ./udpgso_bench_rx -C 1000 -R 10 &
> +	ip netns exec $NS_RCV ./udpgso_bench_rx -C 1000 -R 10 &
>  	local spid=$!
>  	sleep 0.1
>  
> -	ip netns exec $NS_SRC ./udpgso_bench_tx -4 -s 13000 -S 1300 -M 1 -D $BM_NET_V4$DST
> +	ip netns exec $NS_SRC ./udpgso_bench_tx -4 -s 13000 -S 1300 -M 1 -D $RCV_NET_V4$RCV
>  	local retc=$?
>  	wait $spid
>  	local rets=$?
> -- 
> 2.56.0
> 

Re: [PATCH] selftests: net: veth: check UDP GRO forwarding on forwarded traffic

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1 (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] chk_gro() in tools/testing/selftests/net/veth.sh reads
  IpInReceives for the whole NS_DST namespace.
- [Low] create_ns() in tools/testing/selftests/net/veth.sh creates the
  new veth pair in the invoking (host) netns under the fixed names…

Pre-existing issues:
- [Low] In tools/testing/selftests/net/veth.sh, chk_gro() now sends the
  GRO check traffic NS_SRC -> NS_DST -> NS_RCV.

--

selftests: net: veth: check UDP GRO forwarding on forwarded traffic

The rx-udp-gro-forwarding checks in veth.sh fail when the host has a UDP
tunnel socket. In that case GRO does a socket lookup and finds the local
receiver. This patch adds a third namespace behind the destination, so
the datagrams are forwarded instead of delivered locally, and keeps
counting IpInReceives in the destination namespace.

Link: https://lore.kernel.org/netdev/CAF=yD-LZSec9g_0bBgrcQvq2OO1BrxMSRHH=uU7eamCmWcTO_A@mail.gmail.com/

> diff --git a/tools/testing/selftests/net/veth.sh b/tools/testing/selftests/net/veth.sh
> index 17cfe412a2667..fef4ab587c91a 100755
> --- a/tools/testing/selftests/net/veth.sh
> +++ b/tools/testing/selftests/net/veth.sh

[ ... ]

> @@ -48,6 +53,17 @@ create_ns() {
>  		ip -n $BASE$ns addr add dev veth$ns $BM_NET_V4$ns/24
>  		ip -n $BASE$ns addr add dev veth$ns $BM_NET_V6$ns/64 nodad
>  	done
> +
> +	# Forwarded, the datagrams find no local socket, whatever tunnel the host has
> +	ip netns add $NS_RCV
> +	ip -n $NS_RCV link set dev lo up
> +	ip link add name fwd$DST type veth peer name fwd$RCV
> +	ip link set dev fwd$DST netns $NS_DST up
> +	ip link set dev fwd$RCV netns $NS_RCV up

[Severity: Low]
This pair is created in the namespace the script runs in, under the fixed
names fwd1 and fwd3, and none of these commands are checked. What happens
if the host already has an interface named fwd1 or fwd3?

In that case ip link add fails with EEXIST. The following "ip link set dev
fwd$DST netns $NS_DST up" then looks the name up in the host netns and
moves the host's own device into the test namespace.

cleanup() later runs ip netns del on that namespace. A virtual device is
destroyed, and a physical one goes back to init_net without its
configuration.

Also, if ip netns add $NS_RCV fails but the link creation succeeds, the
moves fail and the pair stays in the host netns. cleanup() only deletes
namespaces, so the pair would leak.

The existing veth$SRC/veth$DST setup above uses the same pattern. Could the
new pair be created directly inside the namespaces instead, for example:

    ip -n $NS_DST link add fwd$DST type veth peer name fwd$RCV netns $NS_RCV

> +	ip -n $NS_DST addr add dev fwd$DST $RCV_NET_V4$DST/24
> +	ip -n $NS_RCV addr add dev fwd$RCV $RCV_NET_V4$RCV/24
> +	ip netns exec $NS_DST sysctl -wq net.ipv4.ip_forward=1
> +	ip -n $NS_RCV route add default via $RCV_NET_V4$DST
>  }
>  
>  __chk_flag() {
> @@ -134,15 +150,17 @@ chk_gro() {
>  	local msg="$1"
>  	local expected=$2
>  
> -	ip netns exec $BASE$SRC ping -qc 1 $BM_NET_V4$DST >/dev/null
> +	# Taking veth$SRC down drops the route, so set it for every check
> +	ip -n $NS_SRC route replace ${RCV_NET_V4}0/24 via $BM_NET_V4$DST
> +	ip netns exec $BASE$SRC ping -qc 1 $RCV_NET_V4$RCV >/dev/null
>  	NSTAT_HISTORY=$STATS ip netns exec $NS_DST nstat -n
>  
>  	printf "%-60s" "$msg"
> -	ip netns exec $BASE$DST ./udpgso_bench_rx -C 1000 -R 10 &
> +	ip netns exec $NS_RCV ./udpgso_bench_rx -C 1000 -R 10 &

[Severity: Low]
This isn't a bug introduced by this patch, but the receiver is still
started without -n, so cfg_expected_pkt_nr stays 0. do_recv() in
udpgso_bench_rx.c only checks the count when that is set:

    if (cfg_expected_pkt_nr && (packets != cfg_expected_pkt_nr))

On timeout the receiver just exits 0, even if nothing arrived.

The only count chk_gro() compares is IpInReceives in NS_DST. ip_rcv_core()
increments it before any routing decision:

    __IP_UPD_PO_STATS(net, IPSTATS_MIB_IN, skb->len);

With the new forwarding hop, nothing confirms that NS_DST actually
forwarded the aggregated skb, or that NS_RCV received the 10 datagrams. If
the GRO skb were dropped in ip_forward() or during egress segmentation, the
aggregation checks would still pass.

Would adding -n 10 to the receiver, or also checking IpForwDatagrams, close
this gap?

>  	local spid=$!
>  	sleep 0.1

[Severity: Medium]
Can this sleep now cause a spurious GRO failure?

Only sleep 0.1 makes the receiver bind() before the sender transmits. If
udpgso_bench_rx in NS_RCV has not bound its port by the time the forwarded
datagram arrives, the path would be:

NS_RCV:
  __udp4_lib_rcv()
    icmp_send(skb, ICMP_DEST_UNREACH, ICMP_PORT_UNREACH, 0);

The ICMP then follows the default route added in create_ns() back into
NS_DST on fwd1:

NS_DST:
  ip_rcv_core()
    __IP_UPD_PO_STATS(net, IPSTATS_MIB_IN, skb->len);

nstat reads IpInReceives for the whole NS_DST namespace, so this ICMP is
counted along with the forward-direction datagrams on veth1.

Neither helper turns this into a non-zero exit. udpgso_bench_tx -M 1 sends
one message and exits 0. udpgso_bench_rx without -n times out and exits 0.
chk_gro() would then report "got 2 packets, expected 1" (or 11 vs 10).

Before this patch the receiver was in NS_DST, so the same race only produced
an outgoing ICMP that IpInReceives does not count.

Would it be more robust to wait for the receiver socket to be bound, or to
count only ingress on veth$DST?

>  
> -	ip netns exec $NS_SRC ./udpgso_bench_tx -4 -s 13000 -S 1300 -M 1 -D $BM_NET_V4$DST
> +	ip netns exec $NS_SRC ./udpgso_bench_tx -4 -s 13000 -S 1300 -M 1 -D $RCV_NET_V4$RCV
>  	local retc=$?
>  	wait $spid
>  	local rets=$?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026122440.603302-1-eva.kurchatova%40virtuozzo.com