diff mbox series

[5.10,380/530] udp: never accept GSO_FRAGLIST packets

Message ID 20210512144832.263718249@linuxfoundation.org
State New
Headers show
Series None | expand

Commit Message

Greg KH May 12, 2021, 2:48 p.m. UTC
From: Paolo Abeni <pabeni@redhat.com>

[ Upstream commit 78352f73dc5047f3f744764cc45912498c52f3c9 ]

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

This change addresses the issue introducing and maintaining
a couple of new fields to explicitly accept SKB_GSO_UDP_L4
or GSO_FRAGLIST packets. Additionally updates  udp_unexpected_gso()
accordingly.

UDP sockets enabling UDP_GRO stil keep accept_udp_fraglist
zeroed.

v1 -> v2:
 - use 2 bits instead of a whole GSO bitmask (Willem)

Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 include/linux/udp.h | 16 +++++++++++++---
 net/ipv4/udp.c      |  3 +++
 2 files changed, 16 insertions(+), 3 deletions(-)

Comments

Pavel Machek May 15, 2021, 8:37 a.m. UTC | #1
Hi!

> From: Paolo Abeni <pabeni@redhat.com>

> 

> [ Upstream commit 78352f73dc5047f3f744764cc45912498c52f3c9 ]

> 

> Currently the UDP protocol delivers GSO_FRAGLIST packets to

> the sockets without the expected segmentation.

> 

> This change addresses the issue introducing and maintaining

> a couple of new fields to explicitly accept SKB_GSO_UDP_L4

> or GSO_FRAGLIST packets. Additionally updates  udp_unexpected_gso()

> accordingly.

> 

> UDP sockets enabling UDP_GRO stil keep accept_udp_fraglist

> zeroed.


What is going on here? accept_udp_fraglist variable is read-only.

Should this be dropped? Should 

commit d18931a92a0b5feddd8a39d097b90ae2867db02f
    vxlan: allow L4 GRO passthrough
    
be cherry-picked?

Best regards,
							Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Paolo Abeni May 17, 2021, 6:51 a.m. UTC | #2
On Sat, 2021-05-15 at 10:37 +0200, Pavel Machek wrote:
> Hi!

> 

> > From: Paolo Abeni <pabeni@redhat.com>

> > 

> > [ Upstream commit 78352f73dc5047f3f744764cc45912498c52f3c9 ]

> > 

> > Currently the UDP protocol delivers GSO_FRAGLIST packets to

> > the sockets without the expected segmentation.

> > 

> > This change addresses the issue introducing and maintaining

> > a couple of new fields to explicitly accept SKB_GSO_UDP_L4

> > or GSO_FRAGLIST packets. Additionally updates  udp_unexpected_gso()

> > accordingly.

> > 

> > UDP sockets enabling UDP_GRO stil keep accept_udp_fraglist

> > zeroed.

> 

> What is going on here? accept_udp_fraglist variable is read-only.


Thank you for checking this!

The 'accept_udp_fraglist' field is implicitly initilized to zero at UDP
socket allocation time (done by sk_alloc).

So this patch effectively force segmentation of SKB_GSO_FRAGLIST
packets via the udp_unexpected_gso() helper.

