Re: [PATCH v3 bpf-next] libbpf: add bpf object kern_version attribute setter

On Thu, Mar 25, 2021 at 5:56 PM John Fastabend [off-list ref] wrote:

Andrii Nakryiko wrote:

quoted

On Mon, Mar 22, 2021 at 10:31 PM John Fastabend
[off-list ref] wrote:

quoted

Rafael David Tinoco wrote:

quoted

Unfortunately some distros don't have their kernel version defined
accurately in <linux/version.h> due to different long term support
reasons.

It is important to have a way to override the bpf kern_version
attribute during runtime: some old kernels might still check for
kern_version attribute during bpf_prog_load().

Signed-off-by: Rafael David Tinoco <redacted>
---
 tools/lib/bpf/libbpf.c   | 10 ++++++++++
 tools/lib/bpf/libbpf.h   |  1 +
 tools/lib/bpf/libbpf.map |  1 +
 3 files changed, 12 insertions(+)

Hi Andrii and Rafael,

Did you consider making kernel version an attribute of the load
API, bpf_prog_load_xattr()? This feels slightly more natural
to me, to tell the API the kernel you need at load time.

Um... kern_version is already part of bpf_load_program_attr, used by
bpf_load_program_xattr. What am I missing? But you can't use that with
bpf_object APIs.

Aha I mistyped. It looks like I have a patch floating around on my
stack to add it to bpf_object_load_attr.

Oh, you meant this one. I'm actually trying to move away from having
load() to take options at all. If you check BPF skeletons, their load
doesn't even accept options. Adding getters/setter is better in one
major way:
  - it's more flexible approach and allows to have both per-object
setters/options and per-program ones. bpf_object_load_attr provides
only per-object options, which are often inadequate (see recent
bpf_program__set_attach_target() and bpf_program__set_autoload(),
which are just impossible to sanely do with per-object options)
  - even though we now have the whole forward/backwards compatible
OPTS "framework" within libbpf, I think it's less pleasant to use than
setters. We have to do options on load, because we don't have any
object before open happens (if we had separate new() and open() that
wouldn't be the case), so there is a need to specify things before
bpf_object is instantiated. bpf_object__load() doesn't have this
problem, because we have entire bpf_object and bpf_map/bpf_program to
tweak before we perform load.
  - adding new APIs is inherently forward compatible. And backwards
compatibility is the same between OPTS and new API methods: you need
to make sure to use recent enough libbpf version that has options/API
you need.

So in short, I'm against adding load-time options, because there are
better and more flexible alternatives.

quoted

quoted

Although, I don't use the skeleton pieces so maybe it would be
awkward for that usage.

Yes, low-level APIs are separate. This is for cases where you have
struct bpf_program abstractions, which are loaded by
bpf_object__load(). We could set it at per-program level, but they
should be all the same, so bpf_object__set_kversion() makes more sense
and is more convenient to use. And there is already a getter for that,
so it complements that nicely.

+1

quoted

quoted

Sorry, missed v1,v2 so didn't reply sooner.

quoted

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 058b643cbcb1..3ac3d8dced7f 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -8269,6 +8269,16 @@ int bpf_object__btf_fd(const struct bpf_object *obj)
      return obj->btf ? btf__fd(obj->btf) : -1;
 }

+int bpf_object__set_kversion(struct bpf_object *obj, __u32 kern_version)
+{
+     if (obj->loaded)
+             return -EINVAL;
+
+     obj->kern_version = kern_version;
+
+     return 0;
+}
+

Having a test to read uname and feed it into libbpf using
above to be sure we don't break this in the future would be
nice.

kern_version has been ignored by kernel for a long time. So there is
no way to test this in selftests/bpf. We could use libbpf CI's old
kernel setup to validate, but I don't think it's worth it. It's
extremely unlikely this will ever change or break (and it's a legacy
stuff we move away from anyways, so it's born sort of obsolete).

+1

For the patch, thanks for the details Andrii, thanks for the patch
Rafael it will be useful here.

Acked-by: John Fastabend <john.fastabend@gmail.com>