Re: [PATCH bpf 1/2] flow_dissector: support FLOW_DISSECTOR_KEY_ETH_ADDRS with BPF

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Stanislav Fomichev <sdf@fomichev.me>
Cc: Stanislav Fomichev <sdf@google.com>,
	Network Development <netdev@vger.kernel.org>,
	bpf <bpf@vger.kernel.org>, David Miller <davem@davemloft.net>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Willem de Bruijn <willemb@google.com>,
	Petar Penkov <ppenkov@google.com>
Subject: Re: [PATCH bpf 1/2] flow_dissector: support FLOW_DISSECTOR_KEY_ETH_ADDRS with BPF
Date: Mon, 13 May 2019 18:47:03 -0400	[thread overview]
Message-ID: <CAF=yD-Lg16ETT09=fRd2FTx2FJoGZ9K0s-JHrhv-9OMTqE+5BQ@mail.gmail.com> (raw)
In-Reply-To: <CAF=yD-JKbzuoF_q7gPRjMNCBexn4pxgQ6pTeQSRfPXmwWk5dzg@mail.gmail.com>

On Mon, May 13, 2019 at 5:21 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Mon, May 13, 2019 at 5:02 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 05/13, Willem de Bruijn wrote:
> > > On Mon, May 13, 2019 at 3:53 PM Stanislav Fomichev <sdf@google.com> wrote:
> > > >
> > > > If we have a flow dissector BPF program attached to the namespace,
> > > > FLOW_DISSECTOR_KEY_ETH_ADDRS won't trigger because we exit early.
> > >
> > > I suppose that this is true for a variety of keys? For instance, also
> > > FLOW_DISSECTOR_KEY_IPV4_ADDRS.
>
> > I though the intent was to support most of the basic stuff (eth/ip/tcp/udp)
> > without any esoteric protocols.
>
> Indeed. But this applies both to protocols and the feature set. Both
> are more limited.
>
> > Not sure about FLOW_DISSECTOR_KEY_IPV4_ADDRS,
> > looks like we support that (except FLOW_DISSECTOR_KEY_TIPC part).
>
> Ah, I chose a bad example then.
>
> > > We originally intended BPF flow dissection for all paths except
> > > tc_flower. As that catches all the vulnerable cases on the ingress
> > > path on the one hand and it is infeasible to support all the
> > > flower features, now and future. I think that is the real fix.
>
> > Sorry, didn't get what you meant by the real fix.
> > Don't care about tc_flower? Just support a minimal set of features
> > needed by selftests?
>
> I do mean exclude BPF flow dissector (only) for tc_flower, as we
> cannot guarantee that the BPF program can fully implement the
> requested feature.

Though, the user inserting the BPF flow dissector is the same as the
user inserting the flower program, the (per netns) admin. So arguably
is aware of the constraints incurred by BPF flow dissection. And else
can still detect when a feature is not supported from the (lack of)
output, as in this case of Ethernet address. I don't think we want to
mix BPF and non-BPF flow dissection though. That subverts the safety
argument of switching to BPF for flow dissection.