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

On Wed, Jun 9, 2021 at 11:09 PM Jason Wang [off-list ref] wrote:

在 2021/6/9 上午1:02, Tanner Love 写道:

quoted

  retry:
-                     if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
+                     /* only if flow dissection not already done */
+                     if (!static_branch_unlikely(&sysctl_flow_dissect_vnet_hdr_key) &&
+                         !skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
                                                            NULL, 0, 0, 0,
                                                            0)) {

So I still wonder the benefit we could gain from reusing the bpf flow
dissector here. Consider the only context we need is the flow keys,

Maybe I misunderstand the comment, but the goal is not just to access
the flow keys, but to correlate data in the packet payload with the fields
of the virtio_net_header. E.g., to parse a packet payload to verify that
it matches the gso type and header length. I don't see how we could
make that two separate programs, one to parse a packet (flow dissector)
and one to validate the vnet_hdr.

we
had two choices

a1) embed the vnet header checking inside bpf flow dissector
a2) introduce a dedicated eBPF type for doing that

And we have two ways to access the vnet header

b1) via pesudo __sk_buff
b2) introduce bpf helpers

I second for a2 and b2. The main motivation is to hide the vnet header
details from the bpf subsystem.

b2 assumes that we might have many different variants of
vnet_hdr, but that all will be able to back a functional
interface like "return the header length"? Historically, the
struct has not seen such a rapid rate of change that I think
would warrant introducing this level of indirection.

The little_endian field does demonstrate one form of how
the struct can be context sensitive -- and not self describing.

I think we can capture multiple variants of the struct, if it ever
comes to that, with a versioning field. We did not add that
right away, because that can be introduced later, when a
new version arrives. But we have a plan for the eventuality.

Thanks