[PATCH net 0/4] gro: various fixes related to UDP tunnels

[PATCH net 0/4] gro: various fixes related to UDP tunnels

[Antoine Tenart]

Hello,

We found issues when a UDP tunnel endpoint is in a different netns than
where UDP GRO happens. This kind of setup is actually quite diverse,
from having one leg of the tunnel on a remove host, to having a tunnel
between netns (eg. being bridged in another one or on the host). In our
case that UDP tunnel was geneve.

UDP tunnel packets should not be GROed at the UDP level. The fundamental
issue here is such packet can't be detected in a foolproof way: we can't
know by looking at a packet alone and the current logic of looking up
UDP sockets is fragile (socket could be in another netns, packet could
be modified in between, etc). Because there is no way to make the GRO
code to correctly handle those packets in all cases, this series aims at
two things: making the net stack to correctly behave (as in, no crash
and no invalid packet) when such thing happens, and in some cases to
prevent this "early GRO" from happening.

First three patches fix issues when an "UDP tunneled" packet is being
GROed too early by rx-udp-gro-forwarding or rx-gro-list.

Last patch is preventing locally generated UDP tunnel packets from being
GROed. This turns out to be more complex than this patch alone as it
relies on skb->encapsulation which is currently untrusty in some cases
(see iptunnel_handle_offloads); but that should fix things in practice
and is acceptable for a fix. Future work is required to improve things
(prevent all locally generated UDP tunnel packets from being GROed),
such as fixing the misuse of skb->encapsulation in drivers; but that
would be net-next material.

Thanks!
Antoine

Antoine Tenart (4):
  udp: do not accept non-tunnel GSO skbs landing in a tunnel
  gro: fix ownership transfer
  udp: do not transition UDP fraglist to unnecessary checksum
  udp: prevent local UDP tunnel packets from being GROed

 include/linux/udp.h    | 14 ++++++++++++++
 net/core/gro.c         |  3 ++-
 net/ipv4/udp_offload.c | 23 ++++++++++++-----------
 net/ipv6/udp_offload.c |  8 --------
 4 files changed, 28 insertions(+), 20 deletions(-)

-- 
2.44.0

[PATCH net 1/4] udp: do not accept non-tunnel GSO skbs landing in a tunnel

[Antoine Tenart]

When rx-udp-gro-forwarding is enabled UDP packets might be GROed when
being forwarded. If such packets might land in a tunnel this can cause
various issues and udp_gro_receive makes sure this isn't the case by
looking for a matching socket. This is performed in
udp4/6_gro_lookup_skb but only in the current netns. This is an issue
with tunneled packets when the endpoint is in another netns. In such
cases the packets will be GROed at the UDP level, which leads to various
issues later on. The same thing can happen with rx-gro-list.

We saw this with geneve packets being GROed at the UDP level. In such
case gso_size is set; later the packet goes through the geneve rx path,
the geneve header is pulled, the offset are adjusted and frag_list skbs
are not adjusted with regard to geneve. When those skbs hit
skb_fragment, it will misbehave. Different outcomes are possible
depending on what the GROed skbs look like; from corrupted packets to
kernel crashes.

One example is a BUG_ON[1] triggered in skb_segment while processing the
frag_list. Because gso_size is wrong (geneve header was pulled)
skb_segment thinks there is "geneve header size" of data in frag_list,
although it's in fact the next packet. The BUG_ON itself has nothing to
do with the issue. This is only one of the potential issues.

Looking up for a matching socket in udp_gro_receive is fragile: the
lookup could be extended to all netns (not speaking about performances)
but nothing prevents those packets from being modified in between and we
could still not find a matching socket. It's OK to keep the current
logic there as it should cover most cases but we also need to make sure
we handle tunnel packets being GROed too early.

This is done by extending the checks in udp_unexpected_gso: GSO packets
lacking the SKB_GSO_UDP_TUNNEL/_CUSM bits and landing in a tunnel must
be segmented.

