[PATCH net 0/2] Fix bad offload warning when sending UDP GSO from a tunnel device

[PATCH net 0/2] Fix bad offload warning when sending UDP GSO from a tunnel device

[Jakub Sitnicki]

This series addresses a recent regression report from syzbot [1].
Please see patch 1 description for details.

[1] https://lore.kernel.org/all/000000000000e1609a061d5330ce@google.com/

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
Jakub Sitnicki (2):
      udp: Mark GSO packets as CHECKSUM_UNNECESSARY early on on output
      selftests/net: Add coverage for UDP GSO with egress from tunnel

 net/ipv4/udp.c                        |  7 ++++++
 net/ipv4/udp_offload.c                |  8 -------
 net/ipv6/udp.c                        |  7 ++++++
 tools/testing/selftests/net/udpgso.sh | 41 ++++++++++++++++++++++++++++++++---
 4 files changed, 52 insertions(+), 11 deletions(-)

[PATCH net 1/2] udp: Mark GSO packets as CHECKSUM_UNNECESSARY early on on output

[Jakub Sitnicki]

In commit 10154dbded6d ("udp: Allow GSO transmit from devices with no
checksum offload") we have added a tweak in the UDP GSO code to mark GSO
packets being sent out as CHECKSUM_UNNECESSARY when the egress device
doesn't support checksum offload. This was done to satisfy the offload
checks in the gso stack.

However, when sending a UDP GSO packet from a tunnel device, we will go
through the TX path and the GSO offload twice. Once for the tunnel device,
which acts as a passthru for GSO packets, and once for the underlying
egress device.

Even though a tunnel device acts as a passthru for a UDP GSO packet, GSO
offload checks still happen on transmit from a tunnel device. So if the skb
is not marked as CHECKSUM_UNNECESSARY or CHECKSUM_PARTIAL, we will get a
warning from the gso stack.

Today this can occur in two situations, which we check for in
__ip_append_data() and __ip6_append_data():

1) when the tunnel device does not advertise checksum offload, or
2) when there are IPv6 extension headers present.

Syzbot has triggered the second case, producing a warning as below:

  ip6tnl0: caps=(0x00000006401d7869, 0x00000006401d7869)
  WARNING: CPU: 0 PID: 5112 at net/core/dev.c:3293 skb_warn_bad_offload+0x166/0x1a0 net/core/dev.c:3291
  Modules linked in:
  CPU: 0 PID: 5112 Comm: syz-executor391 Not tainted 6.10.0-rc7-syzkaller-01603-g80ab5445da62 #0
  Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 06/07/2024
  RIP: 0010:skb_warn_bad_offload+0x166/0x1a0 net/core/dev.c:3291
  [...]
  Call Trace:
   <TASK>
   __skb_gso_segment+0x3be/0x4c0 net/core/gso.c:127
   skb_gso_segment include/net/gso.h:83 [inline]
   validate_xmit_skb+0x585/0x1120 net/core/dev.c:3661
   __dev_queue_xmit+0x17a4/0x3e90 net/core/dev.c:4415
   neigh_output include/net/neighbour.h:542 [inline]
   ip6_finish_output2+0xffa/0x1680 net/ipv6/ip6_output.c:137
   ip6_finish_output+0x41e/0x810 net/ipv6/ip6_output.c:222
   ip6_send_skb+0x112/0x230 net/ipv6/ip6_output.c:1958
   udp_v6_send_skb+0xbf5/0x1870 net/ipv6/udp.c:1292
   udpv6_sendmsg+0x23b3/0x3270 net/ipv6/udp.c:1588
   sock_sendmsg_nosec net/socket.c:730 [inline]
   __sock_sendmsg+0xef/0x270 net/socket.c:745
   ____sys_sendmsg+0x525/0x7d0 net/socket.c:2585
   ___sys_sendmsg net/socket.c:2639 [inline]
   __sys_sendmmsg+0x3b2/0x740 net/socket.c:2725
   __do_sys_sendmmsg net/socket.c:2754 [inline]
   __se_sys_sendmmsg net/socket.c:2751 [inline]
   __x64_sys_sendmmsg+0xa0/0xb0 net/socket.c:2751
   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
   do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
   entry_SYSCALL_64_after_hwframe+0x77/0x7f
   [...]
   </TASK>

To fix it mark UDP_GSO packets as CHECKSUM_UNNECESSARY early on the TX
path, when still in the udp layer, since we need to have ip_summed set up
correctly for GSO processing by tunnel devices.