We introduce the above field instead of unconditionally
segmenting SKB_GSO_FRAGLIST, because the next patch will use it (to
avoid unneeded segmentation for performance's sake for UDP tunnel), as
you noted.

Please let me know if the above clarifies the situation.

Thanks!

Paolo
Pavel Machek May 17, 2021, 6:57 a.m. UTC | #3
Hi!

> > > From: Paolo Abeni <pabeni@redhat.com>

> > > 

> > > [ Upstream commit 78352f73dc5047f3f744764cc45912498c52f3c9 ]

> > > 

> > > Currently the UDP protocol delivers GSO_FRAGLIST packets to

> > > the sockets without the expected segmentation.

> > > 

> > > This change addresses the issue introducing and maintaining

> > > a couple of new fields to explicitly accept SKB_GSO_UDP_L4

> > > or GSO_FRAGLIST packets. Additionally updates  udp_unexpected_gso()

> > > accordingly.

> > > 

> > > UDP sockets enabling UDP_GRO stil keep accept_udp_fraglist

> > > zeroed.

> > 

> > What is going on here? accept_udp_fraglist variable is read-only.

> 

> Thank you for checking this!

> 

> The 'accept_udp_fraglist' field is implicitly initilized to zero at UDP

> socket allocation time (done by sk_alloc).

> 

> So this patch effectively force segmentation of SKB_GSO_FRAGLIST

> packets via the udp_unexpected_gso() helper.

> 

> We introduce the above field instead of unconditionally

> segmenting SKB_GSO_FRAGLIST, because the next patch will use it (to

> avoid unneeded segmentation for performance's sake for UDP tunnel), as

> you noted.


Ok, but there's no follow up patch queued for 5.10...? Do we still
need it there?

Best regards,
								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Paolo Abeni May 17, 2021, 7:47 a.m. UTC | #4
On Mon, 2021-05-17 at 08:57 +0200, Pavel Machek wrote:
> From: Paolo Abeni <pabeni@redhat.com>

> > > > 

> > > > [ Upstream commit 78352f73dc5047f3f744764cc45912498c52f3c9 ]

> > > > 

> > > > Currently the UDP protocol delivers GSO_FRAGLIST packets to

> > > > the sockets without the expected segmentation.

> > > > 

> > > > This change addresses the issue introducing and maintaining

> > > > a couple of new fields to explicitly accept SKB_GSO_UDP_L4

> > > > or GSO_FRAGLIST packets. Additionally updates  udp_unexpected_gso()

> > > > accordingly.

> > > > 

> > > > UDP sockets enabling UDP_GRO stil keep accept_udp_fraglist

> > > > zeroed.

> > > 

> > > What is going on here? accept_udp_fraglist variable is read-only.

> > 

> > Thank you for checking this!

> > 

> > The 'accept_udp_fraglist' field is implicitly initilized to zero at UDP

> > socket allocation time (done by sk_alloc).

> > 

> > So this patch effectively force segmentation of SKB_GSO_FRAGLIST

> > packets via the udp_unexpected_gso() helper.

> > 

> > We introduce the above field instead of unconditionally

> > segmenting SKB_GSO_FRAGLIST, because the next patch will use it (to

> > avoid unneeded segmentation for performance's sake for UDP tunnel), as

> > you noted.

> 

> Ok, but there's no follow up patch queued for 5.10...? Do we still

> need it there?


Patch d18931a92a0b5feddd8a39d097b90ae2867db02f is a performance
improvement, I think not worthy/suitable for stable.

For 5.10 78352f73dc5047f3f744764cc45912498c52f3c9 should be enough.

Thanks!

Paolo
diff mbox series

Patch

diff --git a/include/linux/udp.h b/include/linux/udp.h
index aa84597bdc33..ae58ff3b6b5b 100644
--- a/include/linux/udp.h
+++ b/include/linux/udp.h
@@ -51,7 +51,9 @@  struct udp_sock {
 					   * different encapsulation layer set
 					   * this
 					   */
-			 gro_enabled:1;	/* Can accept GRO packets */
+			 gro_enabled:1,	/* Request GRO aggregation */
+			 accept_udp_l4:1,
+			 accept_udp_fraglist:1;
 	/*
 	 * Following member retains the information to create a UDP header
 	 * when the socket is uncorked.
@@ -131,8 +133,16 @@  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;
+	if (!skb_is_gso(skb))
+		return false;
+
+	if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && !udp_sk(sk)->accept_udp_l4)
+		return true;
+
+	if (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST && !udp_sk(sk)->accept_udp_fraglist)
+		return true;
+
+	return false;
 }
 
 #define udp_portaddr_for_each_entry(__sk, list) \
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 4a2fd286787c..9d28b2778e8f 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2657,9 +2657,12 @@  int udp_lib_setsockopt(struct sock *sk, int level, int optname,
 
 	case UDP_GRO:
 		lock_sock(sk);
+
+		/* when enabling GRO, accept the related GSO packet type */
 		if (valbool)
 			udp_tunnel_encap_enable(sk->sk_socket);
 		up->gro_enabled = valbool;
+		up->accept_udp_l4 = valbool;
 		release_sock(sk);
 		break;