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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 9 Jun 2021 23:15:25 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Tanner Love <tannerlove.kernel@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Petar Penkov <ppenkov@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "Michael S . Tsirkin" <mst@...hat.com>,
        Tanner Love <tannerlove@...gle.com>
Subject: 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 <jasowang@...hat.com> 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
>