Note that even if GSO packet gets marked as CHECKSUM_PARTIAL due to tunnel
advertising HW csum offload, it will not prevent software csum offload in
UDP GSO from kicking in if the underlying device doesn't offer csum
offload (for example, a TUN/TAP device with default config). This is
because we recheck device features in gso stack instead relying on the
ip_summed hint.

Fixes: 10154dbded6d ("udp: Allow GSO transmit from devices with no checksum offload")
Reported-by: syzbot+e15b7e15b8a751a91d9a@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/000000000000e1609a061d5330ce@google.com/
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/ipv4/udp.c         | 7 +++++++
 net/ipv4/udp_offload.c | 8 --------
 net/ipv6/udp.c         | 7 +++++++
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 49c622e743e8..b7254b8a1e56 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -946,6 +946,13 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
 		}
 
 		if (datalen > cork->gso_size) {
+			/* On the TX path CHECKSUM_NONE and CHECKSUM_UNNECESSARY
+			 * have the same meaning. However, check for bad
+			 * offloads in the GSO stack expects the latter, if the
+			 * checksum can be calculated in software.
+			 */
+			if (skb->ip_summed == CHECKSUM_NONE)
+				skb->ip_summed = CHECKSUM_UNNECESSARY;
 			skb_shinfo(skb)->gso_size = cork->gso_size;
 			skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
 			skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(datalen,
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index aa2e0a28ca61..59448a2dbf2c 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -357,14 +357,6 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 	else
 		uh->check = gso_make_checksum(seg, ~check) ? : CSUM_MANGLED_0;
 
-	/* On the TX path, CHECKSUM_NONE and CHECKSUM_UNNECESSARY have the same
-	 * meaning. However, check for bad offloads in the GSO stack expects the
-	 * latter, if the checksum was calculated in software. To vouch for the
-	 * segment skbs we actually need to set it on the gso_skb.
-	 */
-	if (gso_skb->ip_summed == CHECKSUM_NONE)
-		gso_skb->ip_summed = CHECKSUM_UNNECESSARY;
-
 	/* update refcount for the packet */
 	if (copy_dtor) {
 		int delta = sum_truesize - gso_skb->truesize;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 6602a2e9cdb5..360392fc2b68 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1262,6 +1262,13 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
 		}
 
 		if (datalen > cork->gso_size) {
+			/* On the TX path CHECKSUM_NONE and CHECKSUM_UNNECESSARY
+			 * have the same meaning. However, check for bad
+			 * offloads in the GSO stack expects the latter, if the
+			 * checksum can be calculated in software.
+			 */
+			if (skb->ip_summed == CHECKSUM_NONE)
+				skb->ip_summed = CHECKSUM_UNNECESSARY;
 			skb_shinfo(skb)->gso_size = cork->gso_size;
 			skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
 			skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(datalen,

-- 
2.40.1

[PATCH net 2/2] selftests/net: Add coverage for UDP GSO with egress from tunnel

[Jakub Sitnicki]

After enabling UDP GSO for devices not offering checksum offload, we have
hit a regression where a bad offload warning can be triggered if egressing
through a tunnel device.

This can happen when the tunnel device has checksum offload disabled or
when IPv6 extension headers are present.

Extend the UDP GSO tests to cover the egress from a tunnel device scenario.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 tools/testing/selftests/net/udpgso.sh | 41 ++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/udpgso.sh b/tools/testing/selftests/net/udpgso.sh
index 85d1fa3c1ff7..3fb6fea06b28 100755
--- a/tools/testing/selftests/net/udpgso.sh
+++ b/tools/testing/selftests/net/udpgso.sh
@@ -28,9 +28,13 @@ test_route_mtu() {
 }
 
 setup_dummy_sink() {
-	ip link add name sink mtu 1500 type dummy
-	ip addr add dev sink 10.0.0.0/24
-	ip addr add dev sink fd00::2/64 nodad
+	mtu="${1:-1500}"
+	prefix4="${2:-10.0.0.2/24}"
+	prefix6="${3:-fd00::2/48}"
+
+	ip link add name sink mtu "${mtu}" type dummy
+	ip addr add dev sink "${prefix4}"
+	ip addr add dev sink "${prefix6}" nodad
 	ip link set dev sink up
 }
 
@@ -52,6 +56,25 @@ test_sw_gso_sw_csum() {
 	ethtool -K sink tx-udp-segmentation off >/dev/null
 }
 
+setup_ipip_tunnel() {
+	setup_dummy_sink 1520 10.1.1.2/24 fd11::2/48
+
+	ip tunnel add iptnl mode ipip local 10.1.1.2 remote 10.1.1.1
+	ip addr add dev iptnl 10.0.0.2/24
+	ip addr add dev iptnl fd00::2/48 nodad
+	ip link set dev iptnl up
+}
+
+test_tunnel_hw_csum() {
+	setup_ipip_tunnel
+	ethtool -K iptnl tx-checksum-ip-generic on >/dev/null
+}
+
+test_tunnel_sw_csum() {
+	setup_ipip_tunnel
+	ethtool -K iptnl tx-checksum-ip-generic off >/dev/null
+}
+
 if [ "$#" -gt 0 ]; then
 	"$1"
 	shift 2 # pop "test_*" arg and "--" delimiter
@@ -99,3 +122,15 @@ echo "ipv4 sw-gso sw-csum"
 
 echo "ipv6 sw-gso sw-csum"
 ./in_netns.sh "$0" test_sw_gso_sw_csum -- ./udpgso -6 -C -R
+
+echo "ipv4 tunnel hw-csum"
+./in_netns.sh "$0" test_tunnel_hw_csum -- ./udpgso -4 -C -R
+
+echo "ipv6 tunnel hw-csum"
+./in_netns.sh "$0" test_tunnel_hw_csum -- ./udpgso -6 -C -R
+
+echo "ipv4 tunnel sw-csum"
+./in_netns.sh "$0" test_tunnel_sw_csum -- ./udpgso -4 -C -R
+
+echo "ipv6 tunnel sw-csum"
+./in_netns.sh "$0" test_tunnel_sw_csum -- ./udpgso -6 -C -R

-- 
2.40.1

Re: [PATCH net 1/2] udp: Mark GSO packets as CHECKSUM_UNNECESSARY early on on output

[Willem de Bruijn]

On Thu, Jul 25, 2024 at 5:56 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> In commit 10154dbded6d ("udp: Allow GSO transmit from devices with no
> checksum offload") we have added a tweak in the UDP GSO code to mark GSO
> packets being sent out as CHECKSUM_UNNECESSARY when the egress device
> doesn't support checksum offload. This was done to satisfy the offload
> checks in the gso stack.
>
> However, when sending a UDP GSO packet from a tunnel device, we will go
> through the TX path and the GSO offload twice. Once for the tunnel device,
> which acts as a passthru for GSO packets, and once for the underlying
> egress device.
>
> Even though a tunnel device acts as a passthru for a UDP GSO packet, GSO
> offload checks still happen on transmit from a tunnel device. So if the skb
> is not marked as CHECKSUM_UNNECESSARY or CHECKSUM_PARTIAL, we will get a
> warning from the gso stack.

I don't entirely understand. The check should not hit on pass through,
where segs == skb:

        if (segs != skb && unlikely(skb_needs_check(skb, tx_path) &&
!IS_ERR(segs)))
                skb_warn_bad_offload(skb);

> Today this can occur in two situations, which we check for in
> __ip_append_data() and __ip6_append_data():
>
> 1) when the tunnel device does not advertise checksum offload, or
> 2) when there are IPv6 extension headers present.
>
> To fix it mark UDP_GSO packets as CHECKSUM_UNNECESSARY early on the TX
> path, when still in the udp layer, since we need to have ip_summed set up
> correctly for GSO processing by tunnel devices.

The previous patch converted segments post segmentation to
CHECKSUM_UNNECESSARY, which is fine as they had
already been checksummed in software, and CHECKSUM_NONE
packets on egress are common.

This creates GSO packets without CHECKSUM_PARTIAL.
Segmentation offload always requires checksum offload. So these
would be weird new packets. And having CHECKSUM_NONE (or
equivalent), but entering software checksumming is also confusing.

The crux is that I don't understand why the warning fires on tunnel
exit when no segmentation takes place there. Hopefully we can fix
in a way that does not introduce these weird GSO packets (but if
not, so be it).

Re: [PATCH net 1/2] udp: Mark GSO packets as CHECKSUM_UNNECESSARY early on on output

[Jakub Sitnicki]

On Thu, Jul 25, 2024 at 10:21 AM -04, Willem de Bruijn wrote:
> On Thu, Jul 25, 2024 at 5:56 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> In commit 10154dbded6d ("udp: Allow GSO transmit from devices with no
>> checksum offload") we have added a tweak in the UDP GSO code to mark GSO
>> packets being sent out as CHECKSUM_UNNECESSARY when the egress device
>> doesn't support checksum offload. This was done to satisfy the offload
>> checks in the gso stack.
>>
>> However, when sending a UDP GSO packet from a tunnel device, we will go
>> through the TX path and the GSO offload twice. Once for the tunnel device,
>> which acts as a passthru for GSO packets, and once for the underlying
>> egress device.
>>
>> Even though a tunnel device acts as a passthru for a UDP GSO packet, GSO
>> offload checks still happen on transmit from a tunnel device. So if the skb
>> is not marked as CHECKSUM_UNNECESSARY or CHECKSUM_PARTIAL, we will get a
>> warning from the gso stack.
>
> I don't entirely understand. The check should not hit on pass through,
> where segs == skb:
>
>         if (segs != skb && unlikely(skb_needs_check(skb, tx_path) &&
> !IS_ERR(segs)))
>                 skb_warn_bad_offload(skb);
>

That's something I should have explained better. Let me try to shed some
light on it now. We're hitting the skb_warn_bad_offload warning because
skb_mac_gso_segment doesn't return any segments (segs == NULL).

And that's because we bail out early out of __udp_gso_segment when we
detect that the tunnel device is capable of tx-udp-segmentation
(GSO_UDP_L4):

	if (skb_gso_ok(gso_skb, features | NETIF_F_GSO_ROBUST)) {
		/* Packet is from an untrusted source, reset gso_segs. */
		skb_shinfo(gso_skb)->gso_segs = DIV_ROUND_UP(gso_skb->len - sizeof(*uh),
							     mss);
		return NULL;
	}

It has not occurred to me before, but in the spirit of commit
8d74e9f88d65 "net: avoid skb_warn_bad_offload on IS_ERR" [1], we could
tighten the check to exclude cases when segs == NULL. I'm thinking of:

	if (segs != skb && !IS_ERR_OR_NULL(segs) && unlikely(skb_needs_check(skb, tx_path)))
		skb_warn_bad_offload(skb);

That would be an alternative. Though I'm not sure I understand the
consequences of such change fully yet. Namely if we're wouldn't be
losing some diagnostics from the bad offload warning.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8d74e9f88d65af8bb2e095aff506aa6eac755ada

>> Today this can occur in two situations, which we check for in
>> __ip_append_data() and __ip6_append_data():
>>
>> 1) when the tunnel device does not advertise checksum offload, or
>> 2) when there are IPv6 extension headers present.
>>
>> To fix it mark UDP_GSO packets as CHECKSUM_UNNECESSARY early on the TX
>> path, when still in the udp layer, since we need to have ip_summed set up
>> correctly for GSO processing by tunnel devices.
>
> The previous patch converted segments post segmentation to
> CHECKSUM_UNNECESSARY, which is fine as they had
> already been checksummed in software, and CHECKSUM_NONE
> packets on egress are common.
>
> This creates GSO packets without CHECKSUM_PARTIAL.
> Segmentation offload always requires checksum offload. So these
> would be weird new packets. And having CHECKSUM_NONE (or
> equivalent), but entering software checksumming is also confusing.

I agree this is confusing to reason about. That is a GSO packet with
CHECKSUM_UNNECESSARY which has not undergone segmentation and csum
offload in software.

Kind of related, I noticed that turning off tx-checksum-ip-generic with
ethtool doesn't disable tx-udp-segmentation. That looks like a bug.

> The crux is that I don't understand why the warning fires on tunnel
> exit when no segmentation takes place there. Hopefully we can fix
> in a way that does not introduce these weird GSO packets (but if
> not, so be it).

Attaching a self contained repro which I've been using to trace and
understand the GSO code:

---8<---

sh# cat repro-full.py
#!/bin/env python
#
# `modprobe ip6_tunnel` might be needed.
#

import os
import subprocess
import shutil
from socket import *

UDP_SEGMENT = 103

cmd = [shutil.which("ip"), "-batch", "/dev/stdin"]
script = b"""
link set dev lo up

link add name sink mtu 1540 type dummy
addr add dev sink fd11::2/48 nodad
link set dev sink up

tunnel add iptnl mode ip6ip6 remote fd11::1 local fd11::2 dev sink
link set dev iptnl mtu 1500
addr add dev iptnl fd00::2/48 nodad
link set dev iptnl up
"""
proc = subprocess.Popen(cmd, stdin=subprocess.PIPE)
proc.communicate(input=script)

os.system("ethtool -K sink tx-udp-segmentation off > /dev/null")
os.system("ethtool -K sink tx-checksum-ip-generic off > /dev/null")

# Alternatively to hopopts:
# os.system("ethtool -K iptnl tx-checksum-ip-generic off")

hopopts = b"\x00" * 8
s = socket(AF_INET6, SOCK_DGRAM)
s.setsockopt(IPPROTO_IPV6, IPV6_HOPOPTS, hopopts)
s.setsockopt(SOL_UDP, UDP_SEGMENT, 145)
s.sendto(b"x" * 3000, ("fd00::1", 9))
sh# perf ftrace -G __skb_gso_segment --graph-opts noirqs,depth=5 -- unshare -n python repro-full.py
# tracer: function_graph
#
# CPU  DURATION                  FUNCTION CALLS
# |     |   |                     |   |   |   |
 16)               |  __skb_gso_segment() {
 16)   0.288 us    |    irq_enter_rcu(); /* = 0xffffa00c03d89ac0 */
 16)   0.172 us    |    idle_cpu(); /* = 0x0 */
 16)               |    skb_mac_gso_segment() {
 16)   0.184 us    |      skb_network_protocol(); /* = 0xdd86 */
 16)   0.161 us    |      __rcu_read_lock(); /* = 0x2 */
 16)               |      ipv6_gso_segment() {
 16)               |        rcu_read_lock_held() {
 16)   0.151 us    |          rcu_lockdep_current_cpu_online(); /* = 0x1 */
 16)   0.514 us    |        } /* rcu_read_lock_held = 0x1 */
 16)               |        rcu_read_lock_held() {
 16)   0.152 us    |          rcu_lockdep_current_cpu_online(); /* = 0x1 */
 16)   0.459 us    |        } /* rcu_read_lock_held = 0x1 */
 16)               |        rcu_read_lock_held() {
 16)   0.151 us    |          rcu_lockdep_current_cpu_online(); /* = 0x1 */
 16)   0.459 us    |        } /* rcu_read_lock_held = 0x1 */
 16)               |        udp6_ufo_fragment() {
 16)   0.237 us    |          __udp_gso_segment(); /* = 0x0 */
 16)   0.727 us    |        } /* udp6_ufo_fragment = 0x0 */
 16)   3.049 us    |      } /* ipv6_gso_segment = 0x0 */
 16)   0.171 us    |      __rcu_read_unlock(); /* = 0x1 */
 16)   4.748 us    |    } /* skb_mac_gso_segment = 0x0 */
 16)               |    skb_warn_bad_offload() {
 [...]
 16) ! 785.215 us  |    } /* skb_warn_bad_offload = 0x0 */
 16) ! 800.986 us  |  } /* __skb_gso_segment = 0x0 */
 16)               |  __skb_gso_segment() {
 16)   0.394 us    |    irq_enter_rcu(); /* = 0xffffa00c03d89ac0 */
 16)   0.181 us    |    idle_cpu(); /* = 0x0 */
 16)               |    skb_mac_gso_segment() {
 16)   0.182 us    |      skb_network_protocol(); /* = 0xdd86 */
 16)   0.178 us    |      __rcu_read_lock(); /* = 0x3 */
 16)               |      ipv6_gso_segment() {
 16)               |        rcu_read_lock_held() {
 16)   0.155 us    |          rcu_lockdep_current_cpu_online(); /* = 0x1 */
 16)   0.556 us    |        } /* rcu_read_lock_held = 0x1 */
 16)               |        rcu_read_lock_held() {
 16)   0.159 us    |          rcu_lockdep_current_cpu_online(); /* = 0x1 */
 16)   0.480 us    |        } /* rcu_read_lock_held = 0x1 */
 16)               |        rcu_read_lock_held() {
 16)   0.159 us    |          rcu_lockdep_current_cpu_online(); /* = 0x1 */
 16)   0.480 us    |        } /* rcu_read_lock_held = 0x1 */
 16)               |        ip6ip6_gso_segment() {
 16) + 22.176 us   |          ipv6_gso_segment(); /* = 0xffffa00c03018c00 */
 16) + 24.875 us   |        } /* ip6ip6_gso_segment = 0xffffa00c03018c00 */
 16) + 27.416 us   |      } /* ipv6_gso_segment = 0xffffa00c03018c00 */
 16)   0.230 us    |      __rcu_read_unlock(); /* = 0x2 */
 16) + 29.065 us   |    } /* skb_mac_gso_segment = 0xffffa00c03018c00 */
 16) + 32.828 us   |  } /* __skb_gso_segment = 0xffffa00c03018c00 */
