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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

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).

-Toke