Message-ID: <CACGkMEu40tJYBuXmkEzJ5MeYHQcZBri2e7iKq9RiU20bSk9T-Q@mail.gmail.com>
Date: Tue, 3 Jun 2025 10:11:27 +0800
From: Jason Wang <jasowang@...hat.com>
To: Paolo Abeni <pabeni@...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 Thu, May 29, 2025 at 7:55 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> 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.


Right, I see. Btw, is skb_set_inner_transport_header() considered to
be better here?

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

Ok.

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

You are right.

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

Right.

>
> >
> >> 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.

Ok.

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

That would be fine.

Thanks

>
> /P
>

Powered by blists - more mailing lists