sh#

Re: [PATCH net 1/2] udp: Mark GSO packets as CHECKSUM_UNNECESSARY early on on output

[Willem de Bruijn]

On Fri, Jul 26, 2024 at 7:23 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> On Thu, Jul 25, 2024 at 10:21 AM -04, Willem de Bruijn wrote:
> > On Thu, Jul 25, 2024 at 5:56 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
> >>
> >> In commit 10154dbded6d ("udp: Allow GSO transmit from devices with no
> >> checksum offload") we have added a tweak in the UDP GSO code to mark GSO
> >> packets being sent out as CHECKSUM_UNNECESSARY when the egress device
> >> doesn't support checksum offload. This was done to satisfy the offload
> >> checks in the gso stack.
> >>
> >> However, when sending a UDP GSO packet from a tunnel device, we will go
> >> through the TX path and the GSO offload twice. Once for the tunnel device,
> >> which acts as a passthru for GSO packets, and once for the underlying
> >> egress device.
> >>
> >> Even though a tunnel device acts as a passthru for a UDP GSO packet, GSO
> >> offload checks still happen on transmit from a tunnel device. So if the skb
> >> is not marked as CHECKSUM_UNNECESSARY or CHECKSUM_PARTIAL, we will get a
> >> warning from the gso stack.
> >
> > I don't entirely understand. The check should not hit on pass through,
> > where segs == skb:
> >
> >         if (segs != skb && unlikely(skb_needs_check(skb, tx_path) &&
> > !IS_ERR(segs)))
> >                 skb_warn_bad_offload(skb);
> >
>
> That's something I should have explained better. Let me try to shed some
> light on it now. We're hitting the skb_warn_bad_offload warning because
> skb_mac_gso_segment doesn't return any segments (segs == NULL).
>
> And that's because we bail out early out of __udp_gso_segment when we
> detect that the tunnel device is capable of tx-udp-segmentation
> (GSO_UDP_L4):
>
>         if (skb_gso_ok(gso_skb, features | NETIF_F_GSO_ROBUST)) {
>                 /* Packet is from an untrusted source, reset gso_segs. */
>                 skb_shinfo(gso_skb)->gso_segs = DIV_ROUND_UP(gso_skb->len - sizeof(*uh),
>                                                              mss);
>                 return NULL;
>         }

Oh I see. Thanks.

> It has not occurred to me before, but in the spirit of commit
> 8d74e9f88d65 "net: avoid skb_warn_bad_offload on IS_ERR" [1], we could
> tighten the check to exclude cases when segs == NULL. I'm thinking of:
>
>         if (segs != skb && !IS_ERR_OR_NULL(segs) && unlikely(skb_needs_check(skb, tx_path)))
>                 skb_warn_bad_offload(skb);

That looks sensible to me. And nicer than the ip_summed conversion in
udp_send_skb.

> That would be an alternative. Though I'm not sure I understand the
> consequences of such change fully yet. Namely if we're wouldn't be
> losing some diagnostics from the bad offload warning.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8d74e9f88d65af8bb2e095aff506aa6eac755ada
>
> >> Today this can occur in two situations, which we check for in
> >> __ip_append_data() and __ip6_append_data():
> >>
> >> 1) when the tunnel device does not advertise checksum offload, or
> >> 2) when there are IPv6 extension headers present.
> >>
> >> To fix it mark UDP_GSO packets as CHECKSUM_UNNECESSARY early on the TX
> >> path, when still in the udp layer, since we need to have ip_summed set up
> >> correctly for GSO processing by tunnel devices.
> >
> > The previous patch converted segments post segmentation to
> > CHECKSUM_UNNECESSARY, which is fine as they had
> > already been checksummed in software, and CHECKSUM_NONE
> > packets on egress are common.
> >
> > This creates GSO packets without CHECKSUM_PARTIAL.
> > Segmentation offload always requires checksum offload. So these
> > would be weird new packets. And having CHECKSUM_NONE (or
> > equivalent), but entering software checksumming is also confusing.
>
> I agree this is confusing to reason about. That is a GSO packet with
> CHECKSUM_UNNECESSARY which has not undergone segmentation and csum
> offload in software.