[1] kernel BUG at net/core/skbuff.c:4408!
    RIP: 0010:skb_segment+0xd2a/0xf70
    __udp_gso_segment+0xaa/0x560

Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets")
Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
 include/linux/udp.h    | 14 ++++++++++++++
 net/ipv4/udp_offload.c |  6 ++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/linux/udp.h b/include/linux/udp.h
index 3748e82b627b..51558d6527f0 100644
--- a/include/linux/udp.h
+++ b/include/linux/udp.h
@@ -150,6 +150,9 @@ static inline void udp_cmsg_recv(struct msghdr *msg, struct sock *sk,
 	}
 }
 
+DECLARE_STATIC_KEY_FALSE(udp_encap_needed_key);
+DECLARE_STATIC_KEY_FALSE(udpv6_encap_needed_key);
+
 static inline bool udp_unexpected_gso(struct sock *sk, struct sk_buff *skb)
 {
 	if (!skb_is_gso(skb))
@@ -163,6 +166,17 @@ static inline bool udp_unexpected_gso(struct sock *sk, struct sk_buff *skb)
 	    !udp_test_bit(ACCEPT_FRAGLIST, sk))
 		return true;
 
+	/* GSO packets lacking the SKB_GSO_UDP_TUNNEL/_CUSM bits might still
+	 * land in a tunnel as the socket check in udp_gro_receive cannot be
+	 * foolproof.
+	 */
+	if ((static_branch_unlikely(&udp_encap_needed_key) ||
+	     static_branch_unlikely(&udpv6_encap_needed_key)) &&
+	    READ_ONCE(udp_sk(sk)->encap_rcv) &&
+	    !(skb_shinfo(skb)->gso_type &
+	      (SKB_GSO_UDP_TUNNEL | SKB_GSO_UDP_TUNNEL_CSUM)))
+		return true;
+
 	return false;
 }
 
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index b9880743765c..e9719afe91cf 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -551,8 +551,10 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
 	unsigned int off = skb_gro_offset(skb);
 	int flush = 1;
 
-	/* we can do L4 aggregation only if the packet can't land in a tunnel
-	 * otherwise we could corrupt the inner stream
+	/* We can do L4 aggregation only if the packet can't land in a tunnel
+	 * otherwise we could corrupt the inner stream. Detecting such packets
+	 * cannot be foolproof and the aggregation might still happen in some
+	 * cases. Such packets should be caught in udp_unexpected_gso later.
 	 */
 	NAPI_GRO_CB(skb)->is_flist = 0;
 	if (!sk || !udp_sk(sk)->gro_receive) {
-- 
2.44.0

[PATCH net 2/4] gro: fix ownership transfer

[Antoine Tenart]

Issue was found while using rx-gro-list. If fragmented packets are GROed
in skb_gro_receive_list, they might be segmented later on and continue
their journey in the stack. In skb_segment_list those skbs can be reused
as-is. This is an issue as their destructor was removed in
skb_gro_receive_list but not the reference to their socket, and then
they can't be orphaned. Fix this by also removing the reference to the
socket.

For example this could be observed,

  kernel BUG at include/linux/skbuff.h:3131!  (skb_orphan)
  RIP: 0010:ip6_rcv_core+0x11bc/0x19a0
  Call Trace:
   ipv6_list_rcv+0x250/0x3f0
   __netif_receive_skb_list_core+0x49d/0x8f0
   netif_receive_skb_list_internal+0x634/0xd40
   napi_complete_done+0x1d2/0x7d0
   gro_cell_poll+0x118/0x1f0

A similar construction is found in skb_gro_receive, apply the same
change there.

Fixes: 5e10da5385d2 ("skbuff: allow 'slow_gro' for skb carring sock reference")
Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
 net/core/gro.c         | 3 ++-
 net/ipv4/udp_offload.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/core/gro.c b/net/core/gro.c
index ee30d4f0c038..83f35d99a682 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -192,8 +192,9 @@ int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb)
 	}
 
 merge:
