netdev - Re: [PATCH net-next 4/8] udp: never accept GSO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+FuTSd4m811Xa0TY=9VTtO7yqPyO7S+ugPHkNwWojuBnJRpTA@mail.gmail.com>
Date:   Wed, 24 Mar 2021 18:12:29 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Alexander Lobakin <alobakin@...me>
Subject: Re: [PATCH net-next 4/8] udp: never accept GSO_FRAGLIST packets

On Wed, Mar 24, 2021 at 3:00 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On Tue, 2021-03-23 at 22:21 -0400, Willem de Bruijn wrote:
> > On Mon, Mar 22, 2021 at 1:12 PM Paolo Abeni <pabeni@...hat.com> wrote:
> > > 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@...hat.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@...hat.com>
> > > > > ---
> > > > >  include/linux/udp.h | 10 ++++++----
> > > > >  net/ipv4/udp.c      | 12 +++++++++++-
> > > > >  2 files changed, 17 insertions(+), 5 deletions(-)
> > > > >
> > > > >         /*
> > > > >          * 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.
> >
> > 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.
>
> ok
>
> > 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 ;)

:)

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.

> /P
>
>
>