netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: "Toke Høiland-Jørgensen" <toke@redhat.com>,
	"Alexei Starovoitov" <alexei.starovoitov@gmail.com>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Martin KaFai Lau" <kafai@fb.com>,
	"Song Liu" <songliubraving@fb.com>, "Yonghong Song" <yhs@fb.com>,
	"Andrii Nakryiko" <andriin@fb.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Jesper Dangaard Brouer" <brouer@redhat.com>,
	"Lorenz Bauer" <lmb@cloudflare.com>,
	"Andrey Ignatov" <rdna@fb.com>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	"David Ahern" <dsahern@gmail.com>
Subject: Re: [PATCH bpf-next 1/4] xdp: Support specifying expected existing program when attaching XDP
Date: Tue, 31 Mar 2020 13:15:44 -0700	[thread overview]
Message-ID: <CAEf4BzYAtvETd+Sh6bBVnrqB=jz00+N1PLgsT6vAkwLhdB2d3w@mail.gmail.com> (raw)
In-Reply-To: <86f95d7a-1659-a092-91a2-abe5d58ceda8@iogearbox.net>

On Tue, Mar 31, 2020 at 6:49 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 3/31/20 12:13 PM, Toke Høiland-Jørgensen wrote:
> > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> >
> >>>> So you install your libxdp-based firewalls and are happy. Then you
> >>>> decide to install this awesome packet analyzer, which doesn't know
> >>>> about libxdp yet. Suddenly, you get all packets analyzer, but no more
> >>>> firewall, until users somehow notices that it's gone. Or firewall
> >>>> periodically checks that it's still runinng. Both not great, IMO, but
> >>>> might be acceptable for some users, I guess. But imagine all the
> >>>> confusion for user, especially if he doesn't give a damn about XDP and
> >>>> other buzzwords, but only needs a reliable firewall :)
> >>>
> >>> Yes, whereas if the firewall is using bpf_link, then the packet analyser
> >>> will be locked out and can't do its thing. Either way you end up with a
> >>> broken application; it's just moving the breakage. In the case of
> >>
> >> Hm... In one case firewall installation reported success and stopped
> >> working afterwards with no notification and user having no clue. In
> >> another, packet analyzer refused to start and reported error to user.
> >> Let's agree to disagree that those are not at all equivalent. To me
> >> silent failure is so much worse, than application failing to start in
> >> the first place.
>
> I sort of agree with both of you that either case is not great. The silent
> override we currently have is not great since it can be evicted at any time
> but also bpf_link to lock-out other programs at XDP layer is not great either
> since there is also huge potential to break existing programs. It's probably

I disagree with the premise that in current setup two XDP applications
can work at all. Best case, one will fail if specified the option to
not attach if something is already attached. Worst case, both will
happily assume (and report to user) that they are working, but only
the one attached last will actually work. Or maybe it's not even the
worst scenario, if both of them use netlink notification and keep
re-attaching themselves and detaching "opponent" (a fun bot fight to
watch...)

So what I hope we are discussing here is the world where some
applications are moving into using libxdp or some other co-operative
approach/daemon. In that case, utmost importance (otherwise its
unreliable and half-working solution) is to prevent XDP applications
not aware about this cooperation approach to break cooperating ones.
And that can be done only if libxdp/daemon can guarantee that *if it
successfully* attaches root XDP program, it won't be replaced by
oblivious XDP application that is not aware of it. So yes, in this
case non-cooperating application won't work (as it might not with
current API), but at least it will report that it can't attach, as
opposed to break *all* other nicely behaving and cooperating XDP
appplications **silently**. There are probably few more frustrating
things than silent corruption/breakage, IMO as both a user and
programmer.

