[RFC PATCH 0/2] Add btf__field_exists

[RFC PATCH 0/2] Add btf__field_exists

[Nikolay Borisov]

Hello,

Here are 2 patches with which I want to probe what's the sentiments towards 2
changes:

1. Introduction of libbpf APIs similar to the bpf counterparts: bpf_core_field_exists,
bpf_core_type_exists and bpf_core_enum_value_exists. Of those I've implemented only
the first one and the reasoning behind this is in the patch itself. However, the
TLDR is that there can be cases where based on the kernel version we have to make a
decision in userspace what set of kprobes to use. There are currently no convenince
api's to do this so one has to essentially open code the checks that can be provided
by the aforementioned APIs.

2. The kernel has for_each_member macro but the libbpf library doesn't provide it,
this results in having to open code members enumeration in various places such as
in find_member_by_name/find_struct_ops_kern_types/bpf_map__init_kern_struct_ops/
parse_btf_map_def  and in the newly introduced btf__field_exists. So how about
bringing the convenience macro to libbpf?

The reason why this series is RFC is if people agree with the proposed changed
I'd be happy to extend it to add more *exists* APIs and do the conversion to
using the  for_each_member macro.

Nikolay Borisov (2):
  libbpf: Add userspace version of for_each_member macro
  libbpf: Add btf__field_exists

 tools/lib/bpf/btf.c      | 28 ++++++++++++++++++++++++++++
 tools/lib/bpf/btf.h      |  8 ++++++++
 tools/lib/bpf/libbpf.map |  1 +
 3 files changed, 37 insertions(+)

--
2.25.1

[RFC PATCH 1/2] libbpf: Add userspace version of for_each_member macro

[Nikolay Borisov]

There are multiple places in libbpf where iteration of struct/union
members is required. Instead of open-coding it let's introduce a
convenience macro.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 tools/lib/bpf/btf.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 061839f04525..74039f8afc63 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -578,6 +578,12 @@ static inline struct btf_decl_tag *btf_decl_tag(const struct btf_type *t)
 	return (struct btf_decl_tag *)(t + 1);
 }
 
+#define for_each_member(i, struct_type, member)         \
+	for (i = 0, member = btf_members(struct_type);  \
+	     i < btf_vlen(struct_type);                 \
+	     i++, member++)
+
+
 #ifdef __cplusplus
 } /* extern "C" */
 #endif
-- 
2.25.1

Re: [RFC PATCH 1/2] libbpf: Add userspace version of for_each_member macro

[Andrii Nakryiko]

On Mon, Apr 4, 2022 at 1:38 AM Nikolay Borisov <nborisov@suse.com> wrote:
>
> There are multiple places in libbpf where iteration of struct/union
> members is required. Instead of open-coding it let's introduce a
> convenience macro.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  tools/lib/bpf/btf.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 061839f04525..74039f8afc63 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -578,6 +578,12 @@ static inline struct btf_decl_tag *btf_decl_tag(const struct btf_type *t)
>         return (struct btf_decl_tag *)(t + 1);
>  }
>
> +#define for_each_member(i, struct_type, member)         \
> +       for (i = 0, member = btf_members(struct_type);  \
> +            i < btf_vlen(struct_type);                 \
> +            i++, member++)
> +
> +

this now becomes part of libbpf's API, do we really need it to be part
of an API? And even if we did add it, we'll need to do this for params
and enums for completeness.

>  #ifdef __cplusplus
>  } /* extern "C" */
>  #endif
> --
> 2.25.1
>

[RFC PATCH 2/2] libbpf: Add btf__field_exists

[Nikolay Borisov]

There isn't currently a convenience function to check if a particular
kernel version is running similar to bpf_core_field_exists. There can be
cases where based on the actual kernel being run different kprobes has
to be used when tracing the kernel. One example is the change introduced
in 4c5b47997521 ("vfs: add fileattr ops"). Before this commit if one
wants to trace fileattr changes this has to be done by a distinct kprobe
on every filesystem as there was no common code where fileattr changes
when through. Post this commit this can be performed by a single kprobe
on the common vfs_fileattr_set function.

To accommodate such use cases simply add a libbpf api btf__field_exists
which can be used to check for the running kernel version and act
appropriately.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 tools/lib/bpf/btf.c      | 28 ++++++++++++++++++++++++++++
 tools/lib/bpf/btf.h      |  2 ++
 tools/lib/bpf/libbpf.map |  1 +
 3 files changed, 31 insertions(+)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 9aa19c89f758..890a2071bd00 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -697,6 +697,34 @@ int btf__resolve_type(const struct btf *btf, __u32 type_id)
 	return type_id;
 }
 
