Date:   Tue, 31 Mar 2020 17:00:10 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        John Fastabend <john.fastabend@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        "David S. Miller" <davem@...emloft.net>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Lorenz Bauer <lmb@...udflare.com>,
        Andrey Ignatov <rdna@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        dsahern@...il.com
Subject: Re: [PATCH bpf-next 1/4] xdp: Support specifying expected existing program when attaching XDP

Daniel Borkmann <daniel@...earbox.net> writes:

> On 3/31/20 12:13 PM, Toke Høiland-Jørgensen wrote:
>> Andrii Nakryiko <andrii.nakryiko@...il.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).

-Toke

Powered by blists - more mailing lists