Andrii Nakryiko wrote:
> On Mon, Mar 22, 2021 at 10:31 PM John Fastabend
> <john.fastabend@xxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxx>
