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

在 2021/6/10 上午11:15, Willem de Bruijn 写道:

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

quoted

在 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.

I may miss something here. I think it could be done via passing flow 
keys built by flow dissectors to vnet header validation eBPF program.

(Looking at validate_vnet_hdr(), it needs only flow keys and skb length).

Having two programs may have other advantages:

1) bpf vnet header validation can work without bpf flow dissector
2) keep bpf flow dissector simpler (unaware of vnet header), otherwise 
you need to prepare a dedicated bpf dissector for virtio-net/tap etc.

quoted

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"?

Probably.

Historically, the
struct has not seen such a rapid rate of change that I think
would warrant introducing this level of indirection.

For the rapid change, yes. But:

1) __sk_buff is part of the general uAPI, adding virtio-net fields looks 
like a layer violation and a duplication to the existing virtio-net uAPI
2) as you said below the vnet header is not self contained
3) using helpers we can make vnet header format transparent to bpf core 
(that's the way how bpf access TCP headers AFAIK)

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

Yes and we probably need to provide more context, e.g the negotiated 
features.

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.

It works but it couples virtio with bpf.

Thanks

quoted

Thanks