Re: [PATCH bpf-next 1/4] xdp: Support specifying expected existing program when attaching XDP

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	John Fastabend <john.fastabend@gmail.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>, Andrii Nakryiko <andriin@fb.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jesper Dangaard Brouer <brouer@redhat.com>,
	Lorenz Bauer <lmb@cloudflare.com>, Andrey Ignatov <rdna@fb.com>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	David Ahern <dsahern@gmail.com>
Subject: Re: [PATCH bpf-next 1/4] xdp: Support specifying expected existing program when attaching XDP
Date: Tue, 31 Mar 2020 13:19:10 -0700	[thread overview]
Message-ID: <CAEf4BzaQANTPcWQu=0m=K9=CEFboBLN36a0B2XeX+qjuPdQ=8w@mail.gmail.com> (raw)
In-Reply-To: <87blocin7p.fsf@toke.dk>

On Tue, Mar 31, 2020 at 8:00 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Daniel Borkmann <daniel@iogearbox.net> writes:
>
> > On 3/31/20 12:13 PM, Toke Høiland-Jørgensen wrote:
> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> >>
> >>>>> So you install your libxdp-based firewalls and are happy. Then you
> >>>>> decide to install this awesome packet analyzer, which doesn't know
> >>>>> about libxdp yet. Suddenly, you get all packets analyzer, but no more
> >>>>> firewall, until users somehow notices that it's gone. Or firewall
> >>>>> periodically checks that it's still runinng. Both not great, IMO, but
> >>>>> might be acceptable for some users, I guess. But imagine all the
> >>>>> confusion for user, especially if he doesn't give a damn about XDP and
> >>>>> other buzzwords, but only needs a reliable firewall :)
> >>>>
> >>>> Yes, whereas if the firewall is using bpf_link, then the packet analyser
> >>>> will be locked out and can't do its thing. Either way you end up with a
> >>>> broken application; it's just moving the breakage. In the case of
> >>>
> >>> Hm... In one case firewall installation reported success and stopped
> >>> working afterwards with no notification and user having no clue. In
> >>> another, packet analyzer refused to start and reported error to user.
> >>> Let's agree to disagree that those are not at all equivalent. To me
> >>> silent failure is so much worse, than application failing to start in
> >>> the first place.
> >
> > I sort of agree with both of you that either case is not great. The silent
> > override we currently have is not great since it can be evicted at any time
> > but also bpf_link to lock-out other programs at XDP layer is not great either
> > since there is also huge potential to break existing programs. It's probably
> > best to discuss on an actual proposal to see the concrete semantics, but my
> > concerns, assuming I didn't misunderstand or got confused on something along
> > the way (if so, please let me know), currently are:
>
> I think you're summarising the issues well, with perhaps one thing
> missing: The goal is to enable multi-prog execution, i.e., execute two
> programs in sequence. So, when things work correctly the flow should be:
>
> App1, loading prog1:
> - get current program from $IFACE
> - current program is NULL:
>   -> build dispatcher(prog1)
>   -> load dispatcher onto $IFACE with UPDATE_IF_NOEXIST flag
>   -> success
>
> Then, app2 loading prog2:
> - get current program from $IFACE
> - current program is dispatcher(prog1):
>   -> build new dispatcher(prog1,prog2)
>   -> atomically replace old dispatcher with new one
>   -> success
>
> As long as app1 and app2 agree on what a dispatcher looks like, and how
> to update it, they can cooperatively install themselves in the chain, as
> long as there's a way to resolve the race between reading and updating
> the state in the kernel.
>
> However, if they *don't* agree on how to build the dispatcher and run in
> sequence, they are fundamentally incompatible. Which also means that
> multi-prog operation is going to be incompatible with any application
> that was written before it was implemented. The only way to avoid that
> is to provide the multi-prog support in the kernel, in a way that is
> compatible with the old API. I'm not sure if this is even possible; but
> I certainly got a very emphatic NACK on any attempt to implement the
> support in the kernel when I posted my initial patch back in the fall.
>
> Also, to your point about needing a specific library: I've been saying
> "using the same library" because I think that is the most likely way to
> get applications to agree. But really, what's needed is more like a
> protocol; there could in theory be several independent implementations
> that interoperate. However, I don't see a way to make things compatible
> with applications that don't follow that protocol; we only get to pick
> the failure mode (and those failure modes I think you summarised quite
> well).

Well, for once we agree with Toke in this thread (regarding last two
paragraphs) :)

>
> -Toke
>