-	/* sk owenrship - if any - completely transferred to the aggregated packet */
+	/* sk ownership - if any - completely transferred to the aggregated packet */
 	skb->destructor = NULL;
+	skb->sk = NULL;
 	delta_truesize = skb->truesize;
 	if (offset > headlen) {
 		unsigned int eat = offset - headlen;
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index e9719afe91cf..3bb69464930b 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -449,8 +449,9 @@ static int skb_gro_receive_list(struct sk_buff *p, struct sk_buff *skb)
 	NAPI_GRO_CB(p)->count++;
 	p->data_len += skb->len;
 
-	/* sk owenrship - if any - completely transferred to the aggregated packet */
+	/* sk ownership - if any - completely transferred to the aggregated packet */
 	skb->destructor = NULL;
+	skb->sk = NULL;
 	p->truesize += skb->truesize;
 	p->len += skb->len;
 
-- 
2.44.0

[PATCH net 3/4] udp: do not transition UDP fraglist to unnecessary checksum

[Antoine Tenart]

udp4/6_gro_complete transition fraglist packets to CHECKSUM_UNNECESSARY
and sets their checksum level based on if the packet is recognized to be
a tunneled one. However there is no safe way to detect a packet is a
tunneled one and in case such packet is GROed at the UDP level, setting
a wrong checksum level will lead to later errors. For example if those
packets are forwarded to the Tx path they could produce the following
dump:

  gen01: hw csum failure
  skb len=3008 headroom=160 headlen=1376 tailroom=0
  mac=(106,14) net=(120,40) trans=160
  shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0))
  csum(0xffff232e ip_summed=2 complete_sw=0 valid=0 level=0)
  hash(0x77e3d716 sw=1 l4=1) proto=0x86dd pkttype=0 iif=12
  ...

Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
 net/ipv4/udp_offload.c | 8 --------
 net/ipv6/udp_offload.c | 8 --------
 2 files changed, 16 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 3bb69464930b..3263ebcaa3f4 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -722,14 +722,6 @@ INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff)
 		skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4);
 		skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count;
 
-		if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
-			if (skb->csum_level < SKB_MAX_CSUM_LEVEL)
-				skb->csum_level++;
-		} else {
-			skb->ip_summed = CHECKSUM_UNNECESSARY;
-			skb->csum_level = 0;
-		}
-
 		return 0;
 	}
 
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index 312bcaeea96f..9289384cb7d0 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -174,14 +174,6 @@ INDIRECT_CALLABLE_SCOPE int udp6_gro_complete(struct sk_buff *skb, int nhoff)
 		skb_shinfo(skb)->gso_type |= (SKB_GSO_FRAGLIST|SKB_GSO_UDP_L4);
 		skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count;
 
-		if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
-			if (skb->csum_level < SKB_MAX_CSUM_LEVEL)
-				skb->csum_level++;
-		} else {
-			skb->ip_summed = CHECKSUM_UNNECESSARY;
-			skb->csum_level = 0;
-		}
-
 		return 0;
 	}
 
-- 
2.44.0

[PATCH net 4/4] udp: prevent local UDP tunnel packets from being GROed

[Antoine Tenart]

GRO has a fundamental issue with UDP tunnel packets as it can't detect
those in a foolproof way and GRO could happen before they reach the
tunnel endpoint. Previous commits have fixed issues when UDP tunnel
packets come from a remote host, but if those packets are issued locally
they could run into checksum issues.

If the inner packet has a partial checksum the information will be lost
in the GRO logic, either in udp4/6_gro_complete or in
udp_gro_complete_segment and packets will have an invalid checksum when
leaving the host.

Prevent local UDP tunnel packets from ever being GROed at the outer UDP
level.

