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

[John Fastabend <john.fastabend@gmail.com>]

Andrii Nakryiko wrote:
> On Mon, Mar 22, 2021 at 10:31 PM John Fastabend
> <john.fastabend@gmail.com> wrote:
> >
> > Rafael David Tinoco wrote:
> > > 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 <rafaeldtinoco@ubuntu.com>
> > > ---
> > >  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.


> >
> > 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

> 
> >
> > Sorry, missed v1,v2 so didn't reply sooner.
> >
> > > 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>