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

[Jason Wang <jasowang@redhat.com>]

在 2021/6/10 下午10:04, Willem de Bruijn 写道:
> On Thu, Jun 10, 2021 at 1:25 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> 在 2021/6/10 下午12:19, Alexei Starovoitov 写道:
>>> On Wed, Jun 9, 2021 at 9:13 PM Jason Wang <jasowang@redhat.com> wrote:
>>>> So I wonder why not simply use helpers to access the vnet header like
>>>> how tcp-bpf access the tcp header?
>>> Short answer - speed.
>>> tcp-bpf accesses all uapi and non-uapi structs directly.
>>>
>> Ok, this makes sense. But instead of coupling device specific stuffs
>> like vnet header and neediness into general flow_keys as a context.
>>
>> It would be better to introduce a vnet header context which contains
>>
>> 1) vnet header
>> 2) flow keys
>> 3) other contexts like endian and virtio-net features
>>
>> So we preserve the performance and decouple the virtio-net stuffs from
>> general structures like flow_keys or __sk_buff.
> You are advocating for a separate BPF program that takes a vnet hdr
> and flow_keys as context and is run separately after flow dissection?


Yes.


>
> I don't understand the benefit of splitting the program in two in this manner.


It decouples a device specific attributes from the general structures 
like flow keys. We have xen-netfront, netvsc and a lot of drivers that 
works for the emulated devices. We could not add all those metadatas as 
the context of flow keys. That's why I suggest to use something more 
generic like XDP from the start. Yes, GSO stuffs was disabled by 
virtio-net on XDP but it's not something that can not be fixed. If the 
GSO and s/g support can not be done in short time, then a virtio-net 
specific BPF program still looks much better than coupling virtio-net 
metadata into flow keys or other general data structures.


>
> Your previous comment mentions two vnet_hdr definitions that can get
> out of sync. Do you mean v1 of this patch, that adds the individual
> fields to bpf_flow_dissector?


No, I meant this part of the patch:


+static int check_virtio_net_hdr_access(struct bpf_verifier_env *env, 
int off,
+                       int size)
+{
+    if (size < 0 || off < 0 ||
+        (u64)off + size > sizeof(struct virtio_net_hdr)) {
+        verbose(env, "invalid access to virtio_net_hdr off=%d size=%d\n",
+            off, size);
+        return -EACCES;
+    }
+    return 0;
+}
+


It prevents the program from accessing e.g num_buffers.


> That is no longer the case: the latest
> version directly access the real struct. As Alexei points out, doing
> this does not set virtio_net_hdr in stone in the ABI. That is a valid
> worry. But so this patch series will not restrict how that struct may
> develop over time. A version field allows a BPF program to parse the
> different variants of the struct -- in the same manner as other
> protocol headers.


The format of the virtio-net header depends on the virtio features, any 
reason for another version? The correct way is to provide features in 
the context, in this case you don't event need the endian hint.


> If you prefer, we can add that field from the start.
> I don't see a benefit to an extra layer of indirection in the form of
> helper functions.
>
> I do see downsides to splitting the program. The goal is to ensure
> consistency between vnet_hdr and packet payload. A program split
> limits to checking vnet_hdr against what the flow_keys struct has
> extracted. That is a great reduction over full packet access.


Full packet access could be still done in bpf flow dissector.


> For
> instance, does the packet contain IP options? No idea.


I don't understand here. You can figure out this in flow dissector, and 
you can extend the flow keys to carry out this information if necessary.

And if you want to have more capability, XDP which is designed for early 
packet filtering is the right way to go which have even more functions 
that a simple bpf flow dissector.


>
> If stable ABI is not a concern and there are no different struct
> definitions that can go out of sync, does that address your main
> concerns?


I think not. Assuming we provide sufficient contexts (e.g the virtio 
features), problem still: 1) coupling virtio-net with flow_keys 2) can't 
work for XDP.

Thanks


>