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

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

[Rafael David Tinoco]

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(+)

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;
+}
+
 int bpf_object__set_priv(struct bpf_object *obj, void *priv,
 			 bpf_object_clear_priv_t clear_priv)
 {
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index a1a424b9b8ff..cf9bc6f1f925 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -143,6 +143,7 @@ LIBBPF_API int bpf_object__unload(struct bpf_object *obj);
 
 LIBBPF_API const char *bpf_object__name(const struct bpf_object *obj);
 LIBBPF_API unsigned int bpf_object__kversion(const struct bpf_object *obj);
+LIBBPF_API int bpf_object__set_kversion(struct bpf_object *obj, __u32 kern_version);
 
 struct btf;
 LIBBPF_API struct btf *bpf_object__btf(const struct bpf_object *obj);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 279ae861f568..f5990f7208ce 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -359,4 +359,5 @@ LIBBPF_0.4.0 {
 		bpf_linker__finalize;
 		bpf_linker__free;
 		bpf_linker__new;
+		bpf_object__set_kversion;
 } LIBBPF_0.3.0;
-- 
2.27.0

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

[John Fastabend]

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.

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

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.

>  int bpf_object__set_priv(struct bpf_object *obj, void *priv,
>  			 bpf_object_clear_priv_t clear_priv)
>  {
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index a1a424b9b8ff..cf9bc6f1f925 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -143,6 +143,7 @@ LIBBPF_API int bpf_object__unload(struct bpf_object *obj);
>  
>  LIBBPF_API const char *bpf_object__name(const struct bpf_object *obj);
>  LIBBPF_API unsigned int bpf_object__kversion(const struct bpf_object *obj);
> +LIBBPF_API int bpf_object__set_kversion(struct bpf_object *obj, __u32 kern_version);
>  
>  struct btf;
>  LIBBPF_API struct btf *bpf_object__btf(const struct bpf_object *obj);
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 279ae861f568..f5990f7208ce 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -359,4 +359,5 @@ LIBBPF_0.4.0 {
>  		bpf_linker__finalize;
>  		bpf_linker__free;
>  		bpf_linker__new;
> +		bpf_object__set_kversion;
>  } LIBBPF_0.3.0;
> -- 
> 2.27.0
> 

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

[Rafael David Tinoco]

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

Hi John,

This is how I'm using:
https://github.com/rafaeldtinoco/portablebpf/blob/master/mine.c#L285

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

having a xxx_bpf object:

xxx_bpf__open_and_load() -> xxx_bpf__load() -> 
bpf_object__load_skeleton() -> bpf_object_load() -> bpf_object__loadxattr()

We would have to add kern_version to 'bpf_object_skeleton' struct, to be 
passed to 'bpf_object__load_skeleton()' to have it passed on.

I'll let Andrii to see what he prefers.

Note:

Reason for all this (including the legacy kprobe logic, in other commit) 
is to continue the 
https://github.com/rafaeldtinoco/conntracker/tree/poc-cmd project, 
making sure it supports 4.x kernels. Still adding bpf support to it 
(identify task/pid per flow) and will replace the libnf* usage with bpf 
after that.

Thanks
-rafaeldtinoco

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

[Andrii Nakryiko]

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.

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

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

>
> >  int bpf_object__set_priv(struct bpf_object *obj, void *priv,
> >                        bpf_object_clear_priv_t clear_priv)
> >  {
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index a1a424b9b8ff..cf9bc6f1f925 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -143,6 +143,7 @@ LIBBPF_API int bpf_object__unload(struct bpf_object *obj);
> >
> >  LIBBPF_API const char *bpf_object__name(const struct bpf_object *obj);
> >  LIBBPF_API unsigned int bpf_object__kversion(const struct bpf_object *obj);
> > +LIBBPF_API int bpf_object__set_kversion(struct bpf_object *obj, __u32 kern_version);
> >
> >  struct btf;
> >  LIBBPF_API struct btf *bpf_object__btf(const struct bpf_object *obj);
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 279ae861f568..f5990f7208ce 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -359,4 +359,5 @@ LIBBPF_0.4.0 {
> >               bpf_linker__finalize;
> >               bpf_linker__free;
> >               bpf_linker__new;
> > +             bpf_object__set_kversion;
> >  } LIBBPF_0.3.0;
> > --
> > 2.27.0
> >

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

[Andrii Nakryiko]

On Thu, Mar 25, 2021 at 1:01 PM Rafael David Tinoco
<rafaeldtinoco@ubuntu.com> 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.
>
> Hi John,
>
> This is how I'm using:
> https://github.com/rafaeldtinoco/portablebpf/blob/master/mine.c#L285
>
> > Although, I don't use the skeleton pieces so maybe it would be
> > awkward for that usage.
>
> having a xxx_bpf object:
>
> xxx_bpf__open_and_load() -> xxx_bpf__load() ->
> bpf_object__load_skeleton() -> bpf_object_load() -> bpf_object__loadxattr()
>
> We would have to add kern_version to 'bpf_object_skeleton' struct, to be
> passed to 'bpf_object__load_skeleton()' to have it passed on.
>
> I'll let Andrii to see what he prefers.

See my reply to John. What he asked for already exists. Having a test
would be nice, but selftests/bpf don't have an infrastructure to even
perform this test, so I don't think it's worth it.

So in summary, LGTM. I'll apply it once bpf-next is ready to accept new patches.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>
> Note:
>
> Reason for all this (including the legacy kprobe logic, in other commit)
> is to continue the
> https://github.com/rafaeldtinoco/conntracker/tree/poc-cmd project,
> making sure it supports 4.x kernels. Still adding bpf support to it
> (identify task/pid per flow) and will replace the libnf* usage with bpf
> after that.
>
> Thanks
> -rafaeldtinoco

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

[John Fastabend]

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>

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

[Andrii Nakryiko]

On Thu, Mar 25, 2021 at 5:56 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
> 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.

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.

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