Message-Id: <6d76aed1-5bc8-42e1-9d50-277d2d66d57f@app.fastmail.com>
Date: Sat, 10 Jun 2023 08:22:39 +0530
From: "Daniel Xu" <dxu@...uu.xyz>
To: "Andrii Nakryiko" <andrii.nakryiko@...il.com>,
 "Stanislav Fomichev" <sdf@...gle.com>
Cc: "Daniel Borkmann" <daniel@...earbox.net>,
 Toke Høiland-Jørgensen <toke@...nel.org>,
 "Timo Beckers" <timo@...line.eu>, "Alexei Starovoitov" <ast@...nel.org>,
 "Andrii Nakryiko" <andrii@...nel.org>, martin.lau@...ux.dev,
 razor@...ckwall.org, john.fastabend@...il.com,
 "Jakub Kicinski" <kuba@...nel.org>, joe@...ium.io, davem@...emloft.net,
 "bpf@...r.kernel.org" <bpf@...r.kernel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 1/7] bpf: Add generic attach/detach/query API for
 multi-progs

Hi all,

On Sat, Jun 10, 2023, at 12:33 AM, Andrii Nakryiko wrote:
> On Fri, Jun 9, 2023 at 9:41 AM Stanislav Fomichev <sdf@...gle.com> wrote:
>>
>> On Fri, Jun 9, 2023 at 7:15 AM Daniel Borkmann <daniel@...earbox.net> wrote:
>> >
>> > On 6/9/23 3:11 PM, Toke Høiland-Jørgensen wrote:
>> > > Timo Beckers <timo@...line.eu> writes:
>> > >> On 6/9/23 13:04, Toke Høiland-Jørgensen wrote:
>> > >>> Daniel Borkmann <daniel@...earbox.net> writes:
>> > [...]
>> > >>>>>>>>>> I'm still not sure whether the hard semantics of first/last is really
>> > >>>>>>>>>> useful. My worry is that some prog will just use BPF_F_FIRST which
>> > >>>>>>>>>> would prevent the rest of the users.. (starting with only
>> > >>>>>>>>>> F_BEFORE/F_AFTER feels 'safer'; we can iterate later on if we really
>> > >>>>>>>>>> need first/laste).
>> > >>>>>>>>> Without FIRST/LAST some scenarios cannot be guaranteed to be safely
>> > >>>>>>>>> implemented. E.g., if I have some hard audit requirements and I need
>> > >>>>>>>>> to guarantee that my program runs first and observes each event, I'll
>> > >>>>>>>>> enforce BPF_F_FIRST when attaching it. And if that attachment fails,
>> > >>>>>>>>> then server setup is broken and my application cannot function.
>> > >>>>>>>>>
>> > >>>>>>>>> In a setup where we expect multiple applications to co-exist, it
>> > >>>>>>>>> should be a rule that no one is using FIRST/LAST (unless it's
>> > >>>>>>>>> absolutely required). And if someone doesn't comply, then that's a bug
>> > >>>>>>>>> and has to be reported to application owners.
>> > >>>>>>>>>
>> > >>>>>>>>> But it's not up to the kernel to enforce this cooperation by
>> > >>>>>>>>> disallowing FIRST/LAST semantics, because that semantics is critical
>> > >>>>>>>>> for some applications, IMO.
>> > >>>>>>>> Maybe that's something that should be done by some other mechanism?
>> > >>>>>>>> (and as a follow up, if needed) Something akin to what Toke
>> > >>>>>>>> mentioned with another program doing sorting or similar.
>> > >>>>>>> The goal of this API is to avoid needing some extra special program to
>> > >>>>>>> do this sorting
>> > >>>>>>>
>> > >>>>>>>> Otherwise, those first/last are just plain simple old priority bands;
>> > >>>>>>>> only we have two now, not u16.
>> > >>>>>>> I think it's different. FIRST/LAST has to be used judiciously, of
>> > >>>>>>> course, but when they are needed, they will have no alternative.
>> > >>>>>>>
>> > >>>>>>> Also, specifying FIRST + LAST is the way to say "I want my program to
>> > >>>>>>> be the only one attached". Should we encourage such use cases? No, of
>> > >>>>>>> course. But I think it's fair  for users to be able to express this.
>> > >>>>>>>
>> > >>>>>>>> I'm mostly coming from the observability point: imagine I have my fancy
>> > >>>>>>>> tc_ingress_tcpdump program that I want to attach as a first program to debug
>> > >>>>>>>> some issue, but it won't work because there is already a 'first' program
>> > >>>>>>>> installed.. Or the assumption that I'd do F_REPLACE | F_FIRST ?
>> > >>>>>>> If your production setup requires that some important program has to
>> > >>>>>>> be FIRST, then yeah, your "let me debug something" program shouldn't
>> > >>>>>>> interfere with it (assuming that FIRST requirement is a real
>> > >>>>>>> requirement and not someone just thinking they need to be first; but
>> > >>>>>>> that's up to user space to decide). Maybe the solution for you in that
>> > >>>>>>> case would be freplace program installed on top of that stubborn FIRST
>> > >>>>>>> program? And if we are talking about local debugging and development,
>> > >>>>>>> then you are a sysadmin and you should be able to force-detach that
>> > >>>>>>> program that is getting in the way.
>> > >>>>>> I'm not really concerned about our production environment. It's pretty
>> > >>>>>> controlled and restricted and I'm pretty certain we can avoid doing
>> > >>>>>> something stupid. Probably the same for your env.
>> > >>>>>>
>> > >>>>>> I'm mostly fantasizing about upstream world where different users don't
>> > >>>>>> know about each other and start doing stupid things like F_FIRST where
>> > >>>>>> they don't really have to be first. It's that "used judiciously" part
>> > >>>>>> that I'm a bit skeptical about :-D
>> > >>>> But in the end how is that different from just attaching themselves blindly
>> > >>>> into the first position (e.g. with before and relative_fd as 0 or the fd/id
>> > >>>> of the current first program) - same, they don't really have to be first.
>> > >>>> How would that not result in doing something stupid? ;) To add to Andrii's
>> > >>>> earlier DDoS mitigation example ... think of K8s environment: one project
>> > >>>> is implementing DDoS mitigation with BPF, another one wants to monitor/
>> > >>>> sample traffic to user space with BPF. Both install as first position by
>> > >>>> default (before + 0). In K8s, there is no built-in Pod dependency management
>> > >>>> so you cannot guarantee whether Pod A comes up before Pod B. So you'll end
>> > >>>> up in a situation where sometimes the monitor runs before the DDoS mitigation
>> > >>>> and on some other nodes it's vice versa. The other case where this gets
>> > >>>> broken (assuming a node where we get first the DDoS mitigation, then the
>> > >>>> monitoring) is when you need to upgrade one of the Pods: monitoring Pod
>> > >>>> gets a new stable update and is being re-rolled out, then it inserts
>> > >>>> itself before the DDoS mitigation mechanism, potentially causing outage.
>> > >>>> With the first/last mechanism these two situations cannot happen. The DDoS
>> > >>>> mitigation software uses first and the monitoring uses before + 0, then no
>> > >>>> matter the re-rollouts or the ordering in which Pods come up, it's always
>> > >>>> at the expected/correct location.
>> > >>> I'm not disputing that these kinds of policy issues need to be solved
>> > >>> somehow. But adding the first/last pinning as part of the kernel hooks
>> > >>> doesn't solve the policy problem, it just hard-codes a solution for one
>> > >>> particular instance of the problem.
>> > >>>
>> > >>> Taking your example from above, what happens when someone wants to
>> > >>> deploy those tools in reverse order? Say the monitoring tool counts
>> > >>> packets and someone wants to also count the DDOS traffic; but the DDOS
>> > >>> protection tool has decided for itself (by setting the FIRST) flag that
>> > >>> it can *only* run as the first program, so there is no way to achieve
>> > >>> this without modifying the application itself.
>> > >>>
>> > >>>>>> Because even with this new ordering scheme, there still should be
>> > >>>>>> some entity to do relative ordering (systemd-style, maybe CNI?).
>> > >>>>>> And if it does the ordering, I don't really see why we need
>> > >>>>>> F_FIRST/F_LAST.
>> > >>>>> I can see I'm a bit late to the party, but FWIW I agree with this:
>> > >>>>> FIRST/LAST will definitely be abused if we add it. It also seems to me
>> > >> It's in the prisoners' best interest to collaborate (and they do! see
>> > >> https://www.youtube.com/watch?v=YK7GyEJdJGo), except the current
>> > >> prio system is limiting and turns out to be really fragile in practice.
>> > >>
>> > >> If your tool wants to attach to tc prio 1 and there's already a prog
>> > >> attached,
>> > >> the most reliable option is basically to blindly replace the attachment,
>> > >> unless
>> > >> you have the possibility to inspect the attached prog and try to figure
>> > >> out if it
>> > >> belongs to another tool. This is fragile in and of itself, and only
>> > >> possible on
>> > >> more recent kernels iirc.
>> > >>
>> > >> With tcx, Cilium could make an initial attachment using F_FIRST and simply
>> > >> update a link at well-known path on subsequent startups. If there's no
>> > >> existing
>> > >> link, and F_FIRST is taken, bail out with an error. The owner of the
>> > >> existing
>> > >> F_FIRST program can be queried and logged; we know for sure the program
>> > >> doesn't belong to Cilium, and we have no interest in detaching it.
>> > >
>> > > That's conflating the benefit of F_FIRST with that of bpf_link, though;
>> > > you can have the replace thing without the exclusive locking.
>> > >
>> > >>>> See above on the issues w/o the first/last. How would you work around them
>> > >>>> in practice so they cannot happen?
>> > >>> By having an ordering configuration that is deterministic. Enforced by
>> > >>> the system-wide management daemon by whichever mechanism suits it. We
>> > >>> could implement a minimal reference policy agent that just reads a
>> > >>> config file in /etc somewhere, and *that* could implement FIRST/LAST
>> > >>> semantics.
>> > >> I think this particular perspective is what's deadlocking this discussion.
>> > >> To me, it looks like distros and hyperscalers are in the same boat with
>> > >> regards to the possibility of coordination between tools. Distros are only
>> > >> responsible for the tools they package themselves, and hyperscalers
>> > >> run a tight ship with mostly in-house tooling already. When it comes to
>> > >> projects out in the wild, that all goes out the window.
>> > >
>> > > Not really: from the distro PoV we absolutely care about arbitrary
>> > > combinations of programs with different authors. Which is why I'm
>> > > arguing against putting anything into the kernel where the first program
>> > > to come along can just grab a hook and lock everyone out.
>> > >
>> > > My assumption is basically this: A system administrator installs
>> > > packages A and B that both use the TC hook. The developers of A and B
>> > > have never heard about each other. It should be possible for that admin
>> > > to run A and B in whichever order they like, without making any changes
>> > > to A and B themselves.
>> >
>> > I would come with the point of view of the K8s cluster operator or platform
>> > engineer, if you will. Someone deeply familiar with K8s, but not necessarily
>> > knowing about kernel internals. I know my org needs to run container A and
>> > container B, so I'll deploy the daemon-sets for both and they get deployed
>> > into my cluster. That platform engineer might have never heard of BPF or might
>> > not even know that container A or container B ships software with BPF. As
>> > mentioned, K8s itself has no concept of Pod ordering as its paradigm is that
>> > everything is loosely coupled. We are now expecting from that person to make
>> > a concrete decision about some BPF kernel internals on various hooks in which
>> > order they should be executed given if they don't then the system becomes
>> > non-deterministic. I think that is quite a big burden and ask to understand.
>> > Eventually that person will say that he/she cannot make this technical decision
>> > and that only one of the two containers can be deployed. I agree with you that
>> > there should be an option for a technically versed person to be able to change
>> > ordering to avoid lock out, but I don't think it will fly asking users to come
>> > up on their own with policies of BPF software in the wild ... similar as you
>> > probably don't want having to deal with writing systemd unit files for software
>> > xyz before you can use your laptop. It's a burden. You expect this to magically
>> > work by default and only if needed for good reasons to make custom changes.
>> > Just the one difference is that the latter ships with the OS (a priori known /
>> > tight-ship analogy).
>> >
>> > >> Regardless of merit or feasability of a system-wide bpf management
>> > >> daemon for k8s, there _is no ordering configuration possible_. K8s is not
>> > >> a distro where package maintainers (or anyone else, really) can coordinate
>> > >> on correctly defining priority of each of the tools they ship. This is
>> > >> effectively
>> > >> the prisoner's dilemma. I feel like most of the discussion so far has been
>> > >> very hand-wavy in 'user space should solve it'. Well, we are user space, and
>> > >> we're here trying to solve it. :)
>> > >>
>> > >> A hypothetical policy/gatekeeper/ordering daemon doesn't possess
>> > >> implicit knowledge about which program needs to go where in the chain,
>> > >> nor is there an obvious heuristic about how to order things. Maintaining
>> > >> such a configuration for all cloud-native tooling out there that possibly
>> > >> uses bpf is simply impossible, as even a tool like Cilium can change
>> > >> dramatically from one release to the next. Having to manage this too
>> > >> would put a significant burden on velocity and flexibility for arguably
>> > >> little benefit to the user.
>> > >>
>> > >> So, daemon/kernel will need to be told how to order things, preferably by
>> > >> the tools (Cilium/datadog-agent) themselves, since the user/admin of the
>> > >> system cannot be expected to know where to position the hundreds of progs
>> > >> loaded by Cilium and how they might interfere with other tools. Figuring
>> > >> this out is the job of the tool, daemon or not.
>> > >>
>> > >> The prisoners _must_ communicate (so, not abuse F_FIRST) for things to
>> > >> work correctly, and it's 100% in their best interest in doing so. Let's not
>> > >> pretend like we're able to solve game theory on this mailing list. :)
>> > >> We'll have to settle for the next-best thing: give user space a safe and
>> > >> clear
>> > >> API to allow it to coordinate and make the right decisions.
>> > >
>> > > But "always first" is not a meaningful concept. It's just what we have
>> > > today (everyone picks priority 1), except now if there are two programs
>> > > that want the same hook, it will be the first program that wins the
>> > > contest (by locking the second one out), instead of the second program
>> > > winning (by overriding the first one) as is the case with the silent
>> > > override semantics we have with TC today. So we haven't solved the
>> > > problem, we've just shifted the breakage.
>> >
>> > Fwiw, it's deterministic, and I think this 1000x better than silently
>> > having a non-deterministic deployment where the two programs ship with
>> > before + 0. That is much harder to debug.
>> >
>> > >> To circle back to the observability case: in offline discussions with
>> > >> Daniel,
>> > >> I've mentioned the need for 'shadow' progs that only collect data and
>> > >> pump it to user space, attached at specific points in the chain (still
>> > >> within tcx!).
>> > >> Their retcodes would be ignored, and context modifications would be
>> > >> rejected, so attaching multiple to the same hook can always succeed,
>> > >> much like cgroup multi. Consider the following:
>> > >>
>> > >> To attach a shadow prog before F_FIRST, a caller could use F_BEFORE |
>> > >> F_FIRST |
>> > >> F_RDONLY. Attaching between first and the 'relative' section: F_AFTER |
>> > >> F_FIRST |
>> > >> F_RDONLY, etc. The rdonly flag could even be made redundant if a new prog/
>> > >> attach type is added for progs like these.
>> > >>
>> > >> This is still perfectly possible to implement on top of Daniel's
>> > >> proposal, and
>> > >> to me looks like it could address many of the concerns around ordering of
>> > >> progs I've seen in this thread, many mention data exfiltration.
>> > >
>> > > It may well be that semantics like this will turn out to be enough. Or
>> > > it may not (I personally believe we'll need something more expressive
>> > > still, and where the system admin has the option to override things; but
>> > > I may turn out to be wrong). Ultimately, my main point wrt this series
>> > > is that this kind of policy decision can be added later, and it's better
>> > > to merge the TCX infrastructure without it, instead of locking ourselves
>> > > into an API that is way too limited today. TCX (and in-kernel XDP
>> > > multiprog) has value without it, so let's merge that first and iterate
>> > > on the policy aspects.
>> >
>> > That's okay and I'll do that for v3 to move on.
>> >
>> > I feel we might repeat the same discussion with no good solution for K8s
>> > users once we come back to this point again.
>>
>> With your cilium vs ddos example, maybe all we really need is for the
>> program to have some signal about whether it's ok to have somebody
>> modify/drop the packets before it?
>> For example, the verifier, depending on whether it sees that the
>> program writes to the data, uses some helpers, or returns
>> TC_ACT_SHOT/etc can classify the program as readonly or non-readonly.
>> And then, we'll have some extra flag during program load/attach that
>> cilium will pass to express "I'm not ok with having a non-readonly
>> program before me".
>
> So this is what Timo is proposing with F_READONLY. And I agree, that
> makes sense and we've discussed the need for something like this
> internally. Specific use case was setsockopt programs. Sometimes they
> should just observe, and we'd like to enforce that.
>
> Once we have this F_READONLY flag support and enforce that during BPF
> program validation, then "I'm not ok with having a non-readonly
> program before me" is exactly F_FIRST. We just say that the F_READONLY
> program can be inserted anywhere because it has no effect on the state
> of the system.

I have a different use case for something like F_READONLY. Basically I would
like to be able to accept precompiled BPF progs from semi-trusted sources
and run / attach the prog in a trusted context. Example could be telling the customer:
"give me a prog that you'd like to run against every packet that enters your network
and I will orchestrate / distribute it across your infrastructure". F_READONLY could be
used as one of the mechanisms to uphold invariants like not being able to bring
down the network.

Thanks,
Daniel

Powered by blists - more mailing lists