Message-ID: <87eetcl1e3.fsf@toke.dk>
Date:   Sat, 28 Mar 2020 20:34:12 +0100
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        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

Alexei Starovoitov <alexei.starovoitov@...il.com> writes:

> On Sat, Mar 28, 2020 at 02:43:18AM +0100, Toke Høiland-Jørgensen wrote:
>> 
>> No, I was certainly not planning to use that to teach libxdp to just
>> nuke any bpf_link it finds attached to an interface. Quite the contrary,
>> the point of this series is to allow libxdp to *avoid* replacing
>> something on the interface that it didn't put there itself.
>
> Exactly! "that it didn't put there itself".
> How are you going to do that?
> I really hope you thought it through and came up with magic.
> Because I tried and couldn't figure out how to do that with IFLA_XDP*
> Please walk me step by step how do you think it's possible.

I'm inspecting the BPF program itself to make sure it's compatible.
Specifically, I'm embedding a piece of metadata into the program BTF,
using Andrii's encoding trick that we also use for defining maps. So
xdp-dispatcher.c contains this[0]:

__uint(dispatcher_version, XDP_DISPATCHER_VERSION) SEC(XDP_METADATA_SECTION);

and libxdp will refuse to touch any program that it finds loaded on an
iface which doesn't have this, or which has a version number that is
higher than what the library understands. The code implementing the
check itself is this[1]:

static int check_dispatcher_version(struct btf *btf)
{
	const char *name = "dispatcher_version";
	const struct btf_type *sec, *def;
	__u32 version;

	sec = btf_get_datasec(btf, XDP_METADATA_SECTION);
	if (!sec)
		return -ENOENT;

	def = btf_get_section_var(btf, sec, name, BTF_KIND_PTR);
	if (IS_ERR(def))
		return PTR_ERR(def);

	if (!get_field_int(btf, name, def, &version))
		return -ENOENT;

	if (version > XDP_DISPATCHER_VERSION) {
		pr_warn("XDP dispatcher version %d higher than supported %d\n",
			version, XDP_DISPATCHER_VERSION);
		return -EOPNOTSUPP;
	}
	pr_debug("Verified XDP dispatcher version %d <= %d\n",
		 version, XDP_DISPATCHER_VERSION);
	return 0;
}

and is called both when loading the BPF object code from disk, and
before operating on a program already loaded into the kernel.

> I'm saying that without bpf_link for xdp libxdp has no ability to
> identify an attachment that is theirs.

Ah, so *that* was what you meant with "unique attachment". It never
occurred to me that answering this question ("is it my program?") was to
be a feature of bpf_link; I always assumed that would be a property of
the bpf_prog itself.

Any reason what I'm describing above wouldn't work for you?

> I suspect what is happening that you found first missing kernel feature
> while implementing libxdp and trying to fix it by extending kernel api.
> Well the reason libxdp is not part of libbpf is for it to be flexible
> in design and have unstable api.
> But you're using this unstable project as the reason to add stable apis
> both to kernel and libbpf. I don't think that's workable because...

That's certainly not my intention. I have done my best to think through
which is the minimum amount of kernel support I need to implement the
libxdp multi-prog feature set. When the initial freplace support landed
there was three things missing:

1. Ability to make freplace attachments permanent
2. Atomic replace of XDP programs
3. Multi-attach for freplace

Andrii already solved 1. with pinning, this is my attempt to solve 2.,
and 3. is TBD.

>> I could understand why you wouldn't want to do
>> that if it was a huge and invasive change; but it really isn't...
>
> Yes. It's a small api extension to both kernel and libbpf.
> But it means that by accepting this small change I sign up on maintaining it
> forever. And I see how second and third such small experimental change will be
> coming in the future. All such design revisions of libxdp will end up on my
> plate to support forever in the kernel and in libbpf. I'm not excited to
> support all of these experimental code.

I understand that, but as I said it's really not my intention to just
dump experimental code on you. And I also do consider this an obvious
API bugfix that is useful in its own right.

> I see two ways out of this stalemate:
> 1. assume that replace_fd extension landed and develop libxdp further
>    into fully fledged library. May be not a complete library, but at least
>    for few more weeks. If then you still think replace_fd is enough
>    I'll land it.
> 2. I can land replace_fd now, but please don't be surprised that
>    I will revert it several weeks from now when it's clear that
>    it's not enough.
>  
> Which one do you prefer?

I prefer 2. Reverting if it does turn out that I'm wrong is fine. Heck,
in that case I'll even send the revert myself :)

-Toke

[0] https://github.com/xdp-project/xdp-tools/blob/xdp-multi-prog/lib/libxdp/xdp-dispatcher.c.in#L61
[1] https://github.com/xdp-project/xdp-tools/blob/xdp-multi-prog/lib/libxdp/libxdp.c#L824

Powered by blists - more mailing lists