Re: [PATCH net-next v3 0/3] virtio_net: add optional flow dissection in virtio_net_hdr_to_skb

quoted

quoted

Several questions:

1) having bpf core to know about virito-net header seems like a layer
violation, it doesn't scale as we may add new fields, actually there's
already fields that is not implemented in the spec but not Linux right now.

struct virtio_net_hdr is used by multiple interfaces, not just virtio.
The interface as is will remain, regardless of additional extensions.

If the interface is extended, the validation can be extended with it.

One possible problem is that there's no sufficient context.

The vnet header length is not a fixed value but depends on the feature
negotiation. The num_buffers (not implemented in this series) is an
example. The field doesn't not exist for legacy device if mergeable
buffer is disabled. If we decide to go with this way, we probably need
to fix this by introducing a vnet header length.

And I'm not sure it can work for all the future cases e.g the semantic
of a field may vary depends on the feature negotiated, but maybe it's
safe since it needs to set the flags.

Another note is that the spec doesn't exclude the possibility to have a
complete new vnet header format in the future. And the bpf program is
unaware of any virtio features.

We can extend the program with a version or type field, if multiple
variants appear. The callers can set this.

Thanks for the examples. As a matter of fact, I do know that kind of
extension. I proposed new fields myself this winter, to for timestamps,
pacing offload and hash info on tx:

https://lore.kernel.org/netdev/20210208185558.995292-1-willemdebruijn.kernel@gmail.com/T/#mcbd4dff966a93d61a31844c9d968e7cd4ee7f0ab (local)

Like num_buffers, those are new fields appended to the struct.

Agreed that if the semantics of the existing fields would change or
a whole new v2 type would be defined (with much stricter semantics
that time around, and validation from the start), then a type field in
the flow dissector will be needed.

That is feasible and won't have to break the BPF interface.

quoted

Just curious: can you share what extra fields may be in the pipeline?
The struct has historically not seen (m)any changes.

For extra fields, I vaguely remember we had some discussions on the
possible method to extend that, but I forget the actual features.

But spec support RSC which may reuse csum_start/offset but it looks to
me RSC is not something like Linux need.

quoted

quoted

2) virtio_net_hdr_to_skb() is not the single entry point, packet could
go via XDP

Do you mean AF_XDP?

Yes and kernel XDP as well. If the packet is redirected or transmitted,
it won't even go to virtio_net_hdr_to_skb().

Redirected packets are already in the kernel.

This is strictly a chokepoint for new packets injected from userspace.

Since there's no GSO/csum support for XDP, it's probably ok, but needs
to consider this for the future consider the multi-buffer XDP is being
developed right now, we can release those restriction.

Yes, we have to make sure not to introduce the same issues with any
XDP GSO extensions, if it comes to that.

quoted

As far as I know, vnet_hdr is the only injection
interface for complex packets that include offload instructions (GSO,
csum) -- which are the ones mostly implicated in bug reports.

Ideally, if GSO/csum is supported by XDP, it would be more simple to use
XDP I think.

That might actually reduce the odds of seeing new virtio_net_hdr extensions?

That legacy interface is here to stay, though, so we have to continue
to be prepared to handle any input that comes that way.