[PATCH net 0/2] net: gro: fix outer network offset

[PATCH net 0/2] net: gro: fix outer network offset

[Paolo Abeni]

The GRO stage has still problems in some edge scenarios when dealing
with encapsulated traffic: the first patch address an issue there
leading to bad checksums.

The second patch introduces a test case covering the relevant scenario:
S/W segmentation after GRO for encapsulated traffic.

Paolo Abeni (2):
  net: gro: fix outer network offset
  selftest: net: add a test-case for encap segmentation after GRO

 net/core/gro.c                            |  2 +
 tools/testing/selftests/net/udpgro_fwd.sh | 65 +++++++++++++++++++++++
 2 files changed, 67 insertions(+)

-- 
2.52.0

[PATCH net 1/2] net: gro: fix outer network offset

[Paolo Abeni]

The udp GRO complete stage assumes that all the packets inserted the RX
have the `encapsulation` flag zeroed. Such assumption is not true, as a
few H/W NICs can set such flag when H/W offloading the checksum for
an UDP encapsulated traffic, the tun driver can inject GSO packets with
UDP encapsulation and the problematic layout can also be created via
a veth based setup.

Due to the above, in the problematic scenarios, udp4_gro_complete() uses
the wrong network offset (inner instead of outer) to compute the outer
UDP header pseudo checksum, leading to csum validation errors later on
in packet processing.

Address the issue always clearing the encapsulation flag at GRO completion
time. Such flag will be set again as needed for encapsulated packets by
udp_gro_complete().

Fixes: 5ef31ea5d053 ("net: gro: fix udp bad offset in socket lookup by adding {inner_}network_offset to napi_gro_cb")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/core/gro.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/gro.c b/net/core/gro.c
index 76f9c3712422..482fa7d7f598 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -265,6 +265,8 @@ static void gro_complete(struct gro_node *gro, struct sk_buff *skb)
 		goto out;
 	}
 
