Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> 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
