Date:   Fri, 27 Mar 2020 12:46:08 +0100
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     John Fastabend <john.fastabend@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        "David S. Miller" <davem@...emloft.net>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Lorenz Bauer <lmb@...udflare.com>,
        Andrey Ignatov <rdna@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next 1/4] xdp: Support specifying expected existing program when attaching XDP

Andrii Nakryiko <andrii.nakryiko@...il.com> writes:

> On Thu, Mar 26, 2020 at 5:35 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@...il.com> writes:
>>
>> > Now for XDP. It has same flawed model. And even if it seems to you
>> > that it's not a big issue, and even if Jakub thinks we are trying to
>> > solve non-existing problem, it is a real problem and a real concern
>> > from people that have to support XDP in production with many
>> > well-meaning developers developing BPF applications independently.
>> > Copying what you wrote in another thread:
>> >
>> >> Setting aside the question of which is the best abstraction to represent
>> >> an attachment, it seems to me that the actual behavioural problem (XDP
>> >> programs being overridden by mistake) would be solvable by this patch,
>> >> assuming well-behaved userspace applications.
>> >
>> > ... this is a horrible and unrealistic assumption that we just cannot
>> > make and accept. However well-behaved userspace applications are, they
>> > are written by people that make mistakes. And rather than blissfully
>> > expect that everything will be fine, we want to have enforcements in
>> > place that will prevent some buggy application to wreck havoc in
>> > production.
>>
>> Look, I'm not trying to tell you how to managed your internal systems.
>> I'm just objecting to your assertion that your deployment model is the
>> only one that can possibly work, and the refusal to consider other
>> alternatives that comes with it.
>
> Your assumption doesn't work for us. Because of that we need something
> like bpf_link.

I'm not disputing what you need for your use case; you obviously know
better than me. I'm really just saying that your use case is not
everyone's use case.

> Existing attachment API doesn't go away and is still supported. Feel
> free to use existing API.

As far as I'm concerned that's what I'm trying to do. This patch series
is really just fixing a bug in the existing API; to which the response
was "no, that API is fundamentally broken, you have to use bpf_link
instead". And *that* is what I am disputing.

(I do have some reservations about details of bpf_link, see below, but
I'm not actually totally against the whole concept).

>> > 1. bpf_link represents a connection (pairing?) of BPF program and some
>> > BPF hook it is attached to. BPF hook could be perf event, cgroup,
>> > netdev, etc. It's a completely independent object in itself, along the
>> > bpf_map and bpf_prog, which has its own lifetime and kernel
>> > representation. To user-space application it is returned as an
>> > installed FD, similar to loaded BPF program and BPF map. It is
>> > important that it's not just a BPF program, because BPF program can be
>> > attached to multiple BPF hooks (e.g., same XDP program can be attached
>> > to multiple interface; same kprobe handler can be installed multiple
>> > times), which means that having BPF program FD isn't enough to
>> > uniquely represent that one specific BPF program attachment and detach
>> > it or query it. Having kernel object for this allows to encapsulate
>> > all these various details of what is attached were and present to
>> > user-space a single handle (FD) to work with.
>>
>> For XDP there is already a unique handle, it's just implicit: Each
>> netdev can have exactly one XDP program loaded. So I don't really see
>> how bpf_link adds anything, other than another API for the same thing?
>
> I certainly failed to explain things clearly if you are still asking
> this. See point #2, once you attach bpf_link you can't just replace
> it. This is what XDP doesn't have right now.

Those are two different things, though. I get that #2 is a new
capability provided by bpf_link, I was just saying #1 isn't (for XDP).

>> > 2. Due to having FD associated with bpf_link, it's not possible to
>> > talk about "owning" bpf_link. If application created link and never
>> > shared its FD with any other application, it is the sole owner of it.
>> > But it also means that you can share it, if you need it. Now, once
>> > application closes FD or app crashes and kernel automatically closes
>> > that FD, bpf_link refcount is decremented. If it was the last or only
>> > FD, it will trigger automatica detachment and clean up of that
>> > particular BPF program attachment. Note, not a clean up of BPF
>> > program, which can still be attached somewhere else: only that
>> > particular attachment.
>>
>> This behaviour is actually one of my reservations against bpf_link for
>> XDP: I think that automatically detaching XDP programs when the FD is
>> closed is very much the wrong behaviour. An XDP program processes
>> packets, and when loading one I very much expect it to keep doing that
>> until I explicitly tell it to stop.
>
> As you mentioned earlier, "it's not the only one mode". Just like with
> tracing APIs, you can imagine scripts that would adds their
> packet-sniffing XDP program temporarily. If they crash, "temporarily"
> turns into "permanently, but no one knows". This is bad. And again,
> it's a choice, just with a default to auto-cleanup, because it's safe,
> even if it requires extra step for applications willing to do
> permanent XDP attachment.

Well, there are two aspects to this: One is what should be the default -
I'd argue that for XDP the most common case is 'permanent attachment'.
But that can be worked around at the library level, so it's not that
important (just a bit annoying for the library implementer, which just
so happens to be me in this case :)).

