# Re: \[PATCH net-next v4 2/3\] virtio\_net: add optional flow dissection in virtio\_net\_hdr\_to\_skb

- From: Willem de Bruijn \<willemdebruijn.kernel@gmail.com\>
- Date: 2021-06-10 03:26:20 
- List: netdev
- Page: https://ratatoskr.run/netdev/2021/06/11205104
- Thread: https://ratatoskr.run/netdev/2021/06/11204926/t.md

```text
On Wed, Jun 9, 2021 at 11:09 PM Jason Wang [ref] wrote:
>
>
> 在 2021/6/9 上午1:02, Tanner Love 写道:
> >   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
>
```