> best to discuss on an actual proposal to see the concrete semantics, but my
> concerns, assuming I didn't misunderstand or got confused on something along
> the way (if so, please let me know), currently are:
>
>   - System service XYZ starts to use XDP with bpf_link one day. Somehow this
>     application gets shipped by default on mainstream distros and starts up
>     during init, then effectively locking out everyone else that used to use
>     the hook today "just fine" given they owned / orchestrated the underlying
>     networking on the host namespace for the nodes they manage (and for that

If XYZ didn't use bpf_link and just used existing API, it would break
everything as well, because see above, they can't co-exist. The
difference is in amount of undeterminism (who starts first and who's
second) and awareness (whether app even knows that it's broken).
Neither are in favor of existing API.

>     it worked before). Now such latter app somehow needs to work around this
>     breakage by undoing the damage that XYZ did in order to be able to operate
>     again. There was mentioned 'human override'. I presume whatever it will be,
>     it will also be done by applications when they don't have another choice.

No, it's opposite. That's why it's **human override**. It's not
intended to be used by application to "unblock" itself.

>     Otherwise we need to go and tell users that XDP is now only _entirely_
>     reserved for system service XYZ if you run distro ABC, but not for everyone
>     else anymore; what answer is there to this? From a PoV where one owns the

That's exactly what Toke's libxdp is intending to be. An XDP
coordination solution/library that other applications that want to
co-exist on the same network interface **have** to use. It intends to
allow all XDP applications to co-exist. That would be the answer - go
use that library.

>     entire distro and ecosystem, this is fine, but where this is not the case
>     as in the rest of the world having to rely on mainstream distros, what is
>     the answer to users (and especially "those that don't give a damn about XDP,
>     but just want to get stuff to work" that used to work before, even if we
>     think silent override is broken)? If the answer is to just 'shrug' and tell
>     'sorry that's the new way it is right now', then apps will try to use whatever
>     'human override' there is, and we're back to square one. To provide a
>     concrete example: if Cilium was configured to load some of its programs on
>     XDP hook, it currently replaces whatever it was there before. The assumption
>     is, that in the scenario we're in, we can orchestrate the hostns networking
>     just fine on K8s nodes since there is just one CNI plugin taking care of that
>     (and that generally also holds true for the other hooks we're using today).
>     Now, while we could switch to bpf_link as well and implement it in iproute2
>     for this specific case, what if someone else starting up earlier and locks
>     our stuff out? How would we work around it?

For one, you can use some tool/script (like the one I posted
yesterday: [0]) to find "offending" application that's not expected to
be using XDP and kill it (and/or investigate why on earth it got
installed/started in your infrastructure without you being aware). I
think this solution is better than nuke ("human override") option,
because it gives you clues on what's misbehaving and needs fixing in
the first place.

  [0] https://gist.github.com/anakryiko/562dff8e39c619a5ee247bb55aa057c7

>
>   - Assuming we have XDP with bpf_link in place with the above, now applications
>     are forced to start using bpf_link in order to not be locked out by others
>     using bpf_link as otherwise their application would break. So they need to
>     support the "old" way of attaching programs as we have today for older
>     kernels and need to support the bpf_link attachment for newer kernels since
>     they cannot rely on the old / existing API anymore. There is also a world
>     outside of C/C++ and thus libbpf / lib{xdp,dispatcher} or whatever, so the
>     whole rest of the ecosystem is forced to implement it as well due to breakage
>     concerns, understandable, but quite a burden.

Multi-XDP (on the same netdev, of course) doesn't exist and is not
possible today with existing API and semantics. The world outside of
C/C++ will either need to use compatible mechanisms (linking and using
libxdp with whatever means their language/runtime provides or at least
re-implementing the same set of protocols and behaviors).

>
>   - Equally, in case of Toke's implementation for the cmpxchg-like mechanism in
>     XDP itself, what happens if an application uses this API and assuming the
>     library would return the error to the application using it if the expected
>     program is not attached? Then the application would go for a forceful override
>     with the existing API today or would it voluntarily bail out and refusing to
>     work if some other non-cooperating application was loaded in the meantime?
>     What is the cmpxchg-like mechanism then solving realistically? (And again,
>     please keep all in mind we cannot force the entire world to use one single
>     library to rule 'em all, there are plenty of other language runtimes out in
>     the wild that cannot just import C/C++.)

It's not about using one specific library, but it is about following
the same protocol. I think that's what Toke, Alexei, Andrey and others
are assuming - that yes, if people want to write reliable XDP
applications co-existing with each other, they will have to use the
same library (easier, if possible) or at least follow the same
protocol.


>
> Thoughts?
>
> Thanks,
> Daniel

  parent reply	other threads:[~2020-03-31 20:15 UTC|newest]

Thread overview: 120+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-19 13:13 [PATCH bpf-next 0/4] XDP: Support atomic replacement of XDP interface attachments Toke Høiland-Jørgensen
2020-03-19 13:13 ` [PATCH bpf-next 1/4] xdp: Support specifying expected existing program when attaching XDP Toke Høiland-Jørgensen
2020-03-19 22:52   ` Jakub Kicinski
2020-03-20  8:48     ` Toke Høiland-Jørgensen
2020-03-20 17:35       ` Jakub Kicinski
2020-03-20 18:17         ` Toke Høiland-Jørgensen
2020-03-20 18:35           ` Jakub Kicinski
2020-03-20 18:30         ` John Fastabend
2020-03-20 20:24           ` Andrii Nakryiko
2020-03-23 11:24             ` Toke Høiland-Jørgensen
2020-03-23 16:54               ` Jakub Kicinski
2020-03-23 18:14               ` Andrii Nakryiko
2020-03-23 19:23                 ` Toke Høiland-Jørgensen
2020-03-24  1:01                   ` David Ahern
2020-03-24  4:53                     ` Andrii Nakryiko
2020-03-24 20:55                       ` David Ahern
2020-03-24 22:56                         ` Andrii Nakryiko
2020-03-24  5:00                   ` Andrii Nakryiko
2020-03-24 10:57                     ` Toke Høiland-Jørgensen
2020-03-24 18:53                       ` Jakub Kicinski
2020-03-24 22:30                         ` Andrii Nakryiko
2020-03-25  1:25                           ` Jakub Kicinski
2020-03-24 19:22                       ` John Fastabend
2020-03-25  1:36                         ` Alexei Starovoitov
2020-03-25  2:15                           ` Jakub Kicinski
2020-03-25 18:06                             ` Alexei Starovoitov
2020-03-25 18:20                               ` Jakub Kicinski
2020-03-25 19:14                                 ` Alexei Starovoitov
2020-03-25 10:42                           ` Toke Høiland-Jørgensen
2020-03-25 18:11                             ` Alexei Starovoitov
2020-03-25 10:30                         ` Toke Høiland-Jørgensen
2020-03-25 17:56                           ` Alexei Starovoitov
2020-03-24 22:25                       ` Andrii Nakryiko
2020-03-25  9:38                         ` Toke Høiland-Jørgensen
2020-03-25 17:55                           ` Alexei Starovoitov
2020-03-26  0:16                           ` Andrii Nakryiko
2020-03-26  5:13                             ` Jakub Kicinski
2020-03-26 18:09                               ` Andrii Nakryiko
2020-03-26 19:40                               ` Alexei Starovoitov
2020-03-26 20:05                                 ` Edward Cree
2020-03-27 11:09                                   ` Lorenz Bauer
2020-03-27 23:11                                   ` Alexei Starovoitov
2020-03-26 10:04                             ` Lorenz Bauer
2020-03-26 17:47                               ` Jakub Kicinski
2020-03-26 19:45                                 ` Alexei Starovoitov
2020-03-26 18:18                               ` Andrii Nakryiko
2020-03-26 19:53                               ` Alexei Starovoitov
2020-03-27 11:11                                 ` Toke Høiland-Jørgensen
2020-04-02 20:21                                   ` bpf: ability to attach freplace to multiple parents Alexei Starovoitov
2020-04-02 21:23                                     ` Toke Høiland-Jørgensen
2020-04-02 21:54                                       ` Alexei Starovoitov
2020-04-03  8:38                                         ` Toke Høiland-Jørgensen
2020-04-07  1:44                                           ` Alexei Starovoitov
2020-04-07  9:20                                             ` Toke Høiland-Jørgensen
2020-05-12  8:34                                         ` Toke Høiland-Jørgensen
2020-05-12  9:53                                           ` Alan Maguire
2020-05-12 13:02                                             ` Toke Høiland-Jørgensen
2020-05-12 23:18                                             ` Alexei Starovoitov
2020-05-12 23:06                                           ` Alexei Starovoitov
2020-05-13 10:25                                             ` Toke Høiland-Jørgensen
2020-04-02 21:24                                     ` Andrey Ignatov
2020-04-02 22:01                                       ` Alexei Starovoitov
2020-03-26 12:35                             ` [PATCH bpf-next 1/4] xdp: Support specifying expected existing program when attaching XDP Toke Høiland-Jørgensen
2020-03-26 19:06                               ` Andrii Nakryiko
2020-03-27 11:06                                 ` Lorenz Bauer
2020-03-27 16:12                                   ` David Ahern
2020-03-27 20:10                                     ` Andrii Nakryiko
2020-03-27 23:02                                     ` Alexei Starovoitov
2020-03-30 15:25                                       ` Edward Cree
2020-03-31  3:43                                         ` Alexei Starovoitov
2020-03-31 22:05                                           ` Edward Cree
2020-03-31 22:16                                             ` Alexei Starovoitov
2020-03-27 19:42                                   ` Andrii Nakryiko
2020-03-27 19:45                                   ` Andrii Nakryiko
2020-03-27 23:09                                   ` Alexei Starovoitov
2020-03-27 11:46                                 ` Toke Høiland-Jørgensen
2020-03-27 20:07                                   ` Andrii Nakryiko
2020-03-27 22:16                                     ` Toke Høiland-Jørgensen
2020-03-27 22:54                                       ` Andrii Nakryiko
2020-03-28  1:09                                         ` Toke Høiland-Jørgensen
2020-03-28  1:44                                           ` Andrii Nakryiko
2020-03-28 19:43                                             ` Toke Høiland-Jørgensen
2020-03-26 19:58                               ` Alexei Starovoitov
2020-03-27 12:06                                 ` Toke Høiland-Jørgensen
2020-03-27 23:00                                   ` Alexei Starovoitov
2020-03-28  1:43                                     ` Toke Høiland-Jørgensen
2020-03-28  2:26                                       ` Alexei Starovoitov
2020-03-28 19:34                                         ` Toke Høiland-Jørgensen
2020-03-28 23:35                                           ` Alexei Starovoitov
2020-03-29 10:39                                             ` Toke Høiland-Jørgensen
2020-03-29 19:26                                               ` Alexei Starovoitov
2020-03-30 10:19                                                 ` Toke Høiland-Jørgensen
2020-03-29 20:23                                           ` Andrii Nakryiko
2020-03-30 13:53                                             ` Toke Høiland-Jørgensen
2020-03-30 20:17                                               ` Andrii Nakryiko
2020-03-31 10:13                                                 ` Toke Høiland-Jørgensen
2020-03-31 13:48                                                   ` Daniel Borkmann
2020-03-31 15:00                                                     ` Toke Høiland-Jørgensen
2020-03-31 20:19                                                       ` Andrii Nakryiko
2020-03-31 20:15                                                     ` Andrii Nakryiko [this message]
2020-03-30 15:41                                             ` Edward Cree
2020-03-30 19:13                                               ` Jakub Kicinski
2020-03-31  4:01                                               ` Alexei Starovoitov
2020-03-31 11:34                                                 ` Toke Høiland-Jørgensen
2020-03-31 18:52                                                   ` Alexei Starovoitov
2020-03-20 20:30       ` Daniel Borkmann
2020-03-20 20:40         ` Daniel Borkmann
2020-03-20 21:30           ` Jakub Kicinski
2020-03-20 21:55             ` Daniel Borkmann
2020-03-20 23:35               ` Jakub Kicinski
2020-03-20 20:39       ` Andrii Nakryiko
2020-03-23 11:25         ` Toke Høiland-Jørgensen
2020-03-23 18:07           ` Andrii Nakryiko
2020-03-23 23:54           ` Andrey Ignatov
2020-03-24 10:16             ` Toke Høiland-Jørgensen
2020-03-20  2:13   ` Yonghong Song
2020-03-20  8:48     ` Toke Høiland-Jørgensen
2020-03-19 13:13 ` [PATCH bpf-next 2/4] tools: Add EXPECTED_FD-related definitions in if_link.h Toke Høiland-Jørgensen
2020-03-19 13:13 ` [PATCH bpf-next 3/4] libbpf: Add function to set link XDP fd while specifying old fd Toke Høiland-Jørgensen
2020-03-19 13:13 ` [PATCH bpf-next 4/4] selftests/bpf: Add tests for attaching XDP programs Toke Høiland-Jørgensen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEf4BzYAtvETd+Sh6bBVnrqB=jz00+N1PLgsT6vAkwLhdB2d3w@mail.gmail.com' \
    --to=andrii.nakryiko@gmail.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=andriin@fb.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=dsahern@gmail.com \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=kuba@kernel.org \
    --cc=lmb@cloudflare.com \
    --cc=netdev@vger.kernel.org \
    --cc=rdna@fb.com \
    --cc=songliubraving@fb.com \
    --cc=toke@redhat.com \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).