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

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

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

quoted

quoted

        /*
         * 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. At the cost of
perhaps two branches instead of one, admittedly. But that seems
premature optimization.