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

On Wed, 2021-03-24 at 18:12 -0400, Willem de Bruijn wrote:

On Wed, Mar 24, 2021 at 3:00 PM Paolo Abeni [off-list ref] wrote:

quoted

On Tue, 2021-03-23 at 22:21 -0400, Willem de Bruijn wrote:

quoted

On Mon, Mar 22, 2021 at 1:12 PM Paolo Abeni [off-list ref] wrote:

quoted

On Mon, 2021-03-22 at 09:42 -0400, Willem de Bruijn wrote:

quoted

On Sun, Mar 21, 2021 at 1:01 PM Paolo Abeni [off-list ref] wrote:

quoted

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

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

quoted

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.

ok.

I'm only doubtful about the likely() annotation: systems with UDP
tunnels likely expect receiving a majority of UDP-encaped traffic,
which in turn will likely be GRO (e.g. TCP over UDP-tunnel).

In my next iteration I'll use the above, dropping the annotation.

Cheers,

Paolo