Re: [PATCH bpf-next 3/5] libbpf: add low level TC-BPF API

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: "Kumar Kartikeya Dwivedi" <memxor@gmail.com>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	bpf <bpf@vger.kernel.org>,
	"Jesper Dangaard Brouer" <brouer@redhat.com>,
	"Toke Høiland-Jørgensen" <toke@redhat.com>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Andrii Nakryiko" <andrii@kernel.org>,
	"Martin KaFai Lau" <kafai@fb.com>,
	"Song Liu" <songliubraving@fb.com>, "Yonghong Song" <yhs@fb.com>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"KP Singh" <kpsingh@kernel.org>, "Shuah Khan" <shuah@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Jesper Dangaard Brouer" <hawk@kernel.org>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"open list" <linux-kernel@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	"open list:KERNEL SELFTEST FRAMEWORK"
	<linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH bpf-next 3/5] libbpf: add low level TC-BPF API
Date: Mon, 5 Apr 2021 10:27:28 -0700	[thread overview]
Message-ID: <CAEf4Bzaeu4apgEtwS_3q1iPuURjPXMs9H43cYUtJSmjPMU5M9A@mail.gmail.com> (raw)
In-Reply-To: <20210403174721.vg4wle327wvossgl@ast-mbp>

On Sat, Apr 3, 2021 at 10:47 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sat, Apr 03, 2021 at 12:38:06AM +0530, Kumar Kartikeya Dwivedi wrote:
> > On Sat, Apr 03, 2021 at 12:02:14AM IST, Alexei Starovoitov wrote:
> > > On Fri, Apr 2, 2021 at 8:27 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > > > [...]
> > >
> > > All of these things are messy because of tc legacy. bpf tried to follow tc style
> > > with cls and act distinction and it didn't quite work. cls with
> > > direct-action is the only
> > > thing that became mainstream while tc style attach wasn't really addressed.
> > > There were several incidents where tc had tens of thousands of progs attached
> > > because of this attach/query/index weirdness described above.
> > > I think the only way to address this properly is to introduce bpf_link style of
> > > attaching to tc. Such bpf_link would support ingress/egress only.
> > > direction-action will be implied. There won't be any index and query
> > > will be obvious.
> >
> > Note that we already have bpf_link support working (without support for pinning
> > ofcourse) in a limited way. The ifindex, protocol, parent_id, priority, handle,
> > chain_index tuple uniquely identifies a filter, so we stash this in the bpf_link
> > and are able to operate on the exact filter during release.
>
> Except they're not unique. The library can stash them, but something else
> doing detach via iproute2 or their own netlink calls will detach the prog.
> This other app can attach to the same spot a different prog and now
> bpf_link__destroy will be detaching somebody else prog.
>
> > > So I would like to propose to take this patch set a step further from
> > > what Daniel said:
> > > int bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS}):
> > > and make this proposed api to return FD.
> > > To detach from tc ingress/egress just close(fd).
> >
> > You mean adding an fd-based TC API to the kernel?
>
> yes.

I'm totally for bpf_link-based TC attachment.

But I think *also* having "legacy" netlink-based APIs will allow
applications to handle older kernels in a much nicer way without extra
dependency on iproute2. We have a similar situation with kprobe, where
currently libbpf only supports "modern" fd-based attachment, but users
periodically ask questions and struggle to figure out issues on older
kernels that don't support new APIs.

So I think we'd have to support legacy TC APIs, but I agree with
Alexei and Daniel that we should keep it to the simplest and most
straightforward API of supporting direction-action attachments and
setting up qdisc transparently (if I'm getting all the terminology
right, after reading Quentin's blog post). That coincidentally should
probably match how bpf_link-based TC API will look like, so all that
can be abstracted behind a single bpf_link__attach_tc() API as well,
right? That's the plan for dealing with kprobe right now, btw. Libbpf
will detect the best available API and transparently fall back (maybe
with some warning for awareness, due to inherent downsides of legacy
APIs: no auto-cleanup being the most prominent one).

>
> > > The user processes will not conflict with each other and will not accidently
> > > detach bpf program that was attached by another user process.
> > > Such api will address the existing tc query/attach/detach race race conditions.
> >
> > Hmm, I think we do solve the race condition by returning the id. As long as you
> > don't misuse the interface and go around deleting filters arbitrarily (i.e. only
> > detach using the id), programs won't step over each other's filters. Storing the
> > id from the netlink response received during detach also eliminates any
> > ambigiuity from probing through get_info after attach. Same goes for actions,
> > and the same applies to the bpf_link returning API (which stashes id/index).
>
> There are plenty of tools and libraries out there that do attach/detach of bpf
> to tc. Everyone is not going to convert to this new libbpf api overnight.
> So 'miuse of the interface' is not a misuse. It's a reality that is going to keep
> happening unless the kernel guarantees ownership of the attachment via FD.
>
> > The only advantage of fd would be the possibility of pinning it, and extending
> > lifetime of the filter.
>
> Pinning is one of the advantages. The main selling point of FD is ownership
> of the attachment.