Due to skb->encapsulation being wrongly used in some drivers this is
actually only preventing UDP tunnel packets with a partial checksum to
be GROed (see iptunnel_handle_offloads) but those were also the packets
triggering issues so in practice this should be sufficient.

Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets")
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
 net/ipv4/udp_offload.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 3263ebcaa3f4..4ea72bd4f6d7 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -559,6 +559,12 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
 	 */
 	NAPI_GRO_CB(skb)->is_flist = 0;
 	if (!sk || !udp_sk(sk)->gro_receive) {
+		/* If the packet was locally encapsulated in a UDP tunnel that
+		 * wasn't detected above, do not GRO.
+		 */
+		if (skb->encapsulation)
+			goto out;
+
 		if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
 			NAPI_GRO_CB(skb)->is_flist = sk ? !udp_test_bit(GRO_ENABLED, sk) : 1;
 
-- 
2.44.0

Re: [PATCH net 1/4] udp: do not accept non-tunnel GSO skbs landing in a tunnel

[kernel test robot]

Hi Antoine,

kernel test robot noticed the following build errors:

[auto build test ERROR on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Antoine-Tenart/udp-do-not-accept-non-tunnel-GSO-skbs-landing-in-a-tunnel/20240315-232048
base:   net/main
patch link:    https://lore.kernel.org/r/20240315151722.119628-2-atenart%40kernel.org
patch subject: [PATCH net 1/4] udp: do not accept non-tunnel GSO skbs landing in a tunnel
config: arc-defconfig (https://download.01.org/0day-ci/archive/20240316/202403160519.XghWVi81-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240316/202403160519.XghWVi81-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403160519.XghWVi81-lkp@intel.com/

All errors (new ones prefixed by >>):

   arc-elf-ld: net/ipv4/udp.o: in function `udp_queue_rcv_skb':
>> udp.c:(.text+0x3aca): undefined reference to `udpv6_encap_needed_key'
>> arc-elf-ld: udp.c:(.text+0x3aca): undefined reference to `udpv6_encap_needed_key'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net 1/4] udp: do not accept non-tunnel GSO skbs landing in a tunnel

[kernel test robot]

Hi Antoine,

kernel test robot noticed the following build errors:

[auto build test ERROR on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Antoine-Tenart/udp-do-not-accept-non-tunnel-GSO-skbs-landing-in-a-tunnel/20240315-232048
base:   net/main
patch link:    https://lore.kernel.org/r/20240315151722.119628-2-atenart%40kernel.org
patch subject: [PATCH net 1/4] udp: do not accept non-tunnel GSO skbs landing in a tunnel
config: nios2-defconfig (https://download.01.org/0day-ci/archive/20240316/202403160550.1TZ0mDSX-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240316/202403160550.1TZ0mDSX-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403160550.1TZ0mDSX-lkp@intel.com/

All errors (new ones prefixed by >>):

   nios2-linux-ld: net/ipv4/udp.o: in function `raw_atomic_read':
   include/linux/atomic/atomic-arch-fallback.h:457:(.text+0x4b28): undefined reference to `udpv6_encap_needed_key'
>> nios2-linux-ld: include/linux/atomic/atomic-arch-fallback.h:457:(.text+0x4b2c): undefined reference to `udpv6_encap_needed_key'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net 1/4] udp: do not accept non-tunnel GSO skbs landing in a tunnel

[Willem de Bruijn]

Antoine Tenart wrote:
> When rx-udp-gro-forwarding is enabled UDP packets might be GROed when
> being forwarded. If such packets might land in a tunnel this can cause
> various issues and udp_gro_receive makes sure this isn't the case by
> looking for a matching socket. This is performed in
> udp4/6_gro_lookup_skb but only in the current netns. This is an issue
> with tunneled packets when the endpoint is in another netns. In such
> cases the packets will be GROed at the UDP level, which leads to various
> issues later on. The same thing can happen with rx-gro-list.
> 
> We saw this with geneve packets being GROed at the UDP level. In such
> case gso_size is set; later the packet goes through the geneve rx path,
> the geneve header is pulled, the offset are adjusted and frag_list skbs
> are not adjusted with regard to geneve. When those skbs hit
> skb_fragment, it will misbehave. Different outcomes are possible
> depending on what the GROed skbs look like; from corrupted packets to
> kernel crashes.
> 
> One example is a BUG_ON[1] triggered in skb_segment while processing the
> frag_list. Because gso_size is wrong (geneve header was pulled)
> skb_segment thinks there is "geneve header size" of data in frag_list,
> although it's in fact the next packet. The BUG_ON itself has nothing to
> do with the issue. This is only one of the potential issues.
> 
> Looking up for a matching socket in udp_gro_receive is fragile: the
> lookup could be extended to all netns (not speaking about performances)
> but nothing prevents those packets from being modified in between and we
> could still not find a matching socket. It's OK to keep the current
> logic there as it should cover most cases but we also need to make sure
> we handle tunnel packets being GROed too early.
> 
> This is done by extending the checks in udp_unexpected_gso: GSO packets
> lacking the SKB_GSO_UDP_TUNNEL/_CUSM bits and landing in a tunnel must
> be segmented.
> 
> [1] kernel BUG at net/core/skbuff.c:4408!
>     RIP: 0010:skb_segment+0xd2a/0xf70
>     __udp_gso_segment+0xaa/0x560
> 
> Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
> Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets")
> Signed-off-by: Antoine Tenart <atenart@kernel.org>

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

Re: [PATCH net 2/4] gro: fix ownership transfer

[Willem de Bruijn]

Antoine Tenart wrote:
> Issue was found while using rx-gro-list. If fragmented packets are GROed

Only if you need to respin: "If packets are GROed with fraglist"

A bit pedantic, but this is subtle stuff. These are not IP fragmented
packets. Or worse, UDP fragmentation offload.

> in skb_gro_receive_list, they might be segmented later on and continue
> their journey in the stack. In skb_segment_list those skbs can be reused
> as-is. This is an issue as their destructor was removed in
> skb_gro_receive_list but not the reference to their socket, and then
> they can't be orphaned. Fix this by also removing the reference to the
> socket.
> 
> For example this could be observed,
> 
>   kernel BUG at include/linux/skbuff.h:3131!  (skb_orphan)
>   RIP: 0010:ip6_rcv_core+0x11bc/0x19a0
>   Call Trace:
>    ipv6_list_rcv+0x250/0x3f0
>    __netif_receive_skb_list_core+0x49d/0x8f0
>    netif_receive_skb_list_internal+0x634/0xd40
>    napi_complete_done+0x1d2/0x7d0
>    gro_cell_poll+0x118/0x1f0
> 
> A similar construction is found in skb_gro_receive, apply the same
> change there.
> 
> Fixes: 5e10da5385d2 ("skbuff: allow 'slow_gro' for skb carring sock reference")
> Signed-off-by: Antoine Tenart <atenart@kernel.org>

Looks fine to me on the understanding that the only GSO packets that
arrive with skb->sk are are result of the referenced commit, and thus
had sock_wfree as destructor.

Re: [PATCH net 4/4] udp: prevent local UDP tunnel packets from being GROed

[Willem de Bruijn]

Antoine Tenart wrote:
> GRO has a fundamental issue with UDP tunnel packets as it can't detect
> those in a foolproof way and GRO could happen before they reach the
> tunnel endpoint. Previous commits have fixed issues when UDP tunnel
> packets come from a remote host, but if those packets are issued locally
> they could run into checksum issues.
> 
> If the inner packet has a partial checksum the information will be lost
> in the GRO logic, either in udp4/6_gro_complete or in
> udp_gro_complete_segment and packets will have an invalid checksum when
> leaving the host.

Before the previous patch, the tunnel code would convert ip_summed to
CHECKSUM_UNNECESSARY. After that patch CHECKSUM_PARTIAL is preserved.
Are the tunneled packets still corrupted once forwarded to the egress
path? In principle CHECKSUM partial with tunnel and GSO should work,
whether built as such or arrived at through GRO.

> Prevent local UDP tunnel packets from ever being GROed at the outer UDP
> level.
> 
> Due to skb->encapsulation being wrongly used in some drivers this is
> actually only preventing UDP tunnel packets with a partial checksum to
> be GROed (see iptunnel_handle_offloads) but those were also the packets
> triggering issues so in practice this should be sufficient.

Because of this in iptunnel_handle_offloads: 

        if (skb->ip_summed != CHECKSUM_PARTIAL) {
                skb->ip_summed = CHECKSUM_NONE;
                /* We clear encapsulation here to prevent badly-written
                 * drivers potentially deciding to offload an inner checksum
                 * if we set CHECKSUM_PARTIAL on the outer header.
                 * This should go away when the drivers are all fixed.
                 */
                skb->encapsulation = 0;
        }
 
> Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
> Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets")
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Antoine Tenart <atenart@kernel.org>

Choosing not to coalesce certain edge case packets that cause problems
is safe and reasonable.

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

Re: [PATCH net 4/4] udp: prevent local UDP tunnel packets from being GROed

[Antoine Tenart]

Hello,

Quoting Willem de Bruijn (2024-03-16 16:43:26)
> Antoine Tenart wrote:
> > GRO has a fundamental issue with UDP tunnel packets as it can't detect
> > those in a foolproof way and GRO could happen before they reach the
> > tunnel endpoint. Previous commits have fixed issues when UDP tunnel
> > packets come from a remote host, but if those packets are issued locally
> > they could run into checksum issues.
> > 
> > If the inner packet has a partial checksum the information will be lost
> > in the GRO logic, either in udp4/6_gro_complete or in
> > udp_gro_complete_segment and packets will have an invalid checksum when
> > leaving the host.
> 
> Before the previous patch, the tunnel code would convert ip_summed to
> CHECKSUM_UNNECESSARY. After that patch CHECKSUM_PARTIAL is preserved.
> Are the tunneled packets still corrupted once forwarded to the egress
> path? In principle CHECKSUM partial with tunnel and GSO should work,
> whether built as such or arrived at through GRO.

Previous patch removed the partial -> unnecessary conversion for
fraglist only; but packets GROed by rx-udp-gro-forwarding can hit
udp_gro_complete_segment and the partial info would be overwritten
there in case of an UDP tunnel packet GROed at the UDP level with the
inner csum being partial.

Thanks!
Antoine

Re: [PATCH net 2/4] gro: fix ownership transfer

[Antoine Tenart]

Quoting Willem de Bruijn (2024-03-16 16:25:13)
> Antoine Tenart wrote:
> > Issue was found while using rx-gro-list. If fragmented packets are GROed
> 
> Only if you need to respin: "If packets are GROed with fraglist"
> 
> A bit pedantic, but this is subtle stuff. These are not IP fragmented
> packets. Or worse, UDP fragmentation offload.

Right, that was only describing which kind of packets were GROed in my
test. Looks like that's confusing, I'll remove it.

> > in skb_gro_receive_list, they might be segmented later on and continue
> > their journey in the stack. In skb_segment_list those skbs can be reused
> > as-is. This is an issue as their destructor was removed in
> > skb_gro_receive_list but not the reference to their socket, and then
> > they can't be orphaned. Fix this by also removing the reference to the
> > socket.
> > 
> > For example this could be observed,
> > 
> >   kernel BUG at include/linux/skbuff.h:3131!  (skb_orphan)
> >   RIP: 0010:ip6_rcv_core+0x11bc/0x19a0
> >   Call Trace:
> >    ipv6_list_rcv+0x250/0x3f0
> >    __netif_receive_skb_list_core+0x49d/0x8f0
> >    netif_receive_skb_list_internal+0x634/0xd40
> >    napi_complete_done+0x1d2/0x7d0
> >    gro_cell_poll+0x118/0x1f0
> > 
> > A similar construction is found in skb_gro_receive, apply the same
> > change there.
> > 
> > Fixes: 5e10da5385d2 ("skbuff: allow 'slow_gro' for skb carring sock reference")
> > Signed-off-by: Antoine Tenart <atenart@kernel.org>
> 
> Looks fine to me on the understanding that the only GSO packets that
> arrive with skb->sk are are result of the referenced commit, and thus
> had sock_wfree as destructor.

The root cause of the issue is a disparity between skb->destructor and
skb->sk; either skb with skb->{destructor,sk} could arrive there and
that was not an issue, or they could not. In both cases the above commit
is introducing that behavior.

Thanks!
Antoine

Re: [PATCH net 1/4] udp: do not accept non-tunnel GSO skbs landing in a tunnel

[Antoine Tenart]

Quoting kernel test robot (2024-03-15 22:21:50)
> Hi Antoine,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on net/main]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Antoine-Tenart/udp-do-not-accept-non-tunnel-GSO-skbs-landing-in-a-tunnel/20240315-232048
> base:   net/main
> patch link:    https://lore.kernel.org/r/20240315151722.119628-2-atenart%40kernel.org
> patch subject: [PATCH net 1/4] udp: do not accept non-tunnel GSO skbs landing in a tunnel
> config: arc-defconfig (https://download.01.org/0day-ci/archive/20240316/202403160519.XghWVi81-lkp@intel.com/config)
> compiler: arc-elf-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240316/202403160519.XghWVi81-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202403160519.XghWVi81-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    arc-elf-ld: net/ipv4/udp.o: in function `udp_queue_rcv_skb':
> >> udp.c:(.text+0x3aca): undefined reference to `udpv6_encap_needed_key'
> >> arc-elf-ld: udp.c:(.text+0x3aca): undefined reference to `udpv6_encap_needed_key'

Issue is with CONFIG_IPV6=n. The following should fix it,

diff --git a/include/linux/udp.h b/include/linux/udp.h
index 51558d6527f0..05231fff8703 100644
--- a/include/linux/udp.h
+++ b/include/linux/udp.h
@@ -151,7 +151,22 @@ static inline void udp_cmsg_recv(struct msghdr *msg, struct sock *sk,
 }
 
 DECLARE_STATIC_KEY_FALSE(udp_encap_needed_key);
+#if IS_ENABLED(CONFIG_IPV6)
 DECLARE_STATIC_KEY_FALSE(udpv6_encap_needed_key);
+#endif
+
+static inline bool udp_encap_needed(void)
+{
+       if (static_branch_unlikely(&udp_encap_needed_key))
+               return true;
+
+#if IS_ENABLED(CONFIG_IPV6)
+       if (static_branch_unlikely(&udpv6_encap_needed_key))
+               return true;
+#endif
+
+       return false;
+}
 
 static inline bool udp_unexpected_gso(struct sock *sk, struct sk_buff *skb)
 {
@@ -170,8 +185,7 @@ static inline bool udp_unexpected_gso(struct sock *sk, struct sk_buff *skb)
         * land in a tunnel as the socket check in udp_gro_receive cannot be
         * foolproof.
         */
-       if ((static_branch_unlikely(&udp_encap_needed_key) ||
-            static_branch_unlikely(&udpv6_encap_needed_key)) &&
+       if (udp_encap_needed() &&
            READ_ONCE(udp_sk(sk)->encap_rcv) &&
            !(skb_shinfo(skb)->gso_type &
              (SKB_GSO_UDP_TUNNEL | SKB_GSO_UDP_TUNNEL_CSUM)))