Re: [PATCH bpf-next 1/4] xdp: Support specifying expected existing program when attaching XDP
From: Andrii Nakryiko <hidden>
Date: 2020-03-31 20:19:24
Also in:
bpf
On Tue, Mar 31, 2020 at 8:00 AM Toke Høiland-Jørgensen [off-list ref] wrote:
Daniel Borkmann [off-list ref] writes:quoted
On 3/31/20 12:13 PM, Toke Høiland-Jørgensen wrote:quoted
Andrii Nakryiko [off-list ref] writes:quoted
quoted
quoted
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 ofHm... 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