diff mbox series

[net-next,v2,4/8] udp: never accept GSO_FRAGLIST packets

Message ID 7fa75957409a3f5d14198261a7eddb2bf1bff8e1.1616692794.git.pabeni@redhat.com
State Superseded
Headers show
Series udp: GRO L4 improvements | expand

Commit Message

Paolo Abeni March 25, 2021, 5:24 p.m. UTC 
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>
---
 include/linux/udp.h | 16 +++++++++++++---
 net/ipv4/udp.c      |  3 +++
 2 files changed, 16 insertions(+), 3 deletions(-)

Comments

Willem de Bruijn March 26, 2021, 6:15 p.m. UTC | #1 
On Thu, Mar 25, 2021 at 1:24 PM Paolo Abeni <pabeni@redhat.com> wrote:
>

> 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>


This looks good to me in principle, thanks for the revision.

I hadn't fully appreciated that gro_enabled implies accept_udp_l4, but
not necessarily vice versa.

It is equivalent to (accept_udp_l4 && !up->gro_receive), right?

Could the extra bit be avoided with

"
+      /* Prefer fraglist GRO unless target is a socket with UDP_GRO,
+       * which requires all but last segments to be of same gso_size,
passed in cmsg */
        if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
-                NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;
+               NAPI_GRO_CB(skb)->is_flist = sk ?
(!udp_sk(sk)->gro_enabled || udp_sk(sk)->accept_udp_fraglist) : 1;

+     /* Apply transport layer GRO if forwarding is enabled or the
flow lands at a local socket */
       if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) ||
            (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) ||
NAPI_GRO_CB(skb)->is_flist) {
                pp = call_gro_receive(udp_gro_receive_segment, head, skb);
                return pp;
        }

+      /* Continue with tunnel GRO */
"

.. not that the extra bit matters a lot. And these two conditions with
gro_enabled are not very obvious.

Just a thought.
Paolo Abeni March 29, 2021, 8:11 a.m. UTC | #2 
On Fri, 2021-03-26 at 14:15 -0400, Willem de Bruijn wrote:
> On Thu, Mar 25, 2021 at 1:24 PM Paolo Abeni <pabeni@redhat.com> wrote:

> > 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>

> 

> This looks good to me in principle, thanks for the revision.

> 

> I hadn't fully appreciated that gro_enabled implies accept_udp_l4, but

> not necessarily vice versa.

> 

> It is equivalent to (accept_udp_l4 && !up->gro_receive), right?


In this series, yes. 

> Could the extra bit be avoided with

> 

> "

> +      /* Prefer fraglist GRO unless target is a socket with UDP_GRO,

> +       * which requires all but last segments to be of same gso_size,

> passed in cmsg */

>         if (skb->dev->features & NETIF_F_GRO_FRAGLIST)

> -                NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;

> +               NAPI_GRO_CB(skb)->is_flist = sk ?

> (!udp_sk(sk)->gro_enabled || udp_sk(sk)->accept_udp_fraglist) : 1;


This is not ovious at all to me.

> +     /* Apply transport layer GRO if forwarding is enabled or the

> flow lands at a local socket */

>        if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) ||

>             (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) ||

> NAPI_GRO_CB(skb)->is_flist) {

>                 pp = call_gro_receive(udp_gro_receive_segment, head, skb);

>                 return pp;

>         }

> 

> +      /* Continue with tunnel GRO */

> "

> 

> .. not that the extra bit matters a lot. And these two conditions with

> gro_enabled are not very obvious.

> 

> Just a thought.


Overall looks more complex to me. I would keep the extra bit, unless
you have strong opinion.

Side note: I was wondering about a follow-up to simplify the condition:

	if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) ||
             (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) || NAPI_GRO_CB(skb)->is_flist) {

Since UDP sockets could process (segmenting as needed) unexpected GSO
packets, we could always do 'NETIF_F_GRO_UDP_FWD', when enabled on the
device level. The above becomes:

	if (skb->dev->features & NETIF_F_GRO_UDP_FWD) ||
            (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) ||
            NAPI_GRO_CB(skb)->is_flist) {

which is hopefully more clear (and simpler). As said, non for this
series anyhow.

Thanks,

Paolo
Willem de Bruijn March 29, 2021, 12:31 p.m. UTC | #3 
On Mon, Mar 29, 2021 at 4:14 AM Paolo Abeni <pabeni@redhat.com> wrote:
>

> On Fri, 2021-03-26 at 14:15 -0400, Willem de Bruijn wrote:

> > On Thu, Mar 25, 2021 at 1:24 PM Paolo Abeni <pabeni@redhat.com> wrote:

> > > 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>

> >

> > This looks good to me in principle, thanks for the revision.

> >

> > I hadn't fully appreciated that gro_enabled implies accept_udp_l4, but

> > not necessarily vice versa.

> >

> > It is equivalent to (accept_udp_l4 && !up->gro_receive), right?

>

> In this series, yes.

>

> > Could the extra bit be avoided with

> >

> > "

> > +      /* Prefer fraglist GRO unless target is a socket with UDP_GRO,

> > +       * which requires all but last segments to be of same gso_size,

> > passed in cmsg */

> >         if (skb->dev->features & NETIF_F_GRO_FRAGLIST)

> > -                NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;

> > +               NAPI_GRO_CB(skb)->is_flist = sk ?

> > (!udp_sk(sk)->gro_enabled || udp_sk(sk)->accept_udp_fraglist) : 1;

>

> This is not ovious at all to me.

>

> > +     /* Apply transport layer GRO if forwarding is enabled or the

> > flow lands at a local socket */

> >        if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) ||

