udp: GRO L4 improvements

When looping back UDP GSO over UDP tunnel packets to an UDP socket, the individual packet csum is currently set to CSUM_NONE. That causes unexpected/wrong csum validation errors later in the UDP receive path.

We could possibly addressing the issue with some additional check and csum mangling in the UDP tunnel code. Since the issue affects only this UDP receive slow path, let's set a suitable csum status there.

Signed-off-by Paolo Abeni <[email protected]>

---
 include/net/udp.h | 18 ++++++++++++++++++
 net/ipv4/udp.c    | 10 ++++++++++
 net/ipv6/udp.c    |  5 +++++
 3 files changed, 33 insertions(+)

include/net/udp.h +6 -0

index d4d064c592328..007683eb3e113 100644
@@ -515,6 +515,24 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
        return segs;
 }
 
+static inline void udp_post_segment_fix_csum(struct sk_buff *skb, int level)
+{
+       /* UDP-lite can't land here - no GRO */
+       WARN_ON_ONCE(UDP_SKB_CB(skb)->partial_cov);
+
+       /* GRO already validated the csum up to 'level', and we just

include/net/udp.h +2 -0

+}
+
 #ifdef CONFIG_BPF_SYSCALL
 struct sk_psock;
 struct proto *udp_bpf_get_proto(struct sock *sk, struct sk_psock *psock);

net/ipv4/udp.c +3 -0

index 4a0478b17243a..ff54135c51ffa 100644
@@ -2168,6 +2168,7 @@ static int udp_queue_rcv_one_skb(struct sock *sk, struct sk_buff *skb)
 static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 {
        struct sk_buff *next, *segs;
+       int csum_level;
        int ret;
 
        if (likely(!udp_unexpected_gso(sk, skb)))

@@ -2175,9 +2176,18 @@ static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 
        BUILD_BUG_ON(sizeof(struct udp_skb_cb) > SKB_GSO_CB_OFFSET);
        __skb_push(skb, -skb_mac_offset(skb));
+       csum_level = !!(skb_shinfo(skb)->gso_type &
+                       (SKB_GSO_UDP_TUNNEL | SKB_GSO_UDP_TUNNEL_CSUM));

net/ipv4/udp.c +0 -0

                ret = udp_queue_rcv_one_skb(sk, skb);
                if (ret > 0)
                        ip_protocol_deliver_rcu(dev_net(skb->dev), skb, ret);

net/ipv6/udp.c +5 -1

index d25e5a9252fdb..e7d4bf3a65c72 100644
@@ -739,16 +739,21 @@ static int udpv6_queue_rcv_one_skb(struct sock *sk, struct sk_buff *skb)
 static int udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 {
        struct sk_buff *next, *segs;
+       int csum_level;
        int ret;
 
        if (likely(!udp_unexpected_gso(sk, skb)))
                return udpv6_queue_rcv_one_skb(sk, skb);
 
        __skb_push(skb, -skb_mac_offset(skb));
+       csum_level = !!(skb_shinfo(skb)->gso_type &
+                       (SKB_GSO_UDP_TUNNEL | SKB_GSO_UDP_TUNNEL_CSUM));
        segs = udp_rcv_segment(sk, skb, false);
        skb_list_walk_safe(segs, skb, next) {
                __skb_pull(skb, skb_transport_offset(skb));
 
+               /* see comments in udp_queue_rcv_skb() */
+               udp_post_segment_fix_csum(skb, csum_level);
                ret = udpv6_queue_rcv_one_skb(sk, skb);
                if (ret > 0)
                        ip6_protocol_deliver_rcu(dev_net(skb->dev), skb, ret,
--

2.26.2

If UDP GRO forwarding (or list) is enabled,

and there are
udp tunnel available in the system, we could end-up doing L4 aggregation for packets targeting the UDP tunnel.

That could inner protocol corruption, as no overaly network parameters is taken in account at aggregation time.

Just skip the fwd GRO if this packet could land in an UDP tunnel.

The current check is broader than what is strictly needed, as the UDP tunnel could be e.g. on top of a different device, but is simple and the performance downside looks not relevant.

Fixes 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")

Fixes 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets")

Signed-off-by Paolo Abeni <[email protected]>

After the previous patch the stack can do L4 UDP aggregation on top of an UDP tunnel.

The current GRO complete code tries frag based aggregation first; in the above scenario will generate corrupted frames.

We need to try first UDP tunnel based aggregation, if the GRO packet requires that. We can use time GRO 'encap_mark' field to track the need GRO complete action. If encap_mark is set, skip the frag_list aggregation.

On tunnel encap GRO complete clear such field, so that an inner frag_list GRO complete could take action.

Signed-off-by Paolo Abeni <[email protected]>

---
 net/ipv4/udp_offload.c | 8 +++++++-
 net/ipv6/udp_offload.c | 3 ++-
 2 files changed, 9 insertions(+), 2 deletions(-)

net/ipv4/udp_offload.c +5 -0

index 25134a3548e99..54e06b88af69a 100644
@@ -642,6 +642,11 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff,
                skb_shinfo(skb)->gso_type = uh->check ? SKB_GSO_UDP_TUNNEL_CSUM
                                        : SKB_GSO_UDP_TUNNEL;
 
+               /* clear the encap mark, so that inner frag_list gro_complete
+                * can take place
+                */
+               NAPI_GRO_CB(skb)->encap_mark = 0;
+
                /* Set encapsulation before calling into inner gro_complete()
                 * functions to make them set up the inner offsets.
                 */

net/ipv4/udp_offload.c +0 -0

                uh->len = htons(skb->len - nhoff);
 
                skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4);

net/ipv6/udp_offload.c +2 -2

index faa823c242923..b3d9ed96e5ea5 100644
@@ -163,7 +163,8 @@ INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff)
        const struct ipv6hdr *ipv6h = ipv6_hdr(skb);
        struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
 
-       if (NAPI_GRO_CB(skb)->is_flist) {
+       /* do fraglist only if there is no outer UDP encap (or we already processed it) */
+       if (NAPI_GRO_CB(skb)->is_flist && !NAPI_GRO_CB(skb)->encap_mark) {
                uh->len = htons(skb->len - nhoff);
 
                skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4);
--

2.26.2

Currently the UDP protocol delivers GSO_FRAGLIST packets to the sockets without the expected segmentation.

This change addresses the issue introducing and maintaining a per socket bitmask of GSO types requiring segmentation. Enabling GSO removes SKB_GSO_UDP_L4 from such mask, while GSO_FRAGLIST packets are never accepted

Note: this also updates the 'unused' field size to really fit the otherwise existing hole. It's size become incorrect after commit bec1f6f69736 ("udp: generate gso with UDP_SEGMENT").

Fixes 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")

Signed-off-by Paolo Abeni <[email protected]>

---
 include/linux/udp.h | 10 ++++++----
 net/ipv4/udp.c      | 12 +++++++++++-
 2 files changed, 17 insertions(+), 5 deletions(-)

include/linux/udp.h +0 -0

index aa84597bdc33c..6da342f15f351 100644

create a bunch of virtual topology and verify that GRO_FRAG_LIST and GRO_FWD aggregate the ingress

packets as expected, and the aggregate packets
are segmented correctly when landing on a socket

Also test L4 aggregation on top of UDP tunnel (vxlan)

Signed-off-by Paolo Abeni <[email protected]>

---
 tools/testing/selftests/net/Makefile      |   1 +
 tools/testing/selftests/net/udpgro_fwd.sh | 251 ++++++++++++++++++++++
 2 files changed, 252 insertions(+)
 create mode 100755 tools/testing/selftests/net/udpgro_fwd.sh

tools/testing/selftests/net/Makefile +1 -0

index 25f198bec0b25..2d71b283dde36 100644
@@ -23,6 +23,7 @@ TEST_PROGS += drop_monitor_tests.sh
 TEST_PROGS += vrf_route_leaking.sh
 TEST_PROGS += bareudp.sh
 TEST_PROGS += unicast_extensions.sh
+TEST_PROGS += udpgro_fwd.sh
 TEST_PROGS_EXTENDED := in_netns.sh
 TEST_GEN_FILES =  socket nettest
 TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy reuseport_addr_any

tools/testing/selftests/net/udpgro_fwd.sh new file +7 -0

new file mode 100755
index 0000000000000..ac7ac56a27524
@@ -0,0 +1,251 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+readonly BASE="ns-$(mktemp -u XXXXXX)"
+readonly SRC=2
+readonly DST=1
+readonly DST_NAT=100

Quoted 29 lines · Paolo Abeni

When looping back UDP GSO over UDP tunnel packets to an UDP socket, the individual packet csum is currently set to CSUM_NONE. That causes unexpected/wrong csum validation errors later in the UDP receive path.

We could possibly addressing the issue with some additional check and csum mangling in the UDP tunnel code. Since the issue affects only this UDP receive slow path, let's set a suitable csum status there.

Signed-off-by Paolo Abeni <[email protected]>

---
 include/net/udp.h | 18 ++++++++++++++++++
 net/ipv4/udp.c    | 10 ++++++++++
 net/ipv6/udp.c    |  5 +++++
 3 files changed, 33 insertions(+)

include/net/udp.h +6 -0

index d4d064c592328..007683eb3e113 100644
@@ -515,6 +515,24 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
        return segs;
 }
 
