Message-ID: <68307b4f745df_180c7829493@willemb.c.googlers.com.notmuch>
Date: Fri, 23 May 2025 09:42:39 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Paolo Abeni <pabeni@...hat.com>, 
 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.

Paolo Abeni wrote:
> 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,

SGTM!

Syzkaller will also give us coverage for the extended virtio_net_hdr
format. It has found many creative uses of that header before.

I did see the offset integrity checks you introduced when parsing the
header. Which is exactly what is needed to avoid such frivolous abuse.
They looked sufficient to me too.

> >> +/* 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.
> 

That's a great explanation thanks. Can you add it either in the commit
message or as a comment at these definitions?

Powered by blists - more mailing lists