Message-ID: <3c2290f1-827c-452d-a818-bd89f4cbbcba@redhat.com>
Date: Thu, 29 May 2025 13:55:30 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: netdev@...r.kernel.org, Willem de Bruijn
 <willemdebruijn.kernel@...il.com>, Andrew Lunn <andrew+netdev@...n.ch>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, "Michael S. Tsirkin" <mst@...hat.com>,
 Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez
 <eperezma@...hat.com>
Subject: Re: [PATCH net-next 5/8] net: implement virtio helpers to handle UDP
 GSO tunneling.

On 5/26/25 6:40 AM, Jason Wang wrote:
> On Wed, May 21, 2025 at 6:34 PM Paolo Abeni <pabeni@...hat.com> wrote:
>> +       if (!gso_inner_type || gso_inner_type == VIRTIO_NET_HDR_GSO_UDP)
>> +               return -EINVAL;
>> +
>> +       /* Relay on csum being present. */
>> +       if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
>> +               return -EINVAL;
>> +
>> +       /* Validate offsets. */
>> +       outer_isv6 = gso_tunnel_type & VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6;
>> +       inner_l3min = virtio_l3min(gso_inner_type == VIRTIO_NET_HDR_GSO_TCPV6);
>> +       outer_l3min = ETH_HLEN + virtio_l3min(outer_isv6);
>> +
>> +       tnl = ((void *)hdr) + tnl_hdr_offset;
>> +       inner_th = __virtio16_to_cpu(little_endian, hdr->csum_start);
>> +       inner_nh = __virtio16_to_cpu(little_endian, tnl->inner_nh_offset);
>> +       outer_th = __virtio16_to_cpu(little_endian, tnl->outer_th_offset);
>> +       if (outer_th < outer_l3min ||
>> +           inner_nh < outer_th + sizeof(struct udphdr) ||
>> +           inner_th < inner_nh + inner_l3min)
>> +               return -EINVAL;
> 
> I wonder if kernel has already had helpers to validate the tunnel
> headers 

Not that I know of.

> or if the above check is sufficient here.

AFAICS yes. Syzkaller is out there just to prove me wrong...


>> +
>> +       /* Let the basic parsing deal with plain GSO features. */
>> +       ret = __virtio_net_hdr_to_skb(skb, hdr, little_endian,
>> +                                     hdr->gso_type & ~gso_tunnel_type);
>> +       if (ret)
>> +               return ret;
>> +
>> +       skb_set_inner_protocol(skb, outer_isv6 ? htons(ETH_P_IPV6) :
>> +                                                htons(ETH_P_IP));
> 
> The outer_isv6 is somehow misleading here, I think we'd better rename
> it as inner_isv6?

There is bug above, thanks for spotting it. I should not use the
`outer_isv6` variable, instead I should compute separately `inner_isv6`

>> +       if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
>> +               if (!tnl_csum_negotiated)
>> +                       return -EINVAL;
>> +
>> +               skb_shinfo(skb)->gso_type |= SKB_GSO_UDP_TUNNEL_CSUM;
>> +       } else {
>> +               skb_shinfo(skb)->gso_type |= SKB_GSO_UDP_TUNNEL;
>> +       }
>> +
>> +       skb->inner_transport_header = inner_th + skb_headroom(skb);
> 
> I may miss something but using skb_headroom() means the value depends
> on the geometry of the skb and the headroom might vary depending on
> the size of the packet and other factors.  (see receive_buf())

Yes, that is correct: the actual inner_transport_header value depends on
the skb geometry, because the (inner) transport header is located at
skb->head + skb->inner_transport_header.

>> +       skb->inner_network_header = inner_nh + skb_headroom(skb);
>> +       skb->inner_mac_header = inner_nh + skb_headroom(skb);
> 
> This actually equals to inner_network_header, is this intended?

Yes. AFAICS the inner mac header field is used only for GSO/TSO.

At this point we don't know if the inner mac header is actually present
nor it's len (could include vlan tag).

Still the above allows correct segmentation by the GSO stage because the
inner mac header is not copied verbatim in the segmented packets, alike
the tunnel header.

With the above code, the inner mac header if really present will be
logically considered part of the tunnel header by the GSO stage.

Note that some devices restrict the TSO capability to some fixed values
of the UDP tunnel sizes and inner mac header. In such cases, they will
fallback to S/W GSO.