+static inline void udp_post_segment_fix_csum(struct sk_buff *skb, int level)
+{
+       /* UDP-lite can't land here - no GRO */
+       WARN_ON_ONCE(UDP_SKB_CB(skb)->partial_cov);
+
+       /* GRO already validated the csum up to 'level', and we just

include/net/udp.h static inline struct sk_buff *udp_rcv_segment(struct sock *sk, · line 533

Quoted 10 lines

include/net/udp.h +10 -0

+        * consumed one header, update the skb accordingly
+        */
+       UDP_SKB_CB(skb)->cscov = skb->len;
+       if (level) {
+               skb->ip_summed = CHECKSUM_UNNECESSARY;
+               skb->csum_level = 0;
+       } else {
+               skb->ip_summed = CHECKSUM_NONE;
+               skb->csum_valid = 1;
+       }

why does this function also update these fields for non-tunneled packets? the commit only describes an issue with tunneled packets.

Quoted 29 lines

include/net/udp.h +2 -0

+}
+
 #ifdef CONFIG_BPF_SYSCALL
 struct sk_psock;
 struct proto *udp_bpf_get_proto(struct sock *sk, struct sk_psock *psock);

net/ipv4/udp.c +6 -0

index 4a0478b17243a..ff54135c51ffa 100644
@@ -2168,6 +2168,7 @@ static int udp_queue_rcv_one_skb(struct sock *sk, struct sk_buff *skb)
 static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 {
        struct sk_buff *next, *segs;
+       int csum_level;
        int ret;
 
        if (likely(!udp_unexpected_gso(sk, skb)))

@@ -2175,9 +2176,18 @@ static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 
        BUILD_BUG_ON(sizeof(struct udp_skb_cb) > SKB_GSO_CB_OFFSET);
        __skb_push(skb, -skb_mac_offset(skb));
+       csum_level = !!(skb_shinfo(skb)->gso_type &
+                       (SKB_GSO_UDP_TUNNEL | SKB_GSO_UDP_TUNNEL_CSUM));
        segs = udp_rcv_segment(sk, skb, true);
        skb_list_walk_safe(segs, skb, next) {
                __skb_pull(skb, skb_transport_offset(skb));
+
+               /* UDP GSO packets looped back after adding UDP encap land here with CHECKSUM none,
+                * instead of adding another check in the tunnel fastpath, we can force valid

net/ipv4/udp.c static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) · line 2190

Quoted 4 lines

net/ipv4/udp.c +4 -0

+                * csums here (packets are locally generated).
+                * Additionally fixup the UDP CB
+                */
+               udp_post_segment_fix_csum(skb, csum_level);

How does this code differentiates locally generated packets with udp tunnel headers from packets arriving from the wire, for which the inner checksum may be incorrect?

Quoted 1 line · Paolo Abeni

If UDP GRO forwarding (or list) is enabled,

Please explicitly mention the gso type SKB_GSO_FRAGLIST. I, at least, didn't immediately grasp that gro forwarding is an alias for that.

Quoted 3 lines

and there are
udp tunnel available in the system, we could end-up doing L4 aggregation for packets targeting the UDP tunnel.

Is this specific to UDP tunnels, or can this also occur with others, such as GRE? (not implying that this patchset needs to address those at the same time)

Quoted 2 lines

Just skip the fwd GRO if this packet could land in an UDP tunnel.

Could you make more clear that this does not skip UDP GRO, only switches from fraglist-based to pure SKB_GSO_UDP_L4.

Quoted 40 lines · Paolo Abeni

After the previous patch the stack can do L4 UDP aggregation on top of an UDP tunnel.

The current GRO complete code tries frag based aggregation first; in the above scenario will generate corrupted frames.

We need to try first UDP tunnel based aggregation, if the GRO packet requires that. We can use time GRO 'encap_mark' field to track the need GRO complete action. If encap_mark is set, skip the frag_list aggregation.

On tunnel encap GRO complete clear such field, so that an inner frag_list GRO complete could take action.

Signed-off-by Paolo Abeni <[email protected]>

---
 net/ipv4/udp_offload.c | 8 +++++++-
 net/ipv6/udp_offload.c | 3 ++-
 2 files changed, 9 insertions(+), 2 deletions(-)

net/ipv4/udp_offload.c +5 -0

index 25134a3548e99..54e06b88af69a 100644
@@ -642,6 +642,11 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff,
                skb_shinfo(skb)->gso_type = uh->check ? SKB_GSO_UDP_TUNNEL_CSUM
                                        : SKB_GSO_UDP_TUNNEL;
 
+               /* clear the encap mark, so that inner frag_list gro_complete
+                * can take place
+                */
+               NAPI_GRO_CB(skb)->encap_mark = 0;
+
                /* Set encapsulation before calling into inner gro_complete()
                 * functions to make them set up the inner offsets.
                 */

@@ -665,7 +670,8 @@ INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff)
        const struct iphdr *iph = ip_hdr(skb);
        struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
 

net/ipv4/udp_offload.c INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff) · line 674

Quoted 3 lines

net/ipv4/udp_offload.c +2 -1