I was mistaken earlier. Was looking at this code just yesterday too for

https://lore.kernel.org/netdev/20240726023359.879166-1-willemdebruijn.kernel@gmail.com/

We do set the GSO skb already skb CHECKSUM_NONE. So your suggestion is
not a significant change.

> Kind of related, I noticed that turning off tx-checksum-ip-generic with
> ethtool doesn't disable tx-udp-segmentation. That looks like a bug.

I saw the same :)

> > The crux is that I don't understand why the warning fires on tunnel
> > exit when no segmentation takes place there. Hopefully we can fix
> > in a way that does not introduce these weird GSO packets (but if
> > not, so be it).
>
> Attaching a self contained repro which I've been using to trace and
> understand the GSO code:
>
> ---8<---
>
> sh# cat repro-full.py
> #!/bin/env python
> #
> # `modprobe ip6_tunnel` might be needed.
> #
>
> import os
> import subprocess
> import shutil
> from socket import *
>
> UDP_SEGMENT = 103
>
> cmd = [shutil.which("ip"), "-batch", "/dev/stdin"]
> script = b"""
> link set dev lo up
>
> link add name sink mtu 1540 type dummy
> addr add dev sink fd11::2/48 nodad
> link set dev sink up
>
> tunnel add iptnl mode ip6ip6 remote fd11::1 local fd11::2 dev sink
> link set dev iptnl mtu 1500
> addr add dev iptnl fd00::2/48 nodad
> link set dev iptnl up
> """
> proc = subprocess.Popen(cmd, stdin=subprocess.PIPE)
> proc.communicate(input=script)
>
> os.system("ethtool -K sink tx-udp-segmentation off > /dev/null")
> os.system("ethtool -K sink tx-checksum-ip-generic off > /dev/null")
>
> # Alternatively to hopopts:
> # os.system("ethtool -K iptnl tx-checksum-ip-generic off")
>
> hopopts = b"\x00" * 8
> s = socket(AF_INET6, SOCK_DGRAM)
> s.setsockopt(IPPROTO_IPV6, IPV6_HOPOPTS, hopopts)
> s.setsockopt(SOL_UDP, UDP_SEGMENT, 145)
> s.sendto(b"x" * 3000, ("fd00::1", 9))
> sh# perf ftrace -G __skb_gso_segment --graph-opts noirqs,depth=5 -- unshare -n python repro-full.py
> # tracer: function_graph
> #
> # CPU  DURATION                  FUNCTION CALLS
> # |     |   |                     |   |   |   |
>  16)               |  __skb_gso_segment() {
>  16)   0.288 us    |    irq_enter_rcu(); /* = 0xffffa00c03d89ac0 */
>  16)   0.172 us    |    idle_cpu(); /* = 0x0 */
>  16)               |    skb_mac_gso_segment() {
>  16)   0.184 us    |      skb_network_protocol(); /* = 0xdd86 */
>  16)   0.161 us    |      __rcu_read_lock(); /* = 0x2 */
>  16)               |      ipv6_gso_segment() {
>  16)               |        rcu_read_lock_held() {
>  16)   0.151 us    |          rcu_lockdep_current_cpu_online(); /* = 0x1 */
>  16)   0.514 us    |        } /* rcu_read_lock_held = 0x1 */
>  16)               |        rcu_read_lock_held() {
>  16)   0.152 us    |          rcu_lockdep_current_cpu_online(); /* = 0x1 */
>  16)   0.459 us    |        } /* rcu_read_lock_held = 0x1 */
>  16)               |        rcu_read_lock_held() {
>  16)   0.151 us    |          rcu_lockdep_current_cpu_online(); /* = 0x1 */
>  16)   0.459 us    |        } /* rcu_read_lock_held = 0x1 */
>  16)               |        udp6_ufo_fragment() {
>  16)   0.237 us    |          __udp_gso_segment(); /* = 0x0 */
>  16)   0.727 us    |        } /* udp6_ufo_fragment = 0x0 */
>  16)   3.049 us    |      } /* ipv6_gso_segment = 0x0 */
>  16)   0.171 us    |      __rcu_read_unlock(); /* = 0x1 */
>  16)   4.748 us    |    } /* skb_mac_gso_segment = 0x0 */
>  16)               |    skb_warn_bad_offload() {
>  [...]
>  16) ! 785.215 us  |    } /* skb_warn_bad_offload = 0x0 */
>  16) ! 800.986 us  |  } /* __skb_gso_segment = 0x0 */
>  16)               |  __skb_gso_segment() {
>  16)   0.394 us    |    irq_enter_rcu(); /* = 0xffffa00c03d89ac0 */
>  16)   0.181 us    |    idle_cpu(); /* = 0x0 */
>  16)               |    skb_mac_gso_segment() {
>  16)   0.182 us    |      skb_network_protocol(); /* = 0xdd86 */
>  16)   0.178 us    |      __rcu_read_lock(); /* = 0x3 */
>  16)               |      ipv6_gso_segment() {
>  16)               |        rcu_read_lock_held() {
>  16)   0.155 us    |          rcu_lockdep_current_cpu_online(); /* = 0x1 */
>  16)   0.556 us    |        } /* rcu_read_lock_held = 0x1 */
>  16)               |        rcu_read_lock_held() {
>  16)   0.159 us    |          rcu_lockdep_current_cpu_online(); /* = 0x1 */
>  16)   0.480 us    |        } /* rcu_read_lock_held = 0x1 */
>  16)               |        rcu_read_lock_held() {
>  16)   0.159 us    |          rcu_lockdep_current_cpu_online(); /* = 0x1 */
>  16)   0.480 us    |        } /* rcu_read_lock_held = 0x1 */
>  16)               |        ip6ip6_gso_segment() {
>  16) + 22.176 us   |          ipv6_gso_segment(); /* = 0xffffa00c03018c00 */
>  16) + 24.875 us   |        } /* ip6ip6_gso_segment = 0xffffa00c03018c00 */
>  16) + 27.416 us   |      } /* ipv6_gso_segment = 0xffffa00c03018c00 */
>  16)   0.230 us    |      __rcu_read_unlock(); /* = 0x2 */
>  16) + 29.065 us   |    } /* skb_mac_gso_segment = 0xffffa00c03018c00 */
>  16) + 32.828 us   |  } /* __skb_gso_segment = 0xffffa00c03018c00 */
> sh#