> >             (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) ||

> > NAPI_GRO_CB(skb)->is_flist) {

> >                 pp = call_gro_receive(udp_gro_receive_segment, head, skb);

> >                 return pp;

> >         }

> >

> > +      /* Continue with tunnel GRO */

> > "

> >

> > .. not that the extra bit matters a lot. And these two conditions with

> > gro_enabled are not very obvious.

> >

> > Just a thought.

>

> Overall looks more complex to me. I would keep the extra bit, unless

> you have strong opinion.


Sounds good.

> Side note: I was wondering about a follow-up to simplify the condition:

>

>         if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) ||

>              (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) || NAPI_GRO_CB(skb)->is_flist) {

>

> Since UDP sockets could process (segmenting as needed) unexpected GSO

> packets, we could always do 'NETIF_F_GRO_UDP_FWD', when enabled on the

> device level. The above becomes:

>

>         if (skb->dev->features & NETIF_F_GRO_UDP_FWD) ||

>             (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) ||

>             NAPI_GRO_CB(skb)->is_flist) {

>

> which is hopefully more clear (and simpler). As said, non for this

> series anyhow.


UDP sockets can segment, but it is expensive. In this case I think the
simplification is not worth the possible regression.
Paolo Abeni March 29, 2021, 1:29 p.m. UTC | #4 
On Mon, 2021-03-29 at 08:31 -0400, Willem de Bruijn wrote:
> On Mon, Mar 29, 2021 at 4:14 AM Paolo Abeni <pabeni@redhat.com> wrote:

> > On Fri, 2021-03-26 at 14:15 -0400, Willem de Bruijn wrote:

> > > On Thu, Mar 25, 2021 at 1:24 PM Paolo Abeni <pabeni@redhat.com> wrote:

> > > > 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>

> > > 

> > > This looks good to me in principle, thanks for the revision.

> > > 

> > > I hadn't fully appreciated that gro_enabled implies accept_udp_l4, but

> > > not necessarily vice versa.

> > > 

> > > It is equivalent to (accept_udp_l4 && !up->gro_receive), right?

> > 

> > In this series, yes.

> > 

> > > Could the extra bit be avoided with

> > > 

> > > "

> > > +      /* Prefer fraglist GRO unless target is a socket with UDP_GRO,

> > > +       * which requires all but last segments to be of same gso_size,

> > > passed in cmsg */

> > >         if (skb->dev->features & NETIF_F_GRO_FRAGLIST)

> > > -                NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;

> > > +               NAPI_GRO_CB(skb)->is_flist = sk ?

> > > (!udp_sk(sk)->gro_enabled || udp_sk(sk)->accept_udp_fraglist) : 1;

> > 

> > This is not ovious at all to me.

> > 

> > > +     /* Apply transport layer GRO if forwarding is enabled or the

> > > flow lands at a local socket */

> > >        if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) ||

> > >             (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) ||

> > > NAPI_GRO_CB(skb)->is_flist) {

> > >                 pp = call_gro_receive(udp_gro_receive_segment, head, skb);

> > >                 return pp;

> > >         }

> > > 

> > > +      /* Continue with tunnel GRO */

> > > "

> > > 

> > > .. not that the extra bit matters a lot. And these two conditions with

> > > gro_enabled are not very obvious.

> > > 

> > > Just a thought.

> > 

> > Overall looks more complex to me. I would keep the extra bit, unless

> > you have strong opinion.

> 

> Sounds good.

> 

> > Side note: I was wondering about a follow-up to simplify the condition:

> > 

> >         if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) ||

> >              (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) || NAPI_GRO_CB(skb)->is_flist) {

> > 

> > Since UDP sockets could process (segmenting as needed) unexpected GSO

> > packets, we could always do 'NETIF_F_GRO_UDP_FWD', when enabled on the

> > device level. The above becomes:

> > 

> >         if (skb->dev->features & NETIF_F_GRO_UDP_FWD) ||

> >             (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) ||

> >             NAPI_GRO_CB(skb)->is_flist) {

> > 

> > which is hopefully more clear (and simpler). As said, non for this

> > series anyhow.

> 

> UDP sockets can segment, but it is expensive. In this case I think the

> simplification is not worth the possible regression.


No strong opinion here, I will not do the thing mentioned above.

Thanks!

Paolo
diff mbox series

Patch

diff --git a/include/linux/udp.h b/include/linux/udp.h
index aa84597bdc33c..ae58ff3b6b5b8 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 fe85dcf8c0087..c0695ce42dc53 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2666,9 +2666,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;