The more important problem is that with "attach link + pin", we need two
operations. So with that there is no longer a way to atomically do a
permanent attach. And also there are two pieces of state (the pinned
bpf_link + the attachment of that to the interface).

>> > 3. This derives from the concept of ownership of bpf_link. Once
>> > bpf_link is attached, no other application that doesn't own that
>> > bpf_link can replace, detach or modify the link. For some cases it
>> > doesn't matter. E.g., for tracing, all attachment to the same fentry
>> > trampoline are completely independent. But for other cases this is
>> > crucial property. E.g., when you attach BPF program in an exclusive
>> > (single) mode, it means that particular cgroup and any of its children
>> > cgroups can have any more BPF programs attached. This is important for
>> > container management systems to enforce invariants and correct
>> > functioning of the system. Right now it's very easy to violate that -
>> > you just go and attach your own BPF program, and previous BPF program
>> > gets automatically detached without original application that put it
>> > there knowing about this. Chaos ensues after that and real people have
>> > to deal with this. Which is why existing
>> > BPF_PROG_ATTACH/BPF_PROG_DETACH API is inadequate and we are adding
>> > bpf_link support.
>>
>> I can totally see how having an option to enforce a policy such as
>> locking out others from installing cgroup BPF programs is useful. But
>> such an option is just that: policy. So building this policy in as a
>> fundamental property of the API seems like a bad idea; that is
>> effectively enforcing policy in the kernel, isn't it?
>
> I hope we won't go into a dictionary definition of what "policy" means
> here :). For me it's about guarantee that kernel gives to user-space.
> bpf_link doesn't care about dictating policies. If you don't want this
> guarantee - don't use bpf_link, use direct program attachment. As
> simple as that. Policy is implemented by user-space application by
> using APIs with just the right guarantees.

Yes, but the user-space application shouldn't get to choose the policy -
the system administrator should. So an application should be able to
*request* this behaviour, but it should be a policy decision whether to
allow it. If the "locking" behaviour is built-in to the API, that
separation becomes impossible.

>> > Those same folks have similar concern with XDP. In the world where
>> > container management installs "root" XDP program which other user
>> > applications can plug into (libxdp use case, right?), it's crucial to
>> > ensure that this root XDP program is not accidentally overwritten by
>> > some well-meaning, but not overly cautious developer experimenting in
>> > his own container with XDP programs. This is where bpf_link ownership
>> > plays a huge role. Tupperware agent (FB's container management agent)
>> > would install root XDP program and will hold onto this bpf_link
>> > without sharing it with other applications. That will guarantee that
>> > the system will be stable and can't be compromised.
>>
>> See this is where we get into "deployment-model specific territory". I
>> mean, sure, in the "central management daemon" model, it makes sense
>> that no other applications can replace the XDP program. But, erm, we
>> already have a mechanism to ensure that: Just don't grant those
>> applications CAP_NET_ADMIN? So again, bpf_link doesn't really seem to
>> add anything other than a different way to do the same thing?
>
> Because there are still applications that need CAP_NET_ADMIN in order
> to function (for other reasons than attaching XDP), so it's impossible
> to enforce with for everyone.

But if you grant an application CAP_NET_ADMIN, it can wreak all sorts of
havoc (the most obvious being just issuing 'ip link down' on the iface).
So you're implicitly trusting it to be well-behaved, so why does this
particular act of misbehaviour need a special kernel enforcement
mechanism?

>> Additionally, in the case where there is *not* a central management
>> daemon (i.e., what I'm implementing with libxdp), this would be the flow
>> implemented by the library without bpf_link:
>>
>> 1. Query kernel for current BPF prog loaded on $IFACE
>> 2. Sanity-check that this program is a dispatcher program installed by
>>    libxdp
>> 3. Create a new dispatcher program with whatever changes we want to do
>>    (such as adding another component program).
>> 4. Atomically replace the old program with the new one using the netlink
>>    API in this patch series.
>>
>> Whereas with bpf_link, it would be:
>>
>> 1. Find the pinned bpf_link for $IFACE (e.g., load from
>>    /sys/fs/bpf/iface-links/$IFNAME).
>
> But now you can hide this mount point from containerized
> root/CAP_NET_ADMIN application, can't you? See the difference? One
> might think about bpf_link as a fine-grained capability in this sense.

Yes, that may be a feature. But it may also be an anti-feature (I can't
move an iface to a new namespace that doesn't have the original bpffs
*without* preventing that namespace from replacing the XDP program).
Also, why are we re-inventing an ad-hoc capability mechanism?

-Toke

Powered by blists - more mailing lists