Re: [PATCH net-next 4/8] udp: never accept GSO_FRAGLIST packets

[Paolo Abeni <pabeni@redhat.com>]

On Mon, 2021-03-22 at 09:42 -0400, Willem de Bruijn wrote:
> On Sun, Mar 21, 2021 at 1:01 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 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 <pabeni@redhat.com>
> > ---
> >  include/linux/udp.h | 10 ++++++----
> >  net/ipv4/udp.c      | 12 +++++++++++-
> >  2 files changed, 17 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/udp.h b/include/linux/udp.h
> > index aa84597bdc33c..6da342f15f351 100644
> > --- a/include/linux/udp.h
> > +++ b/include/linux/udp.h
> > @@ -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?

Before this patch 'gro_enabled' was used in udp_unexpected_gso() to
check for GSO packets acceptance, after this patch such field is not
used there anymore, so does not carry explicilty the 'accept GRO
packets' semantic.

Anyway I don't have strong feeling regarding changing or not this
comment

> >         /*
> >          * 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        */
> >         __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.

I opted for an 'unsigned int' so that I could simply copy a gso_type
there.

> Current sockets that support SKB_GSO_UDP_L4 implicitly also support
> SKB_GSO_FRAGLIST. This patch makes explicit that the second is not
> supported..
> 
> >         /*
> >          * 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;
> +       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.

With the above patch 5/8 becomes messy ?!?

Thanks!

Paolo