netdev - Re: [PATCH RFC v3 5/8] net: implement virtio helpers to handle UDP GSO tunneling.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e0e6139b-8afd-45ea-8396-b872245d398c@redhat.com>
Date: Thu, 12 Jun 2025 12:10:00 +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>, Yuri Benditovich <yuri.benditovich@...nix.com>,
 Akihiko Odaki <akihiko.odaki@...nix.com>
Subject: Re: [PATCH RFC v3 5/8] net: implement virtio helpers to handle UDP
 GSO tunneling.

On 6/12/25 5:53 AM, Jason Wang wrote:
> On Fri, Jun 6, 2025 at 7:46 PM Paolo Abeni <pabeni@...hat.com> wrote:
>> +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);
>
> So tun_vnet_hdr_from_skb() has
>
>         int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0;
>         int tnl_offset = tun_vnet_tnl_offset(flags);
>
>         if (virtio_net_hdr_tnl_from_skb(skb, hdr, tnl_offset,
>                                         tun_vnet_is_little_endian(flags),
>                                         vlan_hlen)) {
>
>
> It looks like the outer vlan_hlen is used for the inner here?
vlan_hlen always refers to the outer vlan tag (if present), as it moves
the (inner) transport csum offset accordingly.

I can a comment to clarify the parsing.

Note that in the above call there is a single set of headers (no
encapsulation) so the vlan_hlen should be unambigous.
>> +
>> +       /* Tunnel support not negotiated but skb ask for it. */
>> +       if (!tnl_offset)
>> +               return -EINVAL;
>> +
>> +       /* 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, true, false, vlan_hlen);

Here I'll add:

	Here vlan_hlen refers to the outer headers set, but still affect
	the inner transport header offset.

>> @@ -181,6 +208,22 @@ struct virtio_net_hdr_v1_hash {
>>         __le16 padding;
>>  };
>>
>> +/* This header after hashing information */
>> +struct virtio_net_hdr_tunnel {
>> +       __le16 outer_th_offset;
>> +       __le16 inner_nh_offset;
>> +};
>> +
>> +struct virtio_net_hdr_v1_tunnel {
>> +       struct virtio_net_hdr_v1 hdr;
>> +       struct virtio_net_hdr_tunnel tnl;
>> +};
>> +
>> +struct virtio_net_hdr_v1_hash_tunnel {
>> +       struct virtio_net_hdr_v1_hash hdr;
>> +       struct virtio_net_hdr_tunnel tnl;
>> +};
>
> Not a native speaker but I realize there's probably an issue:
>
>         le32 hash_value;        (Only if VIRTIO_NET_F_HASH_REPORT
negotiated)
>         le16 hash_report;       (Only if VIRTIO_NET_F_HASH_REPORT
negotiated)
>         le16 padding_reserved;  (Only if VIRTIO_NET_F_HASH_REPORT
negotiated)
>         le16 outer_th_offset    (Only if
> VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO or VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO
> negotiated)
>         le16 inner_nh_offset;   (Only if
> VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO or VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO
> negotiated)
>         le16 outer_nh_offset;   /* Only if VIRTIO_NET_F_OUT_NET_HEADER
> negotiated */
>         /* Only if VIRTIO_NET_F_OUT_NET_HEADER or VIRTIO_NET_F_IPSEC
> negotiated */
>         union {
>                 u8 padding_reserved_2[6];
>                 struct ipsec_resource_hdr {
>                         le32 resource_id;
>                         le16 resource_type;
>                 } ipsec_resource_hdr;
>         };
>
> I thought e.g outer_th_offset should have a fixed offset then
> everything is simplified but it looks not the case here. If we decide
> to do things like this, we will end up with a very huge uAPI
> definition for different features combinations. This doesn't follow
> the existing headers for example num_buffers exist no matter if
> MRG_RXBUF is negotiated.>> At least, if we decide to go with the
dynamic offset, it seems less
> valuable to define those headers with different combinations if both
> device and driver process the vnet header piece wisely

I'm a little confused here. AFAICT the dynamic offset is
requested/mandated by the specifications: if the hash related fields are
not present, they are actually non existing and everything below moves
upward.  I think we spent together quite some time to agree on this.

If you want/intend the tunnel header to be at fixed offset inside the
virtio_hdr regardless of the negotiated features? That would yield to
slightly simpler but also slightly less efficient implementation.

Also I guess (fear mostly) some specification clarification would be needed.

/P