+	/* NICs can feed encapsulated packets into GRO */
+	skb->encapsulation = 0;
 	rcu_read_lock();
 	list_for_each_entry_rcu(ptype, head, list) {
 		if (ptype->type != type || !ptype->callbacks.gro_complete)
-- 
2.52.0

Re: [PATCH net 1/2] net: gro: fix outer network offset

[Willem de Bruijn]

Paolo Abeni wrote:
> The udp GRO complete stage assumes that all the packets inserted the RX
> have the `encapsulation` flag zeroed. Such assumption is not true, as a
> few H/W NICs can set such flag when H/W offloading the checksum for
> an UDP encapsulated traffic, the tun driver can inject GSO packets with
> UDP encapsulation and the problematic layout can also be created via
> a veth based setup.
> 
> Due to the above, in the problematic scenarios, udp4_gro_complete() uses
> the wrong network offset (inner instead of outer) to compute the outer
> UDP header pseudo checksum, leading to csum validation errors later on
> in packet processing.
> 
> Address the issue always clearing the encapsulation flag at GRO completion
> time. Such flag will be set again as needed for encapsulated packets by
> udp_gro_complete().
> 
> Fixes: 5ef31ea5d053 ("net: gro: fix udp bad offset in socket lookup by adding {inner_}network_offset to napi_gro_cb")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Reviewed-by: Willem de Bruijn <willemb@google.com> 

[PATCH net 2/2] selftest: net: add a test-case for encap segmentation after GRO

[Paolo Abeni]

We had a few patches in this area and no explicit coverage so far.
The test case covers the scenario addressed by the previous fix;
reusing the existing udpgro_fwd.sh script to leverage part of the
of the virtual network setup, even if such script is possibly not
a perfect fit.

Note that the mentioned script already contains several shellcheck
violation; this patch does not fix the existing code, just avoids
adding more issues in the new one.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 tools/testing/selftests/net/udpgro_fwd.sh | 65 +++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/tools/testing/selftests/net/udpgro_fwd.sh b/tools/testing/selftests/net/udpgro_fwd.sh
index a39fdc4aa2ff..f2c1f0630b77 100755
--- a/tools/testing/selftests/net/udpgro_fwd.sh
+++ b/tools/testing/selftests/net/udpgro_fwd.sh
@@ -162,6 +162,39 @@ run_test() {
 	echo " ok"
 }
 
+run_test_csum() {
+	local -r msg="$1"
+	local -r dst="$2"
+	local csum_error_filter=UdpInCsumErrors
+	local csum_errors
+
+	printf "%-40s" "$msg"
+
+	is_ipv6 "$dst" && csum_error_filter=Udp6InCsumErrors
+
+	ip netns exec "$NS_DST" iperf3 -s -1 >/dev/null &
+	wait_local_port_listen "$NS_DST" 5201 tcp
+	local spid="$!"
+	ip netns exec "$NS_SRC" iperf3 -c "$dst" -t 2 >/dev/null
+	local retc="$?"
+	wait "$spid"
+	local rets="$?"
+	if [ "$rets" -ne 0 ] || [ "$retc" -ne 0 ]; then
+		echo " fail client exit code $retc, server $rets"
+		ret=1
+		return
+	fi
+
+	csum_errors=$(ip netns exec "$NS_DST" nstat -as "$csum_error_filter" |
+		      grep "$csum_error_filter" | awk '{print $2}')
+	if [ -n "$csum_errors" ] && [ "$csum_errors" -gt 0 ]; then
+		echo " fail - csum error on receive $csum_errors, expected 0"
+		ret=1
+		return
+	fi
+	echo " ok"
+}
+
 run_bench() {
 	local -r msg=$1
 	local -r dst=$2
@@ -260,6 +293,38 @@ for family in 4 6; do
 	ip netns exec $NS_SRC $PING -q -c 1 $OL_NET$DST_NAT >/dev/null
 	run_test "GRO fwd over UDP tunnel" $OL_NET$DST_NAT 10 10 $OL_NET$DST
 	cleanup
+
+	# force segmentation and re-aggregation
+	create_vxlan_pair
+	ip netns exec "$NS_DST" ethtool -K veth"$DST" generic-receive-offload on
+	ip netns exec "$NS_SRC" ethtool -K veth"$SRC" tso off
+	ip -n "$NS_SRC" link set dev veth"$SRC" mtu 1430
+
+	# forward to a 2nd veth pair
+	ip -n "$NS_DST" link add br0 type bridge
+	ip -n "$NS_DST" link set dev veth"$DST" master br0
+
+	# segment the aggregates TSO packet, without csum offload
+	ip -n "$NS_DST" link add veth_segment type veth peer veth_rx
+	for FEATURE in tso tx-udp-segmentation tx-checksumming; do
+		ip netns exec "$NS_DST" ethtool -K veth_segment "$FEATURE" off
+	done
+	ip -n "$NS_DST" link set dev veth_segment master br0 up
+	ip -n "$NS_DST" link set dev br0 up
+	ip -n "$NS_DST" link set dev veth_rx up
+
+	# move the lower layer IP in the last added veth
+	for ADDR in "$BM_NET_V4$DST/24" "$BM_NET_V6$DST/64"; do
+		# the dad argument will let iproute emit a unharmfull warning
+		# with ipv4 addresses
+		ip -n "$NS_DST" addr del dev veth"$DST" "$ADDR" \
+			nodad 2>/dev/null
+		ip -n "$NS_DST" addr add dev veth_rx "$ADDR" \
+			nodad 2>/dev/null
+	done
+
+	run_test_csum "GSO after GRO" "$OL_NET$DST"
+	cleanup
 done
 
 exit $ret
-- 
2.52.0

Re: [PATCH net 2/2] selftest: net: add a test-case for encap segmentation after GRO

[Willem de Bruijn]

Paolo Abeni wrote:
> We had a few patches in this area and no explicit coverage so far.
> The test case covers the scenario addressed by the previous fix;
> reusing the existing udpgro_fwd.sh script to leverage part of the
> of the virtual network setup, even if such script is possibly not
> a perfect fit.
> 
> Note that the mentioned script already contains several shellcheck
> violation; this patch does not fix the existing code, just avoids
> adding more issues in the new one.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
>  tools/testing/selftests/net/udpgro_fwd.sh | 65 +++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/udpgro_fwd.sh b/tools/testing/selftests/net/udpgro_fwd.sh
> index a39fdc4aa2ff..f2c1f0630b77 100755
> --- a/tools/testing/selftests/net/udpgro_fwd.sh
> +++ b/tools/testing/selftests/net/udpgro_fwd.sh
> @@ -162,6 +162,39 @@ run_test() {
>  	echo " ok"
>  }
>  
> +run_test_csum() {
> +	local -r msg="$1"
> +	local -r dst="$2"
> +	local csum_error_filter=UdpInCsumErrors
> +	local csum_errors
> +
> +	printf "%-40s" "$msg"
> +
> +	is_ipv6 "$dst" && csum_error_filter=Udp6InCsumErrors
> +
> +	ip netns exec "$NS_DST" iperf3 -s -1 >/dev/null &
> +	wait_local_port_listen "$NS_DST" 5201 tcp
> +	local spid="$!"
> +	ip netns exec "$NS_SRC" iperf3 -c "$dst" -t 2 >/dev/null
> +	local retc="$?"
> +	wait "$spid"
> +	local rets="$?"
> +	if [ "$rets" -ne 0 ] || [ "$retc" -ne 0 ]; then
> +		echo " fail client exit code $retc, server $rets"
> +		ret=1
> +		return
> +	fi
> +
> +	csum_errors=$(ip netns exec "$NS_DST" nstat -as "$csum_error_filter" |
> +		      grep "$csum_error_filter" | awk '{print $2}')
> +	if [ -n "$csum_errors" ] && [ "$csum_errors" -gt 0 ]; then
> +		echo " fail - csum error on receive $csum_errors, expected 0"
> +		ret=1
> +		return
> +	fi
> +	echo " ok"
> +}
> +
>  run_bench() {
>  	local -r msg=$1
>  	local -r dst=$2
> @@ -260,6 +293,38 @@ for family in 4 6; do
>  	ip netns exec $NS_SRC $PING -q -c 1 $OL_NET$DST_NAT >/dev/null
>  	run_test "GRO fwd over UDP tunnel" $OL_NET$DST_NAT 10 10 $OL_NET$DST
>  	cleanup
> +
> +	# force segmentation and re-aggregation
> +	create_vxlan_pair
> +	ip netns exec "$NS_DST" ethtool -K veth"$DST" generic-receive-offload on
> +	ip netns exec "$NS_SRC" ethtool -K veth"$SRC" tso off
> +	ip -n "$NS_SRC" link set dev veth"$SRC" mtu 1430
> +
> +	# forward to a 2nd veth pair
> +	ip -n "$NS_DST" link add br0 type bridge
> +	ip -n "$NS_DST" link set dev veth"$DST" master br0
> +
> +	# segment the aggregates TSO packet, without csum offload
> +	ip -n "$NS_DST" link add veth_segment type veth peer veth_rx
> +	for FEATURE in tso tx-udp-segmentation tx-checksumming; do
> +		ip netns exec "$NS_DST" ethtool -K veth_segment "$FEATURE" off
> +	done

fwiw, tx off will disable all the dependent tx offloads too

> +	ip -n "$NS_DST" link set dev veth_segment master br0 up
> +	ip -n "$NS_DST" link set dev br0 up
> +	ip -n "$NS_DST" link set dev veth_rx up
> +
> +	# move the lower layer IP in the last added veth
> +	for ADDR in "$BM_NET_V4$DST/24" "$BM_NET_V6$DST/64"; do
> +		# the dad argument will let iproute emit a unharmfull warning

unimportant but minor typo: unharmful. Also above aggregates -> aggregated

> +		# with ipv4 addresses
> +		ip -n "$NS_DST" addr del dev veth"$DST" "$ADDR" \
> +			nodad 2>/dev/null

nodad probably only matters on add?

> +		ip -n "$NS_DST" addr add dev veth_rx "$ADDR" \
> +			nodad 2>/dev/null
> +	done
> +
> +	run_test_csum "GSO after GRO" "$OL_NET$DST"
> +	cleanup
>  done
>  
>  exit $ret
> -- 
> 2.52.0
> 

Re: [PATCH net 2/2] selftest: net: add a test-case for encap segmentation after GRO

[Paolo Abeni]

On 2/1/26 11:19 PM, Willem de Bruijn wrote:
> Paolo Abeni wrote:
>> @@ -260,6 +293,38 @@ for family in 4 6; do
>>  	ip netns exec $NS_SRC $PING -q -c 1 $OL_NET$DST_NAT >/dev/null
>>  	run_test "GRO fwd over UDP tunnel" $OL_NET$DST_NAT 10 10 $OL_NET$DST
>>  	cleanup
>> +
>> +	# force segmentation and re-aggregation
>> +	create_vxlan_pair
>> +	ip netns exec "$NS_DST" ethtool -K veth"$DST" generic-receive-offload on
>> +	ip netns exec "$NS_SRC" ethtool -K veth"$SRC" tso off
>> +	ip -n "$NS_SRC" link set dev veth"$SRC" mtu 1430
>> +
>> +	# forward to a 2nd veth pair
>> +	ip -n "$NS_DST" link add br0 type bridge
>> +	ip -n "$NS_DST" link set dev veth"$DST" master br0
>> +
>> +	# segment the aggregates TSO packet, without csum offload
>> +	ip -n "$NS_DST" link add veth_segment type veth peer veth_rx
>> +	for FEATURE in tso tx-udp-segmentation tx-checksumming; do
>> +		ip netns exec "$NS_DST" ethtool -K veth_segment "$FEATURE" off
>> +	done
> 
> fwiw, tx off will disable all the dependent tx offloads too

Going over the features in this order is intentional to prevent ethtool
from emitting messages about the additional features disabled by
tx-checksumming.

>> +	ip -n "$NS_DST" link set dev veth_segment master br0 up
>> +	ip -n "$NS_DST" link set dev br0 up
>> +	ip -n "$NS_DST" link set dev veth_rx up
>> +
>> +	# move the lower layer IP in the last added veth
>> +	for ADDR in "$BM_NET_V4$DST/24" "$BM_NET_V6$DST/64"; do
>> +		# the dad argument will let iproute emit a unharmfull warning
> 
> unimportant but minor typo: unharmful. Also above aggregates -> aggregated

These ones were not intentional :)

> 
>> +		# with ipv4 addresses
>> +		ip -n "$NS_DST" addr del dev veth"$DST" "$ADDR" \
>> +			nodad 2>/dev/null
> 
> nodad probably only matters on add?

uhm... yes, left-over from c&p.

There is no rush, let me send a v2 addressing the above. Thanks!

Paolo