| 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: <CACGkMEsBsX-3ztNkQTH+J_32LcFaMwv-pOpTX0rXdLMmCj+JAA@mail.gmail.com>
Date: Thu, 12 Jun 2025 11:53:15 +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 Fri, Jun 6, 2025 at 7:46 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> The virtio specification are introducing support for GSO over
> UDP tunnel.
>
> This patch brings in the needed defines and the additional
> virtio hdr parsing/building helpers.
>
> The UDP tunnel support uses additional fields in the virtio hdr,
> and such fields location can change depending on other negotiated
> features - specifically VIRTIO_NET_F_HASH_REPORT.
>
> Try to be as conservative as possible with the new field validation.
>
> Existing implementation for plain GSO offloads allow for invalid/
> self-contradictory values of such fields. With GSO over UDP tunnel we can
> be more strict, with no need to deal with legacy implementation.
>
> Since the checksum-related field validation is asymmetric in the driver
> and in the device, introduce a separate helper to implement the new checks
> (to be used only on the driver side).
>
> Note that while the feature space exceeds the 64-bit boundaries, the
> guest offload space is fixed by the specification of the
> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command to a 64-bit size.
>
> Prior to the UDP tunnel GSO support, each guest offload bit corresponded
> to the feature bit with the same value and vice versa.
>
> Due to the limited 'guest offload' space, relevant features in the high
> 64 bits are 'mapped' to free bits in the lower range. That is simpler
> than defining a new command (and associated features) to exchange an
> extended guest offloads set.
>
> As a consequence, the uAPIs also specify the mapped guest offload value
> corresponding to the UDP tunnel GSO features.
>
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> --
> v2 -> v3:
> - add definitions for possible vnet hdr layouts with tunnel support
>
> v1 -> v2:
> - 'relay' -> 'rely' typo
> - less unclear comment WRT enforced inner GSO checks
> - inner header fields are allowed only with 'modern' virtio,
> thus are always le
> - clarified in the commit message the need for 'mapped features'
> defines
> - assume little_endian is true when UDP GSO is enabled.
> - fix inner proto type value
> ---
> include/linux/virtio_net.h | 191 ++++++++++++++++++++++++++++++--
> include/uapi/linux/virtio_net.h | 43 +++++++
> 2 files changed, 226 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 02a9f4dc594d..bcf80534a739 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -47,9 +47,9 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
> return 0;
> }
>
> -static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> - const struct virtio_net_hdr *hdr,
> - bool little_endian)
> +static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> + const struct virtio_net_hdr *hdr,
> + bool little_endian, u8 hdr_gso_type)
> {
> unsigned int nh_min_len = sizeof(struct iphdr);
> unsigned int gso_type = 0;
> @@ -57,8 +57,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> unsigned int p_off = 0;
> unsigned int ip_proto;
>
> - if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> - switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> + if (hdr_gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> + switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> case VIRTIO_NET_HDR_GSO_TCPV4:
> gso_type = SKB_GSO_TCPV4;
> ip_proto = IPPROTO_TCP;
> @@ -84,7 +84,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> return -EINVAL;
> }
>
> - if (hdr->gso_type & VIRTIO_NET_HDR_GSO_ECN)
> + if (hdr_gso_type & VIRTIO_NET_HDR_GSO_ECN)
> gso_type |= SKB_GSO_TCP_ECN;
>
> if (hdr->gso_size == 0)
> @@ -122,7 +122,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>
> if (!protocol)
> virtio_net_hdr_set_proto(skb, hdr);
> - else if (!virtio_net_hdr_match_proto(protocol, hdr->gso_type))
> + else if (!virtio_net_hdr_match_proto(protocol, hdr_gso_type))
> return -EINVAL;
> else
> skb->protocol = protocol;
> @@ -153,7 +153,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> }
> }
>
> - if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> + if (hdr_gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> u16 gso_size = __virtio16_to_cpu(little_endian, hdr->gso_size);
> unsigned int nh_off = p_off;
> struct skb_shared_info *shinfo = skb_shinfo(skb);
> @@ -199,6 +199,13 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> return 0;
> }
>
> +static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> + const struct virtio_net_hdr *hdr,
> + bool little_endian)
> +{
> + return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type);
> +}
> +
> static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
> struct virtio_net_hdr *hdr,
> bool little_endian,
> @@ -242,4 +249,172 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
> return 0;
> }
>
> +static inline unsigned int virtio_l3min(bool is_ipv6)
> +{
> + return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
> +}
> +
> +static inline int virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> + const struct virtio_net_hdr *hdr,
> + unsigned int tnl_hdr_offset,
> + bool tnl_csum_negotiated,
> + bool little_endian)
> +{
> + u8 gso_tunnel_type = hdr->gso_type & VIRTIO_NET_HDR_GSO_UDP_TUNNEL;
> + unsigned int inner_nh, outer_th, inner_th;
> + unsigned int inner_l3min, outer_l3min;
> + struct virtio_net_hdr_tunnel *tnl;
> + bool outer_isv6, inner_isv6;
> + u8 gso_inner_type;
> + int ret;
> +
> + if (!gso_tunnel_type)
> + return virtio_net_hdr_to_skb(skb, hdr, little_endian);
> +
> + /* Tunnel not supported/negotiated, but the hdr asks for it. */
> + if (!tnl_hdr_offset)
> + return -EINVAL;
> +
> + /* Either ipv4 or ipv6. */
> + if (gso_tunnel_type == VIRTIO_NET_HDR_GSO_UDP_TUNNEL)
> + return -EINVAL;
> +
> + /* The UDP tunnel must carry a GSO packet, but no UFO. */
> + gso_inner_type = hdr->gso_type & ~(VIRTIO_NET_HDR_GSO_ECN |
> + VIRTIO_NET_HDR_GSO_UDP_TUNNEL);
> + if (!gso_inner_type || gso_inner_type == VIRTIO_NET_HDR_GSO_UDP)
> + return -EINVAL;
> +
> + /* Rely 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_isv6 = gso_inner_type == VIRTIO_NET_HDR_GSO_TCPV6;
> + inner_l3min = virtio_l3min(inner_isv6);
> + 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 = le16_to_cpu(tnl->inner_nh_offset);
> + outer_th = le16_to_cpu(tnl->outer_th_offset);
> + if (outer_th < outer_l3min ||
> + inner_nh < outer_th + sizeof(struct udphdr) ||
> + inner_th < inner_nh + inner_l3min)
> + return -EINVAL;
> +
> + /* Let the basic parsing deal with plain GSO features. */
> + ret = __virtio_net_hdr_to_skb(skb, hdr, true,
> + hdr->gso_type & ~gso_tunnel_type);
> + if (ret)
> + return ret;
> +
> + /* In case of USO, the inner protocol is still unknown and
> + * `inner_isv6` is just a guess, additional parsing is needed.
> + * The previous validation ensures that accessing an ipv4 inner
> + * network header is safe.
> + */
> + if (gso_inner_type == VIRTIO_NET_HDR_GSO_UDP_L4) {
> + struct iphdr *iphdr = (struct iphdr *)(skb->data + inner_nh);
> +
> + inner_isv6 = iphdr->version == 6;
> + inner_l3min = virtio_l3min(inner_isv6);
> + if (inner_th < inner_nh + inner_l3min)
> + return -EINVAL;
> + }
> +
> + skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
> + htons(ETH_P_IP));
> + 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);
> + skb->inner_network_header = inner_nh + skb_headroom(skb);
> + skb->inner_mac_header = inner_nh + skb_headroom(skb);
> + skb->transport_header = outer_th + skb_headroom(skb);
> + skb->encapsulation = 1;
> + return 0;
> +}
> +
> +/* Checksum-related fields validation for the driver */
> +static inline int virtio_net_chk_data_valid(struct sk_buff *skb,
> + struct virtio_net_hdr *hdr,
> + bool tnl_csum_negotiated)
> +{
> + 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 (!tnl_csum_negotiated)
> + return -EINVAL;
> + skb->csum_level = 1;
> + return 0;
> + }
> +
> + /* DATA_VALID is mutually exclusive with NEEDS_CSUM, 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);
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?
> +
> + /* 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);
> + skb_shinfo(skb)->gso_type |= tnl_gso_type;
> + if (ret)
> + return ret;
> +
> + if (skb->protocol == htons(ETH_P_IPV6))
> + hdr->gso_type |= VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6;
> + else
> + hdr->gso_type |= VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4;
> +
> + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL_CSUM)
> + hdr->flags |= VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM;
> +
> + tnl = ((void *)hdr) + tnl_offset;
> + inner_nh = skb->inner_network_header - skb_headroom(skb);
> + outer_th = skb->transport_header - skb_headroom(skb);
> + tnl->inner_nh_offset = cpu_to_le16(inner_nh);
> + tnl->outer_th_offset = cpu_to_le16(outer_th);
> + return 0;
> +}
> +
> #endif /* _LINUX_VIRTIO_NET_H */
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index 963540deae66..313761be99b2 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -70,6 +70,28 @@
> * with the same MAC.
> */
> #define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
> +#define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO 65 /* Driver can receive
> + * GSO-over-UDP-tunnel packets
> + */
> +#define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM 66 /* Driver handles
> + * GSO-over-UDP-tunnel
> + * packets with partial csum
> + * for the outer header
> + */
> +#define VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO 67 /* Device can receive
> + * GSO-over-UDP-tunnel packets
> + */
> +#define VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO_CSUM 68 /* Device handles
> + * GSO-over-UDP-tunnel
> + * packets with partial csum
> + * for the outer header
> + */
> +
> +/* Offloads bits corresponding to VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO{,_CSUM}
> + * features
> + */
> +#define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_MAPPED 46
> +#define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM_MAPPED 47
>
> #ifndef VIRTIO_NET_NO_LEGACY
> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
> @@ -131,12 +153,17 @@ struct virtio_net_hdr_v1 {
> #define VIRTIO_NET_HDR_F_NEEDS_CSUM 1 /* Use csum_start, csum_offset */
> #define VIRTIO_NET_HDR_F_DATA_VALID 2 /* Csum is valid */
> #define VIRTIO_NET_HDR_F_RSC_INFO 4 /* rsc info in csum_ fields */
> +#define VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM 8 /* UDP tunnel requires csum offload */
> __u8 flags;
> #define VIRTIO_NET_HDR_GSO_NONE 0 /* Not a GSO frame */
> #define VIRTIO_NET_HDR_GSO_TCPV4 1 /* GSO frame, IPv4 TCP (TSO) */
> #define VIRTIO_NET_HDR_GSO_UDP 3 /* GSO frame, IPv4 UDP (UFO) */
> #define VIRTIO_NET_HDR_GSO_TCPV6 4 /* GSO frame, IPv6 TCP */
> #define VIRTIO_NET_HDR_GSO_UDP_L4 5 /* GSO frame, IPv4& IPv6 UDP (USO) */
> +#define VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 0x20 /* UDP over IPv4 tunnel present */
> +#define VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 0x40 /* UDP over IPv6 tunnel present */
> +#define VIRTIO_NET_HDR_GSO_UDP_TUNNEL (VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 | \
> + VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6)
> #define VIRTIO_NET_HDR_GSO_ECN 0x80 /* TCP has ECN set */
> __u8 gso_type;
> __virtio16 hdr_len; /* Ethernet + IP + tcp/udp hdrs */
> @@ -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
> +
> #ifndef VIRTIO_NET_NO_LEGACY
> /* This header comes first in the scatter-gather list.
> * For legacy virtio, if VIRTIO_F_ANY_LAYOUT is not negotiated, it must
> --
> 2.49.0
>
Thanks
Powered by blists - more mailing lists