| 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
| ||
|
Message-ID: <CACGkMEvQ0XKR8P_XVt=GU8n=_0_ugVDw1bmm-xqAJsKfDZ-3xw@mail.gmail.com>
Date: Mon, 16 Jun 2025 11:17:41 +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>,
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 Thu, Jun 12, 2025 at 6:10 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> 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.
I see.
> >> +
> >> + /* 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.
Thanks, then I want to know if we need to care about the inner vlan or
it is something that is not supported by the kernel right now.
>
> >> @@ -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.
I'm sorry if I lose some context there.
>
> 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.
Yes. I feel it's probably too late to fix the spec. But I meant if the
header offset of tunnel gso stuff is dynamic, it's probably not need
to define:
virtio_net_hdr_v1_tunnel and virtio_net_hdr_v1_hash_tunnel
in the uAPI.
>
> Also I guess (fear mostly) some specification clarification would be needed.
>
> /P
>
Thanks
Powered by blists - more mailing lists