>> +       skb->transport_header = outer_th + skb_headroom(skb);
>> +       skb->encapsulation = 1;
>> +       return 0;
>> +}
>> +
>> +static inline int virtio_net_chk_data_valid(struct sk_buff *skb,
>> +                                           struct virtio_net_hdr *hdr,
>> +                                           bool tun_csum_negotiated)
> 
> This is virtio_net.h so it's better to avoid using "tun". Btw, I
> wonder why this needs to be called by the virtio-net instead of being
> called by hdr_to_skb helpers.

I can squash into virtio_net_hdr_tnl_to_skb(), I kept them separated to
avoid extra long argument lists, but we are dropping an argument from
virtio_net_hdr_tnl_to_skb(), so should be ok.

>> +{
>> +       if (!(hdr->gso_type & VIRTIO_NET_HDR_GSO_UDP_TUNNEL)) {
>> +               if (!(hdr->flags & VIRTIO_NET_HDR_F_DATA_VALID))
>> +                       return 0;
>> +
>> +               skb->ip_summed = CHECKSUM_UNNECESSARY;
>> +               if (!(hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM))
>> +                       return 0;
>> +
>> +               /* tunnel csum packets are invalid when the related
>> +                * feature has not been negotiated
>> +                */
>> +               if (!tun_csum_negotiated)
>> +                       return -EINVAL;
> 
> Should we move this check above VIRTIO_NET_HDR_F_DATA_VALID check?

It could break existing setups. We can safely do extra validation only
when we know that the UDP tunnel features have been negotiated.

>> +               skb->csum_level = 1;
>> +               return 0;
>> +       }
>> +
>> +       /* DATA_VALID is mutually exclusive with NEEDS_CSUM,
> 
> I may miss something but I think we had a discussion about this, and
> the conclusion is it's too late to fix as it may break some legacy
> devices?

I'm not sure what should be fixed here? This check implements exactly
restriction you asked for while discussing the spec. We can't have a
similar check for non UDP tunneled packets, because it could break
existing setup.

> 
>> and GSO
>> +        * over UDP tunnel requires the latter
>> +        */
>> +       if (hdr->flags & VIRTIO_NET_HDR_F_DATA_VALID)
>> +               return -EINVAL;
>> +       return 0;
>> +}
>> +
>> +static inline int virtio_net_hdr_tnl_from_skb(const struct sk_buff *skb,
>> +                                             struct virtio_net_hdr *hdr,
>> +                                             unsigned int tnl_offset,
>> +                                             bool little_endian,
>> +                                             int vlan_hlen)
>> +{
>> +       struct virtio_net_hdr_tunnel *tnl;
>> +       unsigned int inner_nh, outer_th;
>> +       int tnl_gso_type;
>> +       int ret;
>> +
>> +       tnl_gso_type = skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL |
>> +                                                   SKB_GSO_UDP_TUNNEL_CSUM);
>> +       if (!tnl_gso_type)
>> +               return virtio_net_hdr_from_skb(skb, hdr, little_endian, false,
>> +                                              vlan_hlen);
>> +
>> +       /* Tunnel support not negotiated but skb ask for it. */
>> +       if (!tnl_offset)
>> +               return -EINVAL;
> 
> Should we do BUG_ON here?

I don't think so. BUG_ON()s are explicitly discouraged to avoid crashing
the kernel on exceptional/unexpected situation.

The caller will emit rate limited warns with the relevant info, if this
is hit. The BUG_ON() stack trace will add little value.

>> +
>> +       /* Let the basic parsing deal with plain GSO features. */
>> +       skb_shinfo(skb)->gso_type &= ~tnl_gso_type;
>> +       ret = virtio_net_hdr_from_skb(skb, hdr, little_endian, false,
>> +                                     vlan_hlen);
>> +       skb_shinfo(skb)->gso_type |= tnl_gso_type;
>> +       if (ret)
>> +               return ret;
> 
> Could we do the plain GSO after setting inner flags below to avoid
> masking and unmasking tnl_gso_type?

virtio_net_hdr_from_skb() will still receive a skb with UDP tunnel GSO
type and will error out.

The masking coudl be avoided factoring out a __virtio_net_hdr_from_skb()
helper receiving an explicit gso_type argument. I can do that if it's
preferred.

/P

Powered by blists - more mailing lists