On Tue, Jul 14, 2020 at 6:57 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>
> Andrii Nakryiko <andriin@xxxxxx> writes:
>
> > Add bpf_link-based API (bpf_xdp_link) to attach BPF XDP program through
> > BPF_LINK_CREATE command.
>
> I'm still not convinced this is a good idea. As far as I can tell, at
> this point adding this gets you three things:
>
> 1. The ability to 'lock' an attachment in place.
>
> 2. Automatic detach on fd close
>
> 3. API unification with other uses of BPF_LINK_CREATE.
>
>
> Of those, 1. is certainly useful, but can be trivially achieved with the
> existing netlink API (add a flag on attach that prevents removal unless
> the original prog_fd is supplied as EXPECTED_FD).
Given it's trivial to discover attached prog FD on a given ifindex, it
doesn't add much of a peace of mind to the application that installs
bpf_link. Any other XDP-enabled program (even some trivial test
program) can unknowingly break other applications by deciding to
"auto-cleanup" it's previous instance on restart ("what's my previous
prog FD? let's replace it with my up-to-date program FD! What do you
mean it wasn't my prog FD before?). We went over this discussion many
times already: relying on the correct behavior of *other*
applications, which you don't necessarily control, is not working well
in real production use cases.
>
> 2. is IMO the wrong model for XDP, as I believe I argued the last time
> we discussed this :)
> In particular, in a situation with multiple XDP programs attached
> through a dispatcher, the 'owner' application of each program don't
> 'own' the interface attachment anyway, so if using bpf_link for that it
> would have to be pinned somewhere anyway. So the 'automatic detach'
> feature is only useful in the "xdpd" deployment scenario, whereas in the
> common usage model of command-line attachment ('ip link set xdp...') it
> is something that needs to be worked around.
Right, nothing changed since we last discussed. There are cases where
one or another approach is more convenient. Having bpf_link for XDP
finally gives an option to have an auto-detaching (on last FD close)
approach, but you still insist there shouldn't be such an option. Why?
>
> 3. would be kinda nice, I guess, if we were designing the API from
> scratch. But we already have an existing API, so IMO the cost of
> duplication outweighs any benefits of API unification.
Not unification of BPF_LINK_CREATE, but unification of bpf_link
infrastructure in general, with its introspection and discoverability
APIs. bpftool can show which programs are attached where and it can
show PIDs of processes that own the BPF link. With CAP_BPF you have
also more options now how to control who can mess with your bpf_link.
>
> So why is XDP worth it? I assume you weigh this differently, but please
> explain how. Ideally, this should have been in the commit message
> already...
It's the 6th BPF link class we are implementing, I didn't think I
needed to go over all the same general points again. I can point to
patches originally adding BPF link for justification, I suppose.
>
> > bpf_xdp_link is mutually exclusive with direct BPF program attachment,
> > previous BPF program should be detached prior to attempting to create a new
> > bpf_xdp_link attachment (for a given XDP mode). Once link is attached, it
> > can't be replaced by other BPF program attachment or link attachment. It will
> > be detached only when the last BPF link FD is closed.
>
> I was under the impression that forcible attachment of bpf_links was
> already possible, but looking at the code now it doesn't appear to be?
> Wasn't that the whole point of BPF_LINK_GET_FD_BY_ID? I.e., that a
> sysadmin with CAP_SYS_ADMIN privs could grab the offending bpf_link FD
> and force-remove it? I certainly think this should be added before we
> expand bpf_link usage any more...
I still maintain that killing processes that installed the bpf_link is
the better approach. Instead of letting the process believe and act as
if it has an active XDP program, while it doesn't, it's better to
altogether kill/restart the process.
>
> -Toke
>
