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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Jul 16, 2020 at 3:52 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Wed, Jul 15, 2020 at 8:48 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> >>
> >> >> Yup, that was helpful, thanks! I think our difference of opinion on this
> >> >> stems from the same place as our disagreement about point 2.: You are
> >> >> assuming that an application that uses XDP sticks around and holds on to
> >> >> its bpf_link, while I'm assuming it doesn't (and so has to rely on
> >> >> pinning for any persistence). In the latter case you don't really gain
> >> >> much from the bpf_link auto-cleanup, and whether it's a prog fd or a
> >> >> link fd you go find in your bpffs doesn't make that much difference...
> >> >
> >> > Right. But if I had to pick just one implementation (prog fd-based vs
> >> > bpf_link), I'd stick with bpf_link because it is flexible enough to
> >> > "emulate" prog fd attachment (through BPF FS), but the same isn't true
> >> > about prog fd attachment emulating bpf_link. That's it. I really don't
> >> > enjoy harping on that point, but it feels to be silently dismissed all
> >> > the time based on one particular arrangement for particular existing
> >> > XDP flow.
> >>
> >> It can; kinda. But you introduce a dependency on bpffs that wasn't there
> >> before, and you end up with resources that are kept around in the kernel
> >> if the interface disappears (because they are still pinned). So I
> >> consider it a poor emulation.
> >
> > Yes, it's not exactly 100% the same semantics.
> > It is possible with slight additions to API to support essentially
> > exactly the same semantics you want with prog attachment. E.g., we can
> > either have a flag at LINK_CREATE time, or a separate command (e.g.,
> > LINK_PIN or something), that would mark bpf_link as "sticky", bump
> > it's refcnt. What happens then is that even if last FD is closed,
> > there is still refcnt 1 there, and then there are two ways to detach
> > that link:
> >
> > 1) interface/cgroup/whatever is destroyed and bpf_link is
> > auto-detached. At that point auto-detach handler will see that it's a
> > "sticky" bpf_link, will decrement refcnt and subsequently free
> > bpf_link kernel object (unless some application still has FD open, of
> > course).
> >
> > 2) a new LINK_DESTROY BPF command will be introduced, which will only
> > work with "sticky" bpf_links. It will decrement refcnt and do the same
> > stuff as the auto-detach handler does today (so bpf_link->dev = NULL,
> > for XDP link).

So it seems there is an interest in having this LINK_DESTROY command
irrespective of sticky/non-sticky BPF links. I'm going to follow up
with a patch set adding this as a LINK_DETACH command (I think it's a
more proper name) generically to links that it makes most sense for.
I.e., cgroup, netns and (now) xdp links are natural candidates, as
they already have to do this as part of auto-detach. I'll take a look
at tracing/bpf_iter links as well and see how hard it is to support
something like that. Of course `bpftool link detach` command needs to
be added, etc.

Sticky links are a bit more controversial and less clear from API
stand-point, so we can consider them separately from LINK_DETACH this.

> >
> > I don't mind this, as long as this is not a default semantics and
> > require conscious opt-in from whoever creates the link.
>
> Now *this* I would like to see! I have the issue with component progs of
> the multiprog dispatcher being pinned and making everything stick
> around.
>
> [...]
>
> > Sure, thanks, enjoy your vacation! I'll post v3 then with build fixes
> > I have so far.
>
> Thanks! :)
>
> -Toke
>