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]

Message-ID: <CA+FuTSeuq4K=nA_JPomyZv4SkQY0cGWdEf1jftx_1Znd+=tOZw@mail.gmail.com>
Date:   Thu, 10 Jun 2021 10:04:34 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        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 Thu, Jun 10, 2021 at 1:25 AM Jason Wang <jasowang@...hat.com> wrote:
>
>
> 在 2021/6/10 下午12:19, Alexei Starovoitov 写道:
> > On Wed, Jun 9, 2021 at 9:13 PM Jason Wang <jasowang@...hat.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?

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

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? 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. 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. For
instance, does the packet contain IP options? No idea.

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?