| 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: <2ccf883f-17f0-4eda-a851-f640fd2b6e14@redhat.com>
Date: Fri, 23 May 2025 08:09:25 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, netdev@...r.kernel.org
Cc: Jason Wang <jasowang@...hat.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 5/23/25 12:29 AM, Willem de Bruijn wrote:
> Paolo Abeni 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>
>
> No major concerns from me on this series. Much of the design
> conversations took place earlier on the virtio list.
>
> Maybe consider test coverage. If end-to-end testing requires qemu,
> then perhaps KUnit is more suitable for testing basinc to/from skb
> transformations. Just a thought.
My current idea is to follow-up on:
https://lore.kernel.org/netdev/20250522-vsock-vmtest-v8-1-367619bef134@gmail.com/
extending such infra to vhost/virtio, and implement GSO-over-UDP-tunnel
transfer with/without negotiated features on top of that.
In the longer term such infra could be used to have good code coverage
for virtio/vhost bundled into the kernel self-tests.
I hope it could be a follow-up, because I guess this series (and
especially the user-land counter-part) is going to b̵e̵ ̵a̵n̵ ̵h̵u̵g̵e̵
̵b̵l̵o̵o̵d̵b̵a̵t̵h̵ to take some time and effort in the current form.
>> ---
>> 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_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;
>> + 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;
>> +
>> + /* No UDP fragments over UDP tunnel. */
>
> What are udp fragments and why is TCP with ECN not supported?
"udp fragments" is the syncopated form of "UDP datagrams carryed by IP
fragments". I'll use UFO to be clearer ;)
The ECN part is cargo cult on my side from my original implementation
which dates back to ... a lot of time ago. A quick recheck makes me
think I could drop it. I'll have a better look and either document the
choice or drop the check in the next revision.
>> + gso_inner_type = hdr->gso_type & ~(VIRTIO_NET_HDR_GSO_ECN |
>> + gso_tunnel_type);
>> + if (!gso_inner_type || gso_inner_type == VIRTIO_NET_HDR_GSO_UDP)
>> + return -EINVAL;
>> +
>> + /* Relay on csum being present. */
>
> Rely
>
>> #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
>
> I don't quite follow this. These are not real virtio bits?
This comes directly from the recent follow-up on the virtio
specification. While the features space has been extended to 128 bit,
the 'guest offload' space is still 64bit. The 'guest offload' are
used/defined by the specification for the
VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET command, which allows the guest do
dynamically enable/disable H/W GRO at runtime.
Up to ~now each 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.
It's also not a problem from a 'guest offload' space exhaustion PoV
because there are a lot of features in the lower 64 bits range that are
_not_ guest offloads and could be reused for mapping - among them the
'reserved features' that started this somewhat problematic features
space expansion.
For more details:
https://lore.kernel.org/virtio-comment/6af50c9ada76d8168d248827e4af7c44bdfa34a8.1747826378.git.pabeni@redhat.com/T/#u
https://lore.kernel.org/virtio-comment/68c4e73a-fa9e-4e2e-8c38-ed4a322bf47e@redhat.com/
Thanks,
Paolo
Powered by blists - more mailing lists