+bool btf__field_exists(const struct btf *btf, const char *struct_name,
+		       const char *field_name)
+{
+	const struct btf_type *t;
+	struct btf_member *m;
+	int i;
+	__s32 type_id = btf__find_by_name(btf, struct_name);
+
+	if (type_id < 0)
+		return false;
+
+	t = btf__type_by_id(btf, type_id);
+	if (!t)
+		return false;
+
+	if (!btf_is_composite(t))
+		return false;
+
+	for_each_member(i, t, m) {
+		const char *n = btf__name_by_offset(btf, m->name_off);
+
+		if (strcmp(n, field_name) == 0)
+			return true;
+	}
+
+	return false;
+}
+
 __s32 btf__find_by_name(const struct btf *btf, const char *type_name)
 {
 	__u32 i, nr_types = btf__type_cnt(btf);
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 74039f8afc63..1eb8d840b46b 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -144,6 +144,8 @@ LIBBPF_API enum btf_endianness btf__endianness(const struct btf *btf);
 LIBBPF_API int btf__set_endianness(struct btf *btf, enum btf_endianness endian);
 LIBBPF_API __s64 btf__resolve_size(const struct btf *btf, __u32 type_id);
 LIBBPF_API int btf__resolve_type(const struct btf *btf, __u32 type_id);
+LIBBPF_API bool btf__field_exists(const struct btf *btf, const char *struct_name,
+				 const char *field_name);
 LIBBPF_API int btf__align_of(const struct btf *btf, __u32 id);
 LIBBPF_API int btf__fd(const struct btf *btf);
 LIBBPF_API void btf__set_fd(struct btf *btf, int fd);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 529783967793..9a0d50604cca 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -427,6 +427,7 @@ LIBBPF_0.7.0 {
 		bpf_program__log_level;
 		bpf_program__set_log_buf;
 		bpf_program__set_log_level;
+		btf__field_exists;
 		libbpf_probe_bpf_helper;
 		libbpf_probe_bpf_map_type;
 		libbpf_probe_bpf_prog_type;
-- 
2.25.1

Re: [RFC PATCH 0/2] Add btf__field_exists

[Andrii Nakryiko]

On Mon, Apr 4, 2022 at 1:38 AM Nikolay Borisov <nborisov@suse.com> wrote:
>
> Hello,
>
> Here are 2 patches with which I want to probe what's the sentiments towards 2
> changes:
>
> 1. Introduction of libbpf APIs similar to the bpf counterparts: bpf_core_field_exists,
> bpf_core_type_exists and bpf_core_enum_value_exists. Of those I've implemented only
> the first one and the reasoning behind this is in the patch itself. However, the
> TLDR is that there can be cases where based on the kernel version we have to make a
> decision in userspace what set of kprobes to use. There are currently no convenince
> api's to do this so one has to essentially open code the checks that can be provided
> by the aforementioned APIs.
>

The problem is that what you've implemented is not a user-space
equivalent of bpf_core_xxx() macros. CO-RE has extra logic around
___<flavor> suffixes, extra type checks, etc, etc. Helper you are
adding does a very straightforward strings check, which isn't hard to
implement and it doesn't have to be a set in stone API. So I'm a bit
hesitant to add this.

But I can share what I did in similar situations where I had to do
some CO-RE check both on BPF side and know its result in user-space. I
built a separate very simple BPF skeleton and all it did was perform
various feature checks (including those that require CO-RE) and then
returned the result through global variables. You can then trigger
such BPF feature-checking program either through bpf_prog_test_run or
through whatever other means (I actually did a simple sys_enter
program in my case). See [0] for BPF program side and [1] for
user-space activation/consumption of that.

The benefit of this approach is that there is no way BPF and
user-space sides can get "out of sync" in terms of their feature
checking. With skeleton it's also extremely simple to do all this.

  [0] https://github.com/anakryiko/retsnoop/blob/master/src/calib_feat.bpf.c
  [1] https://github.com/anakryiko/retsnoop/blob/master/src/mass_attacher.c#L483-L529


> 2. The kernel has for_each_member macro but the libbpf library doesn't provide it,
> this results in having to open code members enumeration in various places such as
> in find_member_by_name/find_struct_ops_kern_types/bpf_map__init_kern_struct_ops/
> parse_btf_map_def  and in the newly introduced btf__field_exists. So how about
> bringing the convenience macro to libbpf?

see my comment, not sure it's worth it

>
> The reason why this series is RFC is if people agree with the proposed changed
> I'd be happy to extend it to add more *exists* APIs and do the conversion to
> using the  for_each_member macro.
>
> Nikolay Borisov (2):
>   libbpf: Add userspace version of for_each_member macro
>   libbpf: Add btf__field_exists
>
>  tools/lib/bpf/btf.c      | 28 ++++++++++++++++++++++++++++
>  tools/lib/bpf/btf.h      |  8 ++++++++
>  tools/lib/bpf/libbpf.map |  1 +
>  3 files changed, 37 insertions(+)
>
> --
> 2.25.1
>

Re: [RFC PATCH 0/2] Add btf__field_exists

[Nikolay Borisov]

On 6.04.22 г. 2:37 ч., Andrii Nakryiko wrote:
> The problem is that what you've implemented is not a user-space
> equivalent of bpf_core_xxx() macros. CO-RE has extra logic around
> ___<flavor> suffixes, extra type checks, etc, etc. Helper you are
> adding does a very straightforward strings check, which isn't hard to
> implement and it doesn't have to be a set in stone API. So I'm a bit
> hesitant to add this.
> 
> But I can share what I did in similar situations where I had to do
> some CO-RE check both on BPF side and know its result in user-space. I
> built a separate very simple BPF skeleton and all it did was perform
> various feature checks (including those that require CO-RE) and then
> returned the result through global variables. You can then trigger
> such BPF feature-checking program either through bpf_prog_test_run or
> through whatever other means (I actually did a simple sys_enter
> program in my case). See [0] for BPF program side and [1] for
> user-space activation/consumption of that.
> 
> The benefit of this approach is that there is no way BPF and
> user-space sides can get "out of sync" in terms of their feature
> checking. With skeleton it's also extremely simple to do all this.
> 
>    [0]https://github.com/anakryiko/retsnoop/blob/master/src/calib_feat.bpf.c
>    [1]https://github.com/anakryiko/retsnoop/blob/master/src/mass_attacher.c#L483-L529
> 


That's indeed neat, however what is the minimum kernel version required 
to have global variables work ? AFAIU one requirement is to use a 
recent-enough libbpf which supports the skeleton functionality which is 
fine, userspace components can be updated somewhat easily than target 
kernels.

Re: [RFC PATCH 0/2] Add btf__field_exists

[Andrii Nakryiko]

On Tue, Apr 5, 2022 at 11:41 PM Nikolay Borisov <nborisov@suse.com> wrote:
>
>
>
> On 6.04.22 г. 2:37 ч., Andrii Nakryiko wrote:
> > The problem is that what you've implemented is not a user-space
> > equivalent of bpf_core_xxx() macros. CO-RE has extra logic around
> > ___<flavor> suffixes, extra type checks, etc, etc. Helper you are
> > adding does a very straightforward strings check, which isn't hard to
> > implement and it doesn't have to be a set in stone API. So I'm a bit
> > hesitant to add this.
> >
> > But I can share what I did in similar situations where I had to do
> > some CO-RE check both on BPF side and know its result in user-space. I
> > built a separate very simple BPF skeleton and all it did was perform
> > various feature checks (including those that require CO-RE) and then
> > returned the result through global variables. You can then trigger
> > such BPF feature-checking program either through bpf_prog_test_run or
> > through whatever other means (I actually did a simple sys_enter
> > program in my case). See [0] for BPF program side and [1] for
> > user-space activation/consumption of that.
> >
> > The benefit of this approach is that there is no way BPF and
> > user-space sides can get "out of sync" in terms of their feature
> > checking. With skeleton it's also extremely simple to do all this.
> >
> >    [0]https://github.com/anakryiko/retsnoop/blob/master/src/calib_feat.bpf.c
> >    [1]https://github.com/anakryiko/retsnoop/blob/master/src/mass_attacher.c#L483-L529
> >
>
>
> That's indeed neat, however what is the minimum kernel version required
> to have global variables work ? AFAIU one requirement is to use a
> recent-enough libbpf which supports the skeleton functionality which is
> fine, userspace components can be updated somewhat easily than target
> kernels.

You need Linux 5.5 for global variables mapping into user-space (added
in [0]). But you don't need to use global variables to pass
information back to user-space. You can just do a trivial
BPF_MAP_TYPE_ARRAY for this. It's a bit more verbose to work with from
BPF and user-space side, but effectively is the same for this case.
Then you basically have no extra Linux version constraints just
because of this feature probing. Only the need to have vmlinux BTF.

  [0] fc9702273e2e ("bpf: Add mmap() support for BPF_MAP_TYPE_ARRAY")