Re: [PATCH net 1/2] udp: Mark GSO packets as CHECKSUM_UNNECESSARY early on on output

[Jakub Sitnicki]

On Fri, Jul 26, 2024 at 09:58 AM -04, Willem de Bruijn wrote:
> On Fri, Jul 26, 2024 at 7:23 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> On Thu, Jul 25, 2024 at 10:21 AM -04, Willem de Bruijn wrote:
>> > On Thu, Jul 25, 2024 at 5:56 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>> >>
>> >> In commit 10154dbded6d ("udp: Allow GSO transmit from devices with no
>> >> checksum offload") we have added a tweak in the UDP GSO code to mark GSO
>> >> packets being sent out as CHECKSUM_UNNECESSARY when the egress device
>> >> doesn't support checksum offload. This was done to satisfy the offload
>> >> checks in the gso stack.
>> >>
>> >> However, when sending a UDP GSO packet from a tunnel device, we will go
>> >> through the TX path and the GSO offload twice. Once for the tunnel device,
>> >> which acts as a passthru for GSO packets, and once for the underlying
>> >> egress device.
>> >>
>> >> Even though a tunnel device acts as a passthru for a UDP GSO packet, GSO
>> >> offload checks still happen on transmit from a tunnel device. So if the skb
>> >> is not marked as CHECKSUM_UNNECESSARY or CHECKSUM_PARTIAL, we will get a
>> >> warning from the gso stack.
>> >
>> > I don't entirely understand. The check should not hit on pass through,
>> > where segs == skb:
>> >
>> >         if (segs != skb && unlikely(skb_needs_check(skb, tx_path) &&
>> > !IS_ERR(segs)))
>> >                 skb_warn_bad_offload(skb);
>> >
>>
>> That's something I should have explained better. Let me try to shed some
>> light on it now. We're hitting the skb_warn_bad_offload warning because
>> skb_mac_gso_segment doesn't return any segments (segs == NULL).
>>
>> And that's because we bail out early out of __udp_gso_segment when we
>> detect that the tunnel device is capable of tx-udp-segmentation
>> (GSO_UDP_L4):
>>
>>         if (skb_gso_ok(gso_skb, features | NETIF_F_GSO_ROBUST)) {
>>                 /* Packet is from an untrusted source, reset gso_segs. */
>>                 skb_shinfo(gso_skb)->gso_segs = DIV_ROUND_UP(gso_skb->len - sizeof(*uh),
>>                                                              mss);
>>                 return NULL;
>>         }
>
> Oh I see. Thanks.
>
>> It has not occurred to me before, but in the spirit of commit
>> 8d74e9f88d65 "net: avoid skb_warn_bad_offload on IS_ERR" [1], we could
>> tighten the check to exclude cases when segs == NULL. I'm thinking of:
>>
>>         if (segs != skb && !IS_ERR_OR_NULL(segs) && unlikely(skb_needs_check(skb, tx_path)))
>>                 skb_warn_bad_offload(skb);
>
> That looks sensible to me. And nicer than the ip_summed conversion in
> udp_send_skb.

I've audited all existing ->gso_segment callbacks. skb_mac_gso_segment()
returns no segments, that is segs == NULL, if the callback chain ends
with either of these:

… → udp[46]_ufo_fragment → __udp_gso_segment → skb_gso_ok == true
… → tcp[46]_gso_segment → tcp_gso_segment → skb_gso_ok == true
… → sctp_gso_segment → skb_gso_ok == true

IOW when the device advertises that it can handle the desired GSO kind
(skb_gso_ok() returns true).

Considering that a device offering HW GSO and no checksum offload at the
same time makes no sense, I also think that tweaking the bad offload
detection to exclude the !segs case doesn't deprive us of diagnostics.

I will change to that in v2.