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

On Mon, Mar 29, 2021 at 4:14 AM Paolo Abeni [off-list ref] wrote:

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

quoted

On Thu, Mar 25, 2021 at 1:24 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 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.

quoted

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.

quoted

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