-       if (NAPI_GRO_CB(skb)->is_flist) {
+       /* do fraglist only if there is no outer UDP encap (or we already processed it) */
+       if (NAPI_GRO_CB(skb)->is_flist && !NAPI_GRO_CB(skb)->encap_mark) {

Sorry, I don't follow. I thought the point was to avoid fraglist if an outer udp tunnel header is present. But the above code clears the mark and allows entering the fraglist branch exactly when such a header is encountered?

Quoted 19 lines · Paolo Abeni

Currently the UDP protocol delivers GSO_FRAGLIST packets to the sockets without the expected segmentation.

This change addresses the issue introducing and maintaining a per socket bitmask of GSO types requiring segmentation. Enabling GSO removes SKB_GSO_UDP_L4 from such mask, while GSO_FRAGLIST packets are never accepted

Note: this also updates the 'unused' field size to really fit the otherwise existing hole. It's size become incorrect after commit bec1f6f69736 ("udp: generate gso with UDP_SEGMENT").

Fixes 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")

Signed-off-by Paolo Abeni <[email protected]>

---
 include/linux/udp.h | 10 ++++++----
 net/ipv4/udp.c      | 12 +++++++++++-
 2 files changed, 17 insertions(+), 5 deletions(-)

include/linux/udp.h struct udp_sock { · line 54

Quoted 10 lines

include/linux/udp.h +1 -1

index aa84597bdc33c..6da342f15f351 100644
@@ -51,7 +51,7 @@ struct udp_sock {
                                           * different encapsulation layer set
                                           * this
                                           */
-                        gro_enabled:1; /* Can accept GRO packets */
+                        gro_enabled:1; /* Request GRO aggregation */

unnecessary comment change?

Quoted 6 lines

include/linux/udp.h +0 -0

        /*
         * Following member retains the information to create a UDP header
         * when the socket is uncorked.

@@ -68,7 +68,10 @@ struct udp_sock {
 #define UDPLITE_SEND_CC  0x2           /* set via udplite setsockopt         */
 #define UDPLITE_RECV_CC  0x4           /* set via udplite setsocktopt        */

include/linux/udp.h struct udp_sock { · line 74

Quoted 6 lines

include/linux/udp.h +4 -1

        __u8             pcflag;        /* marks socket as UDP-Lite if > 0    */
-       __u8             unused[3];
+       __u8             unused[1];
+       unsigned int     unexpected_gso;/* GSO types this socket can't accept,
+                                        * any of SKB_GSO_UDP_L4 or SKB_GSO_FRAGLIST
+                                        */

An extra unsigned int for this seems overkill.

Current sockets that support SKB_GSO_UDP_L4 implicitly also support SKB_GSO_FRAGLIST. This patch makes explicit that the second is not supported..

include/linux/udp.h static inline void udp_cmsg_recv(struct msghdr *msg, struct sock *sk, · line 137

Quoted 10 lines

include/linux/udp.h +1 -2

        /*
         * For encapsulation sockets.
         */

@@ -131,8 +134,7 @@ static inline void udp_cmsg_recv(struct msghdr *msg, struct sock *sk,
 
 static inline bool udp_unexpected_gso(struct sock *sk, struct sk_buff *skb)
 {
-       return !udp_sk(sk)->gro_enabled && skb_is_gso(skb) &&
-              skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4;
+       return skb_is_gso(skb) && skb_shinfo(skb)->gso_type & udp_sk(sk)->unexpected_gso;

.. just update this function as follows ?

- return !udp_sk(sk)->gro_enabled && skb_is_gso(skb) &&
- skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4;

(quoted diff) +2 -0

+       return skb_is_gso(skb) &&
+                 (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST ||

!udp_sk(sk)->gro_enabled)

where the latter is shorthand for

  (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && !udp_sk(sk)->gro_enabled)

but the are the only two GSO types that could arrive here.

Quoted 2 lines · Paolo Abeni

create a bunch of virtual topology and verify that GRO_FRAG_LIST and GRO_FWD aggregate the ingress

what are these constants? Aliases for SKB_GSO_FRAGLIST and ?

Quoted 18 lines

tools/testing/selftests/net/udpgro_fwd.sh new file +12 -0

new file mode 100755
index 0000000000000..ac7ac56a27524
@@ -0,0 +1,251 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+readonly BASE="ns-$(mktemp -u XXXXXX)"
+readonly SRC=2
+readonly DST=1
+readonly DST_NAT=100
+readonly NS_SRC=$BASE$SRC
+readonly NS_DST=$BASE$DST
+
+# "baremetal" network used for raw UDP traffic
+readonly BM_NET_V4=192.168.1.

tools/testing/selftests/net/udpgro_fwd.sh line 17

Quoted 5 lines

tools/testing/selftests/net/udpgro_fwd.sh new file +5 -0

+readonly BM_NET_V6=2001:db8::
+
+# "overlay" network used for UDP over UDP tunnel traffic
+readonly OL_NET_V4=172.16.1.
+readonly OL_NET_V6=2002:db8::

is it okay to use a prod64 prefix for this? should this be another subnet of 2001:db8:: instead? of fd..

Quoted 73 lines · Willem de Bruijn

Quoted 29 lines · Paolo Abeni

When looping back UDP GSO over UDP tunnel packets to an UDP socket, the individual packet csum is currently set to CSUM_NONE. That causes unexpected/wrong csum validation errors later in the UDP receive path.

We could possibly addressing the issue with some additional check and csum mangling in the UDP tunnel code. Since the issue affects only this UDP receive slow path, let's set a suitable csum status there.

Signed-off-by Paolo Abeni <[email protected]>

---
 include/net/udp.h | 18 ++++++++++++++++++
 net/ipv4/udp.c    | 10 ++++++++++
 net/ipv6/udp.c    |  5 +++++
 3 files changed, 33 insertions(+)

include/net/udp.h +6 -0

index d4d064c592328..007683eb3e113 100644
@@ -515,6 +515,24 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
        return segs;
 }
 
+static inline void udp_post_segment_fix_csum(struct sk_buff *skb, int level)
+{
+       /* UDP-lite can't land here - no GRO */
+       WARN_ON_ONCE(UDP_SKB_CB(skb)->partial_cov);
+
+       /* GRO already validated the csum up to 'level', and we just

include/net/udp.h static inline struct sk_buff *udp_rcv_segment(struct sock *sk, · line 533

Quoted 10 lines

include/net/udp.h +10 -0

+        * consumed one header, update the skb accordingly
+        */
+       UDP_SKB_CB(skb)->cscov = skb->len;
+       if (level) {
+               skb->ip_summed = CHECKSUM_UNNECESSARY;
+               skb->csum_level = 0;
+       } else {
+               skb->ip_summed = CHECKSUM_NONE;
+               skb->csum_valid = 1;
+       }

why does this function also update these fields for non-tunneled packets? the commit only describes an issue with tunneled packets.

Quoted 29 lines

include/net/udp.h +2 -0

+}
+
 #ifdef CONFIG_BPF_SYSCALL
 struct sk_psock;
 struct proto *udp_bpf_get_proto(struct sock *sk, struct sk_psock *psock);

net/ipv4/udp.c +6 -0

index 4a0478b17243a..ff54135c51ffa 100644
@@ -2168,6 +2168,7 @@ static int udp_queue_rcv_one_skb(struct sock *sk, struct sk_buff *skb)
 static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 {
        struct sk_buff *next, *segs;
+       int csum_level;
        int ret;
 
        if (likely(!udp_unexpected_gso(sk, skb)))

@@ -2175,9 +2176,18 @@ static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 
        BUILD_BUG_ON(sizeof(struct udp_skb_cb) > SKB_GSO_CB_OFFSET);
        __skb_push(skb, -skb_mac_offset(skb));
+       csum_level = !!(skb_shinfo(skb)->gso_type &
+                       (SKB_GSO_UDP_TUNNEL | SKB_GSO_UDP_TUNNEL_CSUM));
        segs = udp_rcv_segment(sk, skb, true);
        skb_list_walk_safe(segs, skb, next) {
                __skb_pull(skb, skb_transport_offset(skb));
+
+               /* UDP GSO packets looped back after adding UDP encap land here with CHECKSUM none,
+                * instead of adding another check in the tunnel fastpath, we can force valid

Quoted 8 lines

net/ipv4/udp.c static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) · line 2190

Quoted 4 lines

net/ipv4/udp.c +4 -0

+                * csums here (packets are locally generated).
+                * Additionally fixup the UDP CB
+                */
+               udp_post_segment_fix_csum(skb, csum_level);

How does this code differentiates locally generated packets with udp tunnel headers from packets arriving from the wire, for which the inner checksum may be incorrect?

First thing first, thank you for the detailed review. Digesting all the comments will take time, so please excuse for some latency.

I'll try to reply to both your question here because the replies are related.

My understanding is that UDP GRO, when processing UDP over UDP traffic

with the appropriate features bit set, will validate the checksum for both the inner and the outer header - udp{4,6}_gro_receive will be traversed twice, the fist one for the outer header, the 2nd for the inner.

So when we reach here, the inner header csum could not be incorrect, and I don't do anything to differentiate locally generated GSO packets and GROed one to keep the code simpler.

The udp_post_segment_fix_csum() always set the csum info - even for non tunneled packets to avoid additional branches/make the code more complex. The csum should be valid in any scenario.

I guess I can mention the above either in a code comment and/or in the commit message.

Cheers,

Paolo

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

net/ipv4/udp.c +1 -0

@@ -2168,6 +2168,7 @@ static int udp_queue_rcv_one_skb(struct sock *sk, struct sk_buff *skb)
 static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 {
        struct sk_buff *next, *segs;
+       int csum_level;
        int ret;
 
        if (likely(!udp_unexpected_gso(sk, skb)))

@@ -2175,9 +2176,18 @@ static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

net/ipv4/udp.c +2 -0

        BUILD_BUG_ON(sizeof(struct udp_skb_cb) > SKB_GSO_CB_OFFSET);
        __skb_push(skb, -skb_mac_offset(skb));
+       csum_level = !!(skb_shinfo(skb)->gso_type &
+                       (SKB_GSO_UDP_TUNNEL | SKB_GSO_UDP_TUNNEL_CSUM));
        segs = udp_rcv_segment(sk, skb, true);

Quoted 8 lines

Quoted 8 lines

net/ipv4/udp.c static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) · line 2189

Quoted 8 lines

net/ipv4/udp.c +6 -0

        skb_list_walk_safe(segs, skb, next) {
                __skb_pull(skb, skb_transport_offset(skb));
+
+               /* UDP GSO packets looped back after adding UDP encap land here with CHECKSUM none,
+                * instead of adding another check in the tunnel fastpath, we can force valid
+                * csums here (packets are locally generated).
+                * Additionally fixup the UDP CB
+                */

Quoted 7 lines

Quoted 3 lines

and there are
udp tunnel available in the system, we could end-up doing L4 aggregation for packets targeting the UDP tunnel.

Is this specific to UDP tunnels, or can this also occur with others, such as GRE? (not implying that this patchset needs to address those at the same time)

I did not look at that before your suggestion. Thanks for pointing out.

I think the problem is specific to UDP: when processing the outer UDP header that is potentially eligible for both NETIF_F_GSO_UDP_L4 and gro_receive aggregation and that is the root cause of the problem addressed here.

Quoted 7 lines

Quoted 4 lines

Just skip the fwd GRO if this packet could land in an UDP tunnel.

Could you make more clear that this does not skip UDP GRO, only switches from fraglist-based to pure SKB_GSO_UDP_L4.

Sure, I'll try to rewrite the commit message.

Thanks!

Paolo

Quoted 41 lines · Willem de Bruijn

Quoted 40 lines · Paolo Abeni

After the previous patch the stack can do L4 UDP aggregation on top of an UDP tunnel.

The current GRO complete code tries frag based aggregation first; in the above scenario will generate corrupted frames.

We need to try first UDP tunnel based aggregation, if the GRO packet requires that. We can use time GRO 'encap_mark' field to track the need GRO complete action. If encap_mark is set, skip the frag_list aggregation.

On tunnel encap GRO complete clear such field, so that an inner frag_list GRO complete could take action.

Signed-off-by Paolo Abeni <[email protected]>

---
 net/ipv4/udp_offload.c | 8 +++++++-
 net/ipv6/udp_offload.c | 3 ++-
 2 files changed, 9 insertions(+), 2 deletions(-)

net/ipv4/udp_offload.c +5 -0

index 25134a3548e99..54e06b88af69a 100644
@@ -642,6 +642,11 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff,
                skb_shinfo(skb)->gso_type = uh->check ? SKB_GSO_UDP_TUNNEL_CSUM
                                        : SKB_GSO_UDP_TUNNEL;
 
+               /* clear the encap mark, so that inner frag_list gro_complete
+                * can take place
+                */
+               NAPI_GRO_CB(skb)->encap_mark = 0;
+
                /* Set encapsulation before calling into inner gro_complete()
                 * functions to make them set up the inner offsets.
                 */

@@ -665,7 +670,8 @@ INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff)
        const struct iphdr *iph = ip_hdr(skb);
        struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
 

Quoted 8 lines

net/ipv4/udp_offload.c INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff) · line 674

Quoted 3 lines

net/ipv4/udp_offload.c +2 -1

-       if (NAPI_GRO_CB(skb)->is_flist) {
+       /* do fraglist only if there is no outer UDP encap (or we already processed it) */
+       if (NAPI_GRO_CB(skb)->is_flist && !NAPI_GRO_CB(skb)->encap_mark) {

Sorry, I don't follow. I thought the point was to avoid fraglist if an outer udp tunnel header is present. But the above code clears the mark and allows entering the fraglist branch exactly when such a header is encountered?

The relevant UDP packet has gone through:

[l2/l3 GRO] -> udp_gro_receive -> udp_sk(sk)->gro_receive -> [some more GRO layers] -> udp_gro_receive (again)

The first udp_gro_receive set NAPI_GRO_CB(skb)->encap_mark, the latter udp_gro_receive set NAPI_GRO_CB(skb)->is_flist.

Then, at GRO complete time:

[l2/l3 GRO] -> udp{4,6}_gro_complete -> udp_sk(sk)->gro_complete -> [more GRO layers] -> udp{4,6}_gro_complete (again).

In the first udp{4,6}_gro_complete invocation 'encap_mark' is 1, so with this patch we do the 'udp_sk(sk)->gro_complete' path. In the second udp{4,6}_gro_complete invocation 'encap_mark' has been cleared (by udp_gro_complete), so we do the SKB_GSO_FRAGLIST completion.

In case SKB_GSO_FRAGLIST with no UDP tunnel, 'encap_mark' is 0 and we do the SKB_GSO_FRAGLIST completion.

Another alternative, possibly more readable, would be avoid clearing 'encap_mark' in udp_gro_complete() and replacing the above check with:

        if (NAPI_GRO_CB(skb)->is_flist &&
            (!NAPI_GRO_CB(skb)->encap_mark ||
             (NAPI_GRO_CB(skb)->encap_mark && skb->encapsulation))) {

I opted otherwise to simplify the conditional expression.

Please let me know if the above is somewhat more clear and if you have preferecens between the two options.

Quoted 21 lines · Willem de Bruijn

Quoted 19 lines · Paolo Abeni

Currently the UDP protocol delivers GSO_FRAGLIST packets to the sockets without the expected segmentation.

This change addresses the issue introducing and maintaining a per socket bitmask of GSO types requiring segmentation. Enabling GSO removes SKB_GSO_UDP_L4 from such mask, while GSO_FRAGLIST packets are never accepted

Note: this also updates the 'unused' field size to really fit the otherwise existing hole. It's size become incorrect after commit bec1f6f69736 ("udp: generate gso with UDP_SEGMENT").

Fixes 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")

Signed-off-by Paolo Abeni <[email protected]>

---
 include/linux/udp.h | 10 ++++++----
 net/ipv4/udp.c      | 12 +++++++++++-
 2 files changed, 17 insertions(+), 5 deletions(-)

Quoted 14 lines

include/linux/udp.h struct udp_sock { · line 74

Quoted 12 lines

/*
* Following member retains the information to create a UDP header
* when the socket is uncorked.

include/linux/udp.h +4 -1

@@ -68,7 +68,10 @@ struct udp_sock {
 #define UDPLITE_SEND_CC  0x2           /* set via udplite setsockopt         */
 #define UDPLITE_RECV_CC  0x4           /* set via udplite setsocktopt        */
        __u8             pcflag;        /* marks socket as UDP-Lite if > 0    */
-       __u8             unused[3];
+       __u8             unused[1];
+       unsigned int     unexpected_gso;/* GSO types this socket can't accept,
+                                        * any of SKB_GSO_UDP_L4 or SKB_GSO_FRAGLIST
+                                        */

An extra unsigned int for this seems overkill.

Should be more clear after the next patch.

Using an explicit 'acceptable GSO types' field makes the patch 5/8 quite simple.

After this patch the 'udp_sock' struct size remains unchanged and even the number of 'udp_sock' cachelines touched for every packet is unchanged.

Quoted 83 lines · Paolo Abeni

Quoted 73 lines · Willem de Bruijn

Quoted 29 lines · Paolo Abeni

When looping back UDP GSO over UDP tunnel packets to an UDP socket, the individual packet csum is currently set to CSUM_NONE. That causes unexpected/wrong csum validation errors later in the UDP receive path.

We could possibly addressing the issue with some additional check and csum mangling in the UDP tunnel code. Since the issue affects only this UDP receive slow path, let's set a suitable csum status there.

Signed-off-by Paolo Abeni <[email protected]>

---
 include/net/udp.h | 18 ++++++++++++++++++
 net/ipv4/udp.c    | 10 ++++++++++
 net/ipv6/udp.c    |  5 +++++
 3 files changed, 33 insertions(+)

include/net/udp.h +6 -0

index d4d064c592328..007683eb3e113 100644
@@ -515,6 +515,24 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
        return segs;
 }
 
+static inline void udp_post_segment_fix_csum(struct sk_buff *skb, int level)
+{
+       /* UDP-lite can't land here - no GRO */
+       WARN_ON_ONCE(UDP_SKB_CB(skb)->partial_cov);
+
+       /* GRO already validated the csum up to 'level', and we just

include/net/udp.h static inline struct sk_buff *udp_rcv_segment(struct sock *sk, · line 533

Quoted 10 lines

include/net/udp.h +10 -0

+        * consumed one header, update the skb accordingly
+        */
+       UDP_SKB_CB(skb)->cscov = skb->len;
+       if (level) {
+               skb->ip_summed = CHECKSUM_UNNECESSARY;
+               skb->csum_level = 0;
+       } else {
+               skb->ip_summed = CHECKSUM_NONE;
+               skb->csum_valid = 1;
+       }

why does this function also update these fields for non-tunneled packets? the commit only describes an issue with tunneled packets.

Quoted 29 lines

include/net/udp.h +2 -0

+}
+
 #ifdef CONFIG_BPF_SYSCALL
 struct sk_psock;
 struct proto *udp_bpf_get_proto(struct sock *sk, struct sk_psock *psock);

net/ipv4/udp.c +6 -0

index 4a0478b17243a..ff54135c51ffa 100644
@@ -2168,6 +2168,7 @@ static int udp_queue_rcv_one_skb(struct sock *sk, struct sk_buff *skb)
 static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 {
        struct sk_buff *next, *segs;
+       int csum_level;
        int ret;
 
        if (likely(!udp_unexpected_gso(sk, skb)))

@@ -2175,9 +2176,18 @@ static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 
        BUILD_BUG_ON(sizeof(struct udp_skb_cb) > SKB_GSO_CB_OFFSET);
        __skb_push(skb, -skb_mac_offset(skb));
+       csum_level = !!(skb_shinfo(skb)->gso_type &
+                       (SKB_GSO_UDP_TUNNEL | SKB_GSO_UDP_TUNNEL_CSUM));
        segs = udp_rcv_segment(sk, skb, true);
        skb_list_walk_safe(segs, skb, next) {
                __skb_pull(skb, skb_transport_offset(skb));
+
+               /* UDP GSO packets looped back after adding UDP encap land here with CHECKSUM none,
+                * instead of adding another check in the tunnel fastpath, we can force valid

Quoted 8 lines

net/ipv4/udp.c static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) · line 2190

Quoted 4 lines

net/ipv4/udp.c +4 -0

+                * csums here (packets are locally generated).
+                * Additionally fixup the UDP CB
+                */
+               udp_post_segment_fix_csum(skb, csum_level);

How does this code differentiates locally generated packets with udp tunnel headers from packets arriving from the wire, for which the inner checksum may be incorrect?

Quoted 2 lines

First thing first, thank you for the detailed review. Digesting all the comments will take time, so please excuse for some latency.

Apologies for my own delayed response. I also need to take time to understand the existing code and diffs :) And have a few questions.

Quoted 4 lines

I'll try to reply to both your question here because the replies are related.

My understanding is that UDP GRO, when processing UDP over UDP traffic

This is a UDP GSO packet egress packet that was further encapsulated with a GSO_UDP_TUNNEL on egress, then looped to the ingress path?

Then in the ingress path it has traversed the GRO layer.

Is this veth with XDP? That seems unlikely for GSO packets. But there aren't many paths that will loop a packet through napi_gro_receive or napi_gro_frags.

Quoted 4 lines

with the appropriate features bit set, will validate the checksum for both the inner and the outer header - udp{4,6}_gro_receive will be traversed twice, the fist one for the outer header, the 2nd for the inner.

GRO will validate multiple levels of checksums with CHECKSUM_COMPLETE. It can only validate the outer checksum with CHECKSUM_UNNECESSARY, I believe?

As for looped packets with CHECKSUM_PARTIAL: we definitely have found bugs in that path before. I think it's fine to set csum_valid on any packets that can unambiguously be identified as such. Hence the detailed questions above on which exact packets this code is targeting, so that there are not accidental false positives that look the same but have a different ip_summed.

Quoted 6 lines

I did not look at that before your suggestion. Thanks for pointing out.

I think the problem is specific to UDP: when processing the outer UDP header that is potentially eligible for both NETIF_F_GSO_UDP_L4 and gro_receive aggregation and that is the root cause of the problem addressed here.

Can you elaborate on the exact problem? The commit mentions "inner protocol corruption, as no overaly network parameters is taken in account at aggregation time."

My understanding is that these are udp gro aggregated GSO_UDP_L4 packets forwarded to a udp tunnel device. They are not encapsulated yet. Which overlay network parameters are not, but should have been, taken account at aggregation time?

Quoted 36 lines · Paolo Abeni

Quoted 20 lines · Willem de Bruijn

Quoted 19 lines · Paolo Abeni

Currently the UDP protocol delivers GSO_FRAGLIST packets to the sockets without the expected segmentation.

This change addresses the issue introducing and maintaining a per socket bitmask of GSO types requiring segmentation. Enabling GSO removes SKB_GSO_UDP_L4 from such mask, while GSO_FRAGLIST packets are never accepted

Note: this also updates the 'unused' field size to really fit the otherwise existing hole. It's size become incorrect after commit bec1f6f69736 ("udp: generate gso with UDP_SEGMENT").

Fixes 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")

Signed-off-by Paolo Abeni <[email protected]>

---
 include/linux/udp.h | 10 ++++++----
 net/ipv4/udp.c      | 12 +++++++++++-
 2 files changed, 17 insertions(+), 5 deletions(-)

Quoted 14 lines

include/linux/udp.h struct udp_sock { · line 74

Quoted 12 lines

/*
* Following member retains the information to create a UDP header
* when the socket is uncorked.

include/linux/udp.h +4 -1

@@ -68,7 +68,10 @@ struct udp_sock {
 #define UDPLITE_SEND_CC  0x2           /* set via udplite setsockopt         */
 #define UDPLITE_RECV_CC  0x4           /* set via udplite setsocktopt        */
        __u8             pcflag;        /* marks socket as UDP-Lite if > 0    */
-       __u8             unused[3];
+       __u8             unused[1];
+       unsigned int     unexpected_gso;/* GSO types this socket can't accept,
+                                        * any of SKB_GSO_UDP_L4 or SKB_GSO_FRAGLIST
+                                        */

An extra unsigned int for this seems overkill.

Quoted 8 lines

Should be more clear after the next patch.

Using an explicit 'acceptable GSO types' field makes the patch 5/8 quite simple.

After this patch the 'udp_sock' struct size remains unchanged and even the number of 'udp_sock' cachelines touched for every packet is unchanged.

But there is opportunity cost, of course. Next time we need to add something to the struct, we will add a new cacheline.

A 32-bit field for just 2 bits, where 1 already exists does seem like overkill.

More importantly, I just think it's less obvious code than a pair of fields

accepts_udp_l4:1,
accepts_udp_fraglist:1,

Local sockets can only accept the first, as there does not exist an interface to pass along the multiple frag sizes that a frag_list based approach might have.

Sockets with encap_rcv != NULL may opt-in to being able to handle either.

I think explicit code will be more maintainable.

At the cost of
perhaps two branches instead of one, admittedly. But that seems premature optimization.

Quoted 56 lines · Willem de Bruijn

Quoted 36 lines · Paolo Abeni

Quoted 20 lines · Willem de Bruijn

Quoted 19 lines · Paolo Abeni

Currently the UDP protocol delivers GSO_FRAGLIST packets to the sockets without the expected segmentation.

This change addresses the issue introducing and maintaining a per socket bitmask of GSO types requiring segmentation. Enabling GSO removes SKB_GSO_UDP_L4 from such mask, while GSO_FRAGLIST packets are never accepted

Note: this also updates the 'unused' field size to really fit the otherwise existing hole. It's size become incorrect after commit bec1f6f69736 ("udp: generate gso with UDP_SEGMENT").

Fixes 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")

Signed-off-by Paolo Abeni <[email protected]>

---
 include/linux/udp.h | 10 ++++++----
 net/ipv4/udp.c      | 12 +++++++++++-
 2 files changed, 17 insertions(+), 5 deletions(-)

Quoted 14 lines

include/linux/udp.h struct udp_sock { · line 74

Quoted 12 lines

/*
* Following member retains the information to create a UDP header
* when the socket is uncorked.

include/linux/udp.h +4 -1

@@ -68,7 +68,10 @@ struct udp_sock {
 #define UDPLITE_SEND_CC  0x2           /* set via udplite setsockopt         */
 #define UDPLITE_RECV_CC  0x4           /* set via udplite setsocktopt        */
        __u8             pcflag;        /* marks socket as UDP-Lite if > 0    */
-       __u8             unused[3];
+       __u8             unused[1];
+       unsigned int     unexpected_gso;/* GSO types this socket can't accept,
+                                        * any of SKB_GSO_UDP_L4 or SKB_GSO_FRAGLIST
+                                        */

An extra unsigned int for this seems overkill.

Quoted 8 lines

Should be more clear after the next patch.

Using an explicit 'acceptable GSO types' field makes the patch 5/8 quite simple.

After this patch the 'udp_sock' struct size remains unchanged and even the number of 'udp_sock' cachelines touched for every packet is unchanged.

But there is opportunity cost, of course. Next time we need to add something to the struct, we will add a new cacheline.

A 32-bit field for just 2 bits, where 1 already exists does seem like overkill.

More importantly, I just think it's less obvious code than a pair of fields

accepts_udp_l4:1,
accepts_udp_fraglist:1,

Quoted 7 lines

Local sockets can only accept the first, as there does not exist an interface to pass along the multiple frag sizes that a frag_list based approach might have.

Sockets with encap_rcv != NULL may opt-in to being able to handle either.

I think explicit code will be more maintainable.

ok

Quoted 3 lines

At the cost of
perhaps two branches instead of one, admittedly. But that seems premature optimization.

well, if it don't hurt too much your eyes, something along the following could save udp_sock space and code branches:

    rejects_udp_l4_fraglist:2;

#define SKB_GSO_UDP_L4_SHIFT (NETIF_F_GSO_UDP_L4_BIT - NETIF_F_GSO_SHIFT)
static inline bool udp_unexpected_gso(struct sock *sk, struct sk_buff *skb)

 {
        BUILD_BUG_ON(1 << SKB_GSO_UDP_L4_SHIFT != SKB_GSO_UDP_L4);
        BUILD_BUG_ON(1 << (SKB_GSO_UDP_L4_SHIFT + 1) != SKB_GSO_FRAGLIST);
        return skb_is_gso(skb) && skb_shinfo(skb)->gso_type &
                (udp_sk(sk)->rejects_udp_l4_fraglist << SKB_GSO_UDP_L4_SHIFT);

}

(not sure if /me runs/hides ;)

/P

Quoted 8 lines

This is a UDP GSO packet egress packet that was further encapsulated with a GSO_UDP_TUNNEL on egress, then looped to the ingress path?

Then in the ingress path it has traversed the GRO layer.

Is this veth with XDP? That seems unlikely for GSO packets. But there aren't many paths that will loop a packet through napi_gro_receive or napi_gro_frags.

This patch addresses the following scenario:

sk ->vxlan -> veth -> (xdp in use, TSO disabled, GRO on) -> veth -> vxlan -> sk

What I meant here is that the issue is not visible with:

(baremetal, NETIF_F_GRO_UDP_FWD | NETIF_F_GRO_FRAGLIST enabled -> vxlan -> sk

Quoted 8 lines

Quoted 4 lines

with the appropriate features bit set, will validate the checksum for both the inner and the outer header - udp{4,6}_gro_receive will be traversed twice, the fist one for the outer header, the 2nd for the inner.

GRO will validate multiple levels of checksums with CHECKSUM_COMPLETE. It can only validate the outer checksum with CHECKSUM_UNNECESSARY, I believe?

I possibly miss some bits here ?!?

AFAICS:

udp4_gro_receive() -> skb_gro_checksum_validate_zero_check() -> __skb_gro_checksum_validate -> (if not already valid) __skb_gro_checksum_validate_complete() -> (if not CHECKSUM_COMPLETE) __skb_gro_checksum_complete()

the latter will validate the UDP checksum at the current nesting level (and set the csum-related bits in the GRO skb cb to the same status as CHECKSUM_COMPLETE)

When processing UDP over UDP tunnel packet, the gro call chain will be:

[l2/l3 GRO] -> udp4_gro_receive (validate outher header csum) -> udp_sk(sk)->gro_receive -> [other gro layers] -> udp4_gro_receive (validate inner header csum) -> ...

Quoted 6 lines

As for looped packets with CHECKSUM_PARTIAL: we definitely have found bugs in that path before. I think it's fine to set csum_valid on any packets that can unambiguously be identified as such. Hence the detailed questions above on which exact packets this code is targeting, so that there are not accidental false positives that look the same but have a different ip_summed.

I see this change is controversial.

Since the addressed scenario is
really a corner case, a simpler alternative would be replacing udp_post_segment_fix_csum with:

static inline void udp_post_segment_fix_cb(struct sk_buff *skb, int level)

{
        /* UDP-lite can't land here - no GRO */
        WARN_ON_ONCE(UDP_SKB_CB(skb)->partial_cov);

        /* UDP CB mirrors the GSO packet, we must re-init it */
        UDP_SKB_CB(skb)->cscov = skb->len;
}

the downside will be an additional, later, unneeded csum validation for the

sk ->vxlan -> veth -> (xdp in use, TSO disabled, GRO on) -> veth -> vxlan -> sk

scenario. WDYT?

Quoted 7 lines · Willem de Bruijn

Quoted 6 lines

I did not look at that before your suggestion. Thanks for pointing out.

I think the problem is specific to UDP: when processing the outer UDP header that is potentially eligible for both NETIF_F_GSO_UDP_L4 and gro_receive aggregation and that is the root cause of the problem addressed here.

Quoted 8 lines

Can you elaborate on the exact problem? The commit mentions "inner protocol corruption, as no overaly network parameters is taken in account at aggregation time."

My understanding is that these are udp gro aggregated GSO_UDP_L4 packets forwarded to a udp tunnel device. They are not encapsulated yet. Which overlay network parameters are not, but should have been, taken account at aggregation time?

The scenario is as follow:

* a NIC has NETIF_F_GRO_UDP_FWD or NETIF_F_GRO_FRAGLIST enabled
* an UDP tunnel is configured/enabled in the system
* the above NIC receives some UDP-tunneled packets, targeting the mentioned tunnel
* the packets go through gro_receive and they reache 'udp_gro_receive()' while processing the outer UDP header.

without this patch, udp_gro_receive_segment() will kick in and the outer UDP header will be aggregated according to SKB_GSO_FRAGLIST or SKB_GSO_UDP_L4, even if this is really e.g. a vxlan packet.

Different vxlan ids will be ignored/aggregated to the same GSO packet. Inner headers will be ignored, too, so that e.g. TCP over vxlan push packets will be held in the GRO engine till the next flush, etc.

Please let me know if the above is more clear.

Quoted 39 lines

Quoted 8 lines

This is a UDP GSO packet egress packet that was further encapsulated with a GSO_UDP_TUNNEL on egress, then looped to the ingress path?

Then in the ingress path it has traversed the GRO layer.

Is this veth with XDP? That seems unlikely for GSO packets. But there aren't many paths that will loop a packet through napi_gro_receive or napi_gro_frags.

This patch addresses the following scenario:

sk ->vxlan -> veth -> (xdp in use, TSO disabled, GRO on) -> veth -> vxlan -> sk

What I meant here is that the issue is not visible with:

(baremetal, NETIF_F_GRO_UDP_FWD | NETIF_F_GRO_FRAGLIST enabled -> vxlan -> sk

Quoted 8 lines

Quoted 4 lines

with the appropriate features bit set, will validate the checksum for both the inner and the outer header - udp{4,6}_gro_receive will be traversed twice, the fist one for the outer header, the 2nd for the inner.

GRO will validate multiple levels of checksums with CHECKSUM_COMPLETE. It can only validate the outer checksum with CHECKSUM_UNNECESSARY, I believe?

I possibly miss some bits here ?!?

AFAICS:

udp4_gro_receive() -> skb_gro_checksum_validate_zero_check() -> __skb_gro_checksum_validate -> (if not already valid) __skb_gro_checksum_validate_complete() -> (if not CHECKSUM_COMPLETE) __skb_gro_checksum_complete()

the latter will validate the UDP checksum at the current nesting level (and set the csum-related bits in the GRO skb cb to the same status as CHECKSUM_COMPLETE)

Quoted 5 lines

When processing UDP over UDP tunnel packet, the gro call chain will be:

[l2/l3 GRO] -> udp4_gro_receive (validate outher header csum) -> udp_sk(sk)->gro_receive -> [other gro layers] -> udp4_gro_receive (validate inner header csum) -> ...

Agreed. But __skb_gro_checksum_validate on the first invocation will call skb_gro_incr_csum_unnecessary, so that on the second invocation csum_cnt == 0 and triggers a real checksum validation?

At least, that is my understanding. Intuitively, CHECKSUM_UNNECESSARY only validates the first checksum, so says nothing about the validity of any subsequent ones.

But it seems I'm mistaken?

__skb_gro_checksum_validate has an obvious exception for locally generated packets by excluding CHECKSUM_PARTIAL. Looped packets usually have CHECKSUM_PARTIAL set. Unfortunately, this is similar to the issue that I looked at earlier this year with looped UDP packets with MSG_MORE: those are also changed to CHECKSUM_NONE (and exposed a checksum bug: 52cbd23a119c).

Is there perhaps some other way that we can identify that these are local packets? They should trivially avoid all checksum checks.

Quoted 7 lines

Quoted 6 lines

As for looped packets with CHECKSUM_PARTIAL: we definitely have found bugs in that path before. I think it's fine to set csum_valid on any packets that can unambiguously be identified as such. Hence the detailed questions above on which exact packets this code is targeting, so that there are not accidental false positives that look the same but have a different ip_summed.

Quoted 1 line

I see this change is controversial.

I have no concerns with the fix. It is just a very narrow path (veth + xdp - tso + gro ..), and to minimize risk I would try to avoid updating state of unrelated packets. That was my initial comment: I don't understand the need for the else clause.

Quoted 13 lines

Since the addressed scenario is
really a corner case, a simpler alternative would be replacing udp_post_segment_fix_csum with:

static inline void udp_post_segment_fix_cb(struct sk_buff *skb, int level)

{
        /* UDP-lite can't land here - no GRO */
        WARN_ON_ONCE(UDP_SKB_CB(skb)->partial_cov);

        /* UDP CB mirrors the GSO packet, we must re-init it */
        UDP_SKB_CB(skb)->cscov = skb->len;
}

Quoted 5 lines

the downside will be an additional, later, unneeded csum validation for the

sk ->vxlan -> veth -> (xdp in use, TSO disabled, GRO on) -> veth -> vxlan -> sk

scenario. WDYT?

No, let's definitely avoid an unneeded checksum verification.

Quoted 78 lines · Paolo Abeni

Quoted 56 lines · Willem de Bruijn

Quoted 36 lines · Paolo Abeni

Quoted 20 lines · Willem de Bruijn

Quoted 19 lines · Paolo Abeni

Currently the UDP protocol delivers GSO_FRAGLIST packets to the sockets without the expected segmentation.

This change addresses the issue introducing and maintaining a per socket bitmask of GSO types requiring segmentation. Enabling GSO removes SKB_GSO_UDP_L4 from such mask, while GSO_FRAGLIST packets are never accepted

Note: this also updates the 'unused' field size to really fit the otherwise existing hole. It's size become incorrect after commit bec1f6f69736 ("udp: generate gso with UDP_SEGMENT").

Fixes 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")

Signed-off-by Paolo Abeni <[email protected]>

---
 include/linux/udp.h | 10 ++++++----
 net/ipv4/udp.c      | 12 +++++++++++-
 2 files changed, 17 insertions(+), 5 deletions(-)

Quoted 14 lines

include/linux/udp.h struct udp_sock { · line 74

Quoted 12 lines

/*
* Following member retains the information to create a UDP header
* when the socket is uncorked.

include/linux/udp.h +4 -1

@@ -68,7 +68,10 @@ struct udp_sock {
 #define UDPLITE_SEND_CC  0x2           /* set via udplite setsockopt         */
 #define UDPLITE_RECV_CC  0x4           /* set via udplite setsocktopt        */
        __u8             pcflag;        /* marks socket as UDP-Lite if > 0    */
-       __u8             unused[3];
+       __u8             unused[1];
+       unsigned int     unexpected_gso;/* GSO types this socket can't accept,
+                                        * any of SKB_GSO_UDP_L4 or SKB_GSO_FRAGLIST
+                                        */

An extra unsigned int for this seems overkill.

Quoted 8 lines

Should be more clear after the next patch.

Using an explicit 'acceptable GSO types' field makes the patch 5/8 quite simple.

After this patch the 'udp_sock' struct size remains unchanged and even the number of 'udp_sock' cachelines touched for every packet is unchanged.

But there is opportunity cost, of course. Next time we need to add something to the struct, we will add a new cacheline.

A 32-bit field for just 2 bits, where 1 already exists does seem like overkill.

More importantly, I just think it's less obvious code than a pair of fields

accepts_udp_l4:1,
accepts_udp_fraglist:1,

Quoted 7 lines

Local sockets can only accept the first, as there does not exist an interface to pass along the multiple frag sizes that a frag_list based approach might have.

Sockets with encap_rcv != NULL may opt-in to being able to handle either.

I think explicit code will be more maintainable.

ok

Quoted 3 lines

At the cost of
perhaps two branches instead of one, admittedly. But that seems premature optimization.

well, if it don't hurt too much your eyes, something along the following could save udp_sock space and code branches:

    rejects_udp_l4_fraglist:2;

#define SKB_GSO_UDP_L4_SHIFT (NETIF_F_GSO_UDP_L4_BIT - NETIF_F_GSO_SHIFT)
static inline bool udp_unexpected_gso(struct sock *sk, struct sk_buff *skb)

Quoted 8 lines

 {
        BUILD_BUG_ON(1 << SKB_GSO_UDP_L4_SHIFT != SKB_GSO_UDP_L4);
        BUILD_BUG_ON(1 << (SKB_GSO_UDP_L4_SHIFT + 1) != SKB_GSO_FRAGLIST);
        return skb_is_gso(skb) && skb_shinfo(skb)->gso_type &
                (udp_sk(sk)->rejects_udp_l4_fraglist << SKB_GSO_UDP_L4_SHIFT);

}

(not sure if /me runs/hides ;)

:)

My opinion is just one, but I do find this a lot less readable and hence maintainable than

if (likely(!skb_is_gso(skb)))

     return true;

  if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && !udp_sk(sk)->accept_udp_l4)
    return false;

if (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST &&
!udp_sk(sk)->accept_udp_fraglist)

    return false;

return true;

at no obvious benefit. The tunnel gso code is hard enough to fathom as it is.

Hello,

Quoted 49 lines · Willem de Bruijn

Quoted 39 lines

Quoted 8 lines

This is a UDP GSO packet egress packet that was further encapsulated with a GSO_UDP_TUNNEL on egress, then looped to the ingress path?

Then in the ingress path it has traversed the GRO layer.

Is this veth with XDP? That seems unlikely for GSO packets. But there aren't many paths that will loop a packet through napi_gro_receive or napi_gro_frags.

This patch addresses the following scenario:

sk ->vxlan -> veth -> (xdp in use, TSO disabled, GRO on) -> veth -> vxlan -> sk

What I meant here is that the issue is not visible with:

(baremetal, NETIF_F_GRO_UDP_FWD | NETIF_F_GRO_FRAGLIST enabled -> vxlan -> sk

Quoted 8 lines

Quoted 4 lines

with the appropriate features bit set, will validate the checksum for both the inner and the outer header - udp{4,6}_gro_receive will be traversed twice, the fist one for the outer header, the 2nd for the inner.

GRO will validate multiple levels of checksums with CHECKSUM_COMPLETE. It can only validate the outer checksum with CHECKSUM_UNNECESSARY, I believe?

I possibly miss some bits here ?!?

AFAICS:

udp4_gro_receive() -> skb_gro_checksum_validate_zero_check() -> __skb_gro_checksum_validate -> (if not already valid) __skb_gro_checksum_validate_complete() -> (if not CHECKSUM_COMPLETE) __skb_gro_checksum_complete()

the latter will validate the UDP checksum at the current nesting level (and set the csum-related bits in the GRO skb cb to the same status as CHECKSUM_COMPLETE)

Quoted 5 lines

When processing UDP over UDP tunnel packet, the gro call chain will be:

[l2/l3 GRO] -> udp4_gro_receive (validate outher header csum) -> udp_sk(sk)->gro_receive -> [other gro layers] -> udp4_gro_receive (validate inner header csum) -> ...

Agreed. But __skb_gro_checksum_validate on the first invocation will call skb_gro_incr_csum_unnecessary, so that on the second invocation csum_cnt == 0 and triggers a real checksum validation?

Quoted 5 lines

At least, that is my understanding. Intuitively, CHECKSUM_UNNECESSARY only validates the first checksum, so says nothing about the validity of any subsequent ones.

But it seems I'm mistaken?

AFAICS, it depends ;) From skbuff.h:

* skb->csum_level indicates the number of consecutive checksums found in
* the packet minus one that have been verified as CHECKSUM_UNNECESSARY.

if skb->csum_level > 0, the NIC validate additional headers. The intel ixgbe driver use that for vxlan RX csum offload. Such field translates into:

        NAPI_GRO_CB(skb)->csum_cnt

inside the GRO engine, and skb_gro_incr_csum_unnecessary takes care of the updating it after validation.

Quoted 17 lines

__skb_gro_checksum_validate has an obvious exception for locally generated packets by excluding CHECKSUM_PARTIAL. Looped packets usually have CHECKSUM_PARTIAL set. Unfortunately, this is similar to the issue that I looked at earlier this year with looped UDP packets with MSG_MORE: those are also changed to CHECKSUM_NONE (and exposed a checksum bug: 52cbd23a119c).

Is there perhaps some other way that we can identify that these are local packets? They should trivially avoid all checksum checks.

Quoted 7 lines

Quoted 6 lines

As for looped packets with CHECKSUM_PARTIAL: we definitely have found bugs in that path before. I think it's fine to set csum_valid on any packets that can unambiguously be identified as such. Hence the detailed questions above on which exact packets this code is targeting, so that there are not accidental false positives that look the same but have a different ip_summed.

Quoted 6 lines

Quoted 1 line

I see this change is controversial.

I have no concerns with the fix. It is just a very narrow path (veth + xdp - tso + gro ..), and to minimize risk I would try to avoid updating state of unrelated packets. That was my initial comment: I don't understand the need for the else clause.

The branch is there because I wrote this patch before the patches 5,6,7 later in this series. GSO UDP L4 over UDP tunnel packets were segmented at the UDP tunnel level, and that 'else' branch preserves the appropriate 'csum_level' value to avoid later (if/when the packet lands in a plain UDP socket) csum validation.

Quoted 13 lines

Quoted 13 lines

Since the addressed scenario is
really a corner case, a simpler alternative would be replacing udp_post_segment_fix_csum with:

static inline void udp_post_segment_fix_cb(struct sk_buff *skb, int level)

{
        /* UDP-lite can't land here - no GRO */
        WARN_ON_ONCE(UDP_SKB_CB(skb)->partial_cov);

        /* UDP CB mirrors the GSO packet, we must re-init it */
        UDP_SKB_CB(skb)->cscov = skb->len;
}

Quoted 7 lines

Quoted 5 lines

the downside will be an additional, later, unneeded csum validation for the

sk ->vxlan -> veth -> (xdp in use, TSO disabled, GRO on) -> veth -> vxlan -> sk

scenario. WDYT?

No, let's definitely avoid an unneeded checksum verification.

Ok.

My understanding is that the following should be better:

static inline void udp_post_segment_fix_csum(struct sk_buff *skb)

{
        /* UDP-lite can't land here - no GRO */
        WARN_ON_ONCE(UDP_SKB_CB(skb)->partial_cov);

        /* UDP packets generated with UDP_SEGMENT and traversing:
         * UDP tunnel(xmit) -> veth (segmentation) -> veth (gro) -> UDP tunnel (rx)
         * land here with CHECKSUM_NONE. Instead of adding another check
         * in the tunnel fastpath, we can force valid csums here:
         * packets are locally generated and the GRO engine already validated
         * the csum.
         * Additionally fixup the UDP CB
         */
        UDP_SKB_CB(skb)->cscov = skb->len;
        if (skb->ip_summed == CHECKSUM_NONE && !skb->csum_valid)
                skb->csum_valid = 1;
}

I'll use the above in v2.

Quoted 11 lines · Paolo Abeni

AFAICS, it depends ;) From skbuff.h:

* skb->csum_level indicates the number of consecutive checksums found in
* the packet minus one that have been verified as CHECKSUM_UNNECESSARY.

if skb->csum_level > 0, the NIC validate additional headers. The intel ixgbe driver use that for vxlan RX csum offload. Such field translates into:

        NAPI_GRO_CB(skb)->csum_cnt

Quoted 2 lines

inside the GRO engine, and skb_gro_incr_csum_unnecessary takes care of the updating it after validation.

True. I glanced over those cases.

More importantly, where exactly do these looped packets get converted from CHECKSUM_PARTIAL to CHECKSUM_NONE before this patch?

Quoted 20 lines

My understanding is that the following should be better:

static inline void udp_post_segment_fix_csum(struct sk_buff *skb)

{
        /* UDP-lite can't land here - no GRO */
        WARN_ON_ONCE(UDP_SKB_CB(skb)->partial_cov);

        /* UDP packets generated with UDP_SEGMENT and traversing:
         * UDP tunnel(xmit) -> veth (segmentation) -> veth (gro) -> UDP tunnel (rx)
         * land here with CHECKSUM_NONE. Instead of adding another check
         * in the tunnel fastpath, we can force valid csums here:
         * packets are locally generated and the GRO engine already validated
         * the csum.
         * Additionally fixup the UDP CB
         */
        UDP_SKB_CB(skb)->cscov = skb->len;
        if (skb->ip_summed == CHECKSUM_NONE && !skb->csum_valid)
                skb->csum_valid = 1;
}

Quoted 1 line

I'll use the above in v2.

Do I understand correctly that this avoids matching tunneled packets that arrive from the network with rx checksumming disabled, because __skb_gro_checksum_complete will have been called on the outer packet and have set skb->csum_valid?

Yes, this just (1) identifying the packet as being of local source and then (2) setting csum_valid sounds great to me, thanks.