| 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: <CACGkMEtkbx8VZ2HAuDUbO9LStzoM6JQVcmA+6e+jM1o=r9wKow@mail.gmail.com>
Date: Mon, 26 May 2025 12:40: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>
Subject: Re: [PATCH net-next 5/8] net: implement virtio helpers to handle UDP
GSO tunneling.
On Wed, May 21, 2025 at 6:34 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.
>
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> ---
> include/linux/virtio_net.h | 177 ++++++++++++++++++++++++++++++--
> include/uapi/linux/virtio_net.h | 33 ++++++
> 2 files changed, 202 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 02a9f4dc594d0..cf9c712a67cd4 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,158 @@ 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)
Considering tunnel gso requires VERSION_1, I think there's no chance
for little_endian to be false here.
> +{
> + 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;
> + u8 gso_inner_type;
> + bool outer_isv6;
> + 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_IPV4 &&
> + gso_tunnel_type & VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6)
> + return -EINVAL;
Could be simplified with gso_tunnel_type == VIRTIO_NET_HDR_GSO_UDP_TUNNEL ?
> +
> + /* No UDP fragments over UDP tunnel. */
> + gso_inner_type = hdr->gso_type & ~(VIRTIO_NET_HDR_GSO_ECN |
> + gso_tunnel_type);
VIRTIO_NET_HDR_GSO_UDP_TUNNEL seems to be better than gso_tunnel_type here.
> + 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 or if the above check is sufficient here.
> +
> + /* 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?
> + 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())
> + 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?
> + 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.
> +{
> + 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?
> + 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?
> 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?
> +
> + /* 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?
> +
> + 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_virtio16(little_endian, inner_nh);
> + tnl->outer_th_offset = __cpu_to_virtio16(little_endian, outer_th);
little_endian should be true here as it depends on version 1.
> + return 0;
> +}
> +
> #endif /* _LINUX_VIRTIO_NET_H */
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index 963540deae66a..1f1ff88a5749f 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,12 @@ struct virtio_net_hdr_v1_hash {
> __le16 padding;
> };
>
> +/* This header after hashing information */
> +struct virtio_net_hdr_tunnel {
> + __virtio16 outer_th_offset;
> + __virtio16 inner_nh_offset;
> +};
> +
> #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