Re: [PATCH bpf-next 2/7] bpf, xdp: add bpf_link-based XDP attachment API

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

Andrii Nakryiko <andriin@fb.com> 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).

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.

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.

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

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

-Toke