[PATCH bpf-next 0/2] libbpf: support "module:function" syntax for tracing programs

[PATCH bpf-next 0/2] libbpf: support "module:function" syntax for tracing programs

[Viktor Malik]

In some situations, it is useful to explicitly specify a kernel module
to search for a tracing program target (e.g. when a function of the same
name exists in multiple modules or in vmlinux).

This change enables that by allowing the "module:function" syntax for
the find_kernel_btf_id function. Thanks to this, the syntax can be used
both from a SEC macro (i.e. `SEC(fentry/module:function)`) and via the
bpf_program__set_attach_target API call.

Viktor Malik (2):
  libbpf: support "module:function" syntax for tracing programs
  selftests/bpf: add tests for the "module:function" syntax

 tools/lib/bpf/libbpf.c                        | 33 ++++++++++++++-----
 .../selftests/bpf/prog_tests/module_attach.c  |  6 ++++
 .../selftests/bpf/progs/test_module_attach.c  | 23 +++++++++++++
 3 files changed, 53 insertions(+), 9 deletions(-)

-- 
2.44.0

[PATCH bpf-next 1/2] libbpf: support "module:function" syntax for tracing programs

[Viktor Malik]

In some situations, it is useful to explicitly specify a kernel module
to search for a tracing program target (e.g. when a function of the same
name exists in multiple modules or in vmlinux).

This patch enables that by allowing the "module:function" syntax for the
find_kernel_btf_id function. Thanks to this, the syntax can be used both
from a SEC macro (i.e. `SEC(fentry/module:function)`) and via the
bpf_program__set_attach_target API call.

Signed-off-by: Viktor Malik <vmalik@redhat.com>
---
 tools/lib/bpf/libbpf.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 97eb6e5dd7c8..5a136876cd1c 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9858,16 +9858,28 @@ static int find_kernel_btf_id(struct bpf_object *obj, const char *attach_name,
 			      enum bpf_attach_type attach_type,
 			      int *btf_obj_fd, int *btf_type_id)
 {
-	int ret, i;
+	int ret, i, mod_len;
+	const char *fun_name, *mod_name = NULL;
 
-	ret = find_attach_btf_id(obj->btf_vmlinux, attach_name, attach_type);
-	if (ret > 0) {
-		*btf_obj_fd = 0; /* vmlinux BTF */
-		*btf_type_id = ret;
-		return 0;
+	fun_name = strchr(attach_name, ':');
+	if (fun_name) {
+		mod_name = attach_name;
+		mod_len = fun_name - mod_name;
+		fun_name++;
+	}
+
+	if (!mod_name || strncmp(mod_name, "vmlinux", mod_len) == 0) {
+		ret = find_attach_btf_id(obj->btf_vmlinux,
+					 mod_name ? fun_name : attach_name,
+					 attach_type);
+		if (ret > 0) {
+			*btf_obj_fd = 0; /* vmlinux BTF */
+			*btf_type_id = ret;
+			return 0;
+		}
+		if (ret != -ENOENT)
+			return ret;
 	}
-	if (ret != -ENOENT)
-		return ret;
 
 	ret = load_module_btfs(obj);
 	if (ret)
@@ -9876,7 +9888,10 @@ static int find_kernel_btf_id(struct bpf_object *obj, const char *attach_name,
 	for (i = 0; i < obj->btf_module_cnt; i++) {
 		const struct module_btf *mod = &obj->btf_modules[i];
 
-		ret = find_attach_btf_id(mod->btf, attach_name, attach_type);
+		if (mod_name && strncmp(mod->name, mod_name, mod_len))
+			continue;
+
+		ret = find_attach_btf_id(mod->btf, mod_name ? fun_name : attach_name, attach_type);
 		if (ret > 0) {
 			*btf_obj_fd = mod->fd;
 			*btf_type_id = ret;
-- 
2.44.0

Re: [PATCH bpf-next 1/2] libbpf: support "module:function" syntax for tracing programs

[Andrii Nakryiko]

On Fri, Apr 26, 2024 at 5:17 AM Viktor Malik <vmalik@redhat.com> wrote:
>
> In some situations, it is useful to explicitly specify a kernel module
> to search for a tracing program target (e.g. when a function of the same
> name exists in multiple modules or in vmlinux).
>
> This patch enables that by allowing the "module:function" syntax for the
> find_kernel_btf_id function. Thanks to this, the syntax can be used both
> from a SEC macro (i.e. `SEC(fentry/module:function)`) and via the
> bpf_program__set_attach_target API call.
>
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> ---
>  tools/lib/bpf/libbpf.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>

Looks good, just stylistic nits below.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 97eb6e5dd7c8..5a136876cd1c 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -9858,16 +9858,28 @@ static int find_kernel_btf_id(struct bpf_object *obj, const char *attach_name,
>                               enum bpf_attach_type attach_type,
>                               int *btf_obj_fd, int *btf_type_id)
>  {
> -       int ret, i;
> +       int ret, i, mod_len;
> +       const char *fun_name, *mod_name = NULL;

bikeshedding nit, but let's call it fn_name ("fun" doesn't associate
with "function" in my head at all)

>
> -       ret = find_attach_btf_id(obj->btf_vmlinux, attach_name, attach_type);
> -       if (ret > 0) {
> -               *btf_obj_fd = 0; /* vmlinux BTF */
> -               *btf_type_id = ret;
> -               return 0;
> +       fun_name = strchr(attach_name, ':');
> +       if (fun_name) {
> +               mod_name = attach_name;
> +               mod_len = fun_name - mod_name;
> +               fun_name++;
> +       }
> +
> +       if (!mod_name || strncmp(mod_name, "vmlinux", mod_len) == 0) {
> +               ret = find_attach_btf_id(obj->btf_vmlinux,
> +                                        mod_name ? fun_name : attach_name,
> +                                        attach_type);
> +               if (ret > 0) {
> +                       *btf_obj_fd = 0; /* vmlinux BTF */
> +                       *btf_type_id = ret;
> +                       return 0;
> +               }
> +               if (ret != -ENOENT)
> +                       return ret;
>         }
> -       if (ret != -ENOENT)
> -               return ret;
>
>         ret = load_module_btfs(obj);
>         if (ret)
> @@ -9876,7 +9888,10 @@ static int find_kernel_btf_id(struct bpf_object *obj, const char *attach_name,
>         for (i = 0; i < obj->btf_module_cnt; i++) {
>                 const struct module_btf *mod = &obj->btf_modules[i];
>
> -               ret = find_attach_btf_id(mod->btf, attach_name, attach_type);
> +               if (mod_name && strncmp(mod->name, mod_name, mod_len))

please add explicit `== 0` after strncmp(), like you did above for vmlinux

> +                       continue;
> +
> +               ret = find_attach_btf_id(mod->btf, mod_name ? fun_name : attach_name, attach_type);
>                 if (ret > 0) {
>                         *btf_obj_fd = mod->fd;
>                         *btf_type_id = ret;
> --
> 2.44.0
>

[PATCH bpf-next 2/2] selftests/bpf: add tests for the "module:function" syntax

[Viktor Malik]

The previous patch added support for the "module:function" syntax for
tracing programs. This adds tests for explicitly specifying the module
name via the SEC macro and via the bpf_program__set_attach_target call.

Signed-off-by: Viktor Malik <vmalik@redhat.com>
---
 .../selftests/bpf/prog_tests/module_attach.c  |  6 +++++
 .../selftests/bpf/progs/test_module_attach.c  | 23 +++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c
index f53d658ed080..6d391d95f96e 100644
--- a/tools/testing/selftests/bpf/prog_tests/module_attach.c
+++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c
@@ -51,6 +51,10 @@ void test_module_attach(void)
 					     0, "bpf_testmod_test_read");
 	ASSERT_OK(err, "set_attach_target");
 
+	err = bpf_program__set_attach_target(skel->progs.handle_fentry_explicit_manual,
+					     0, "bpf_testmod:bpf_testmod_test_read");
+	ASSERT_OK(err, "set_attach_target_explicit");
+
 	err = test_module_attach__load(skel);
 	if (CHECK(err, "skel_load", "failed to load skeleton\n"))
 		return;
@@ -70,6 +74,8 @@ void test_module_attach(void)
 	ASSERT_EQ(bss->tp_btf_read_sz, READ_SZ, "tp_btf");
 	ASSERT_EQ(bss->fentry_read_sz, READ_SZ, "fentry");
 	ASSERT_EQ(bss->fentry_manual_read_sz, READ_SZ, "fentry_manual");
+	ASSERT_EQ(bss->fentry_explicit_read_sz, READ_SZ, "fentry_explicit");
+	ASSERT_EQ(bss->fentry_explicit_manual_read_sz, READ_SZ, "fentry_explicit_manual");
 	ASSERT_EQ(bss->fexit_read_sz, READ_SZ, "fexit");
 	ASSERT_EQ(bss->fexit_ret, -EIO, "fexit_tet");
 	ASSERT_EQ(bss->fmod_ret_read_sz, READ_SZ, "fmod_ret");
diff --git a/tools/testing/selftests/bpf/progs/test_module_attach.c b/tools/testing/selftests/bpf/progs/test_module_attach.c
index 8a1b50f3a002..cc1a012d038f 100644
--- a/tools/testing/selftests/bpf/progs/test_module_attach.c
+++ b/tools/testing/selftests/bpf/progs/test_module_attach.c
@@ -73,6 +73,29 @@ int BPF_PROG(handle_fentry_manual,
 	return 0;
 }
 
+__u32 fentry_explicit_read_sz = 0;
+
+SEC("fentry/bpf_testmod:bpf_testmod_test_read")
+int BPF_PROG(handle_fentry_explicit,
+	     struct file *file, struct kobject *kobj,
+	     struct bin_attribute *bin_attr, char *buf, loff_t off, size_t len)
+{
+	fentry_explicit_read_sz = len;
+	return 0;
+}
+
+
+__u32 fentry_explicit_manual_read_sz = 0;
+
+SEC("fentry")
+int BPF_PROG(handle_fentry_explicit_manual,
+	     struct file *file, struct kobject *kobj,
+	     struct bin_attribute *bin_attr, char *buf, loff_t off, size_t len)
+{
+	fentry_explicit_manual_read_sz = len;
+	return 0;
+}
+
 __u32 fexit_read_sz = 0;
 int fexit_ret = 0;
 
-- 
2.44.0

Re: [PATCH bpf-next 2/2] selftests/bpf: add tests for the "module:function" syntax

[Andrii Nakryiko]

On Fri, Apr 26, 2024 at 5:17 AM Viktor Malik <vmalik@redhat.com> wrote:
>
> The previous patch added support for the "module:function" syntax for
> tracing programs. This adds tests for explicitly specifying the module
> name via the SEC macro and via the bpf_program__set_attach_target call.
>
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> ---
>  .../selftests/bpf/prog_tests/module_attach.c  |  6 +++++
>  .../selftests/bpf/progs/test_module_attach.c  | 23 +++++++++++++++++++
>  2 files changed, 29 insertions(+)
>

LGTM

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

[...]

Re: [PATCH bpf-next 0/2] libbpf: support "module:function" syntax for tracing programs

[Andrii Nakryiko]

On Fri, Apr 26, 2024 at 5:17 AM Viktor Malik <vmalik@redhat.com> wrote:
>
> In some situations, it is useful to explicitly specify a kernel module
> to search for a tracing program target (e.g. when a function of the same
> name exists in multiple modules or in vmlinux).
>
> This change enables that by allowing the "module:function" syntax for
> the find_kernel_btf_id function. Thanks to this, the syntax can be used
> both from a SEC macro (i.e. `SEC(fentry/module:function)`) and via the
> bpf_program__set_attach_target API call.
>

how about function[module] syntax. This follows how modules are
reported in kallsyms and a bunch of other kernel-generated files. I've
been using this syntax in retsnoop for a while, and it feels very
natural. It's also distinctive enough to be recognizable and parseable
without any possible confusions.

Can you please also check if we can/should support this for kprobes as well?

> Viktor Malik (2):
>   libbpf: support "module:function" syntax for tracing programs
>   selftests/bpf: add tests for the "module:function" syntax
>
>  tools/lib/bpf/libbpf.c                        | 33 ++++++++++++++-----
>  .../selftests/bpf/prog_tests/module_attach.c  |  6 ++++
>  .../selftests/bpf/progs/test_module_attach.c  | 23 +++++++++++++
>  3 files changed, 53 insertions(+), 9 deletions(-)
>
> --
> 2.44.0
>

Re: [PATCH bpf-next 0/2] libbpf: support "module:function" syntax for tracing programs

[Viktor Malik]

On 4/26/24 18:54, Andrii Nakryiko wrote:
> On Fri, Apr 26, 2024 at 5:17 AM Viktor Malik <vmalik@redhat.com> wrote:
>>
>> In some situations, it is useful to explicitly specify a kernel module
>> to search for a tracing program target (e.g. when a function of the same
>> name exists in multiple modules or in vmlinux).
>>
>> This change enables that by allowing the "module:function" syntax for
>> the find_kernel_btf_id function. Thanks to this, the syntax can be used
>> both from a SEC macro (i.e. `SEC(fentry/module:function)`) and via the
>> bpf_program__set_attach_target API call.
>>
> 
> how about function[module] syntax. This follows how modules are
> reported in kallsyms and a bunch of other kernel-generated files. I've
> been using this syntax in retsnoop for a while, and it feels very
> natural. It's also distinctive enough to be recognizable and parseable
> without any possible confusions.
> 
> Can you please also check if we can/should support this for kprobes as well?

For kprobes, it's a bit more complicated. The legacy kprobe attachment
(via tracefs) supports the "module:function" syntax [1] (which is the
reason I chose that syntax in the first place). On the other hand,
kprobe attachment via the perf_event_open syscall eventually calls
kallsyms_lookup_name for the passed function name which doesn't support
specifying the module at all.

So, to properly support this for kprobes, we'd have to extend
kallsyms_lookup_name to handle the "function[module]" or
"module:function" syntax (here, the former makes more sense).

Since kallsyms_lookup_name is used in many places, I would prefer adding
the kprobe support separately. In any case, the "function[module]"
syntax feels more natural for non-legacy kprobes, so I'm ok with using
it. libbpf will just have to do the transformation to "module:function"
for legacy kprobe attachment.

Let me know if that makes sense and I'll post v2.

Thanks!
Viktor

[1] https://www.kernel.org/doc/Documentation/trace/kprobetrace.rst

> 
>> Viktor Malik (2):
>>   libbpf: support "module:function" syntax for tracing programs
>>   selftests/bpf: add tests for the "module:function" syntax
>>
>>  tools/lib/bpf/libbpf.c                        | 33 ++++++++++++++-----
>>  .../selftests/bpf/prog_tests/module_attach.c  |  6 ++++
>>  .../selftests/bpf/progs/test_module_attach.c  | 23 +++++++++++++
>>  3 files changed, 53 insertions(+), 9 deletions(-)
>>
>> --
>> 2.44.0
>>
> 

Re: [PATCH bpf-next 0/2] libbpf: support "module:function" syntax for tracing programs

[Andrii Nakryiko]

On Mon, Apr 29, 2024 at 5:32 AM Viktor Malik <vmalik@redhat.com> wrote:
>
> On 4/26/24 18:54, Andrii Nakryiko wrote:
> > On Fri, Apr 26, 2024 at 5:17 AM Viktor Malik <vmalik@redhat.com> wrote:
> >>
> >> In some situations, it is useful to explicitly specify a kernel module
> >> to search for a tracing program target (e.g. when a function of the same
> >> name exists in multiple modules or in vmlinux).
> >>
> >> This change enables that by allowing the "module:function" syntax for
> >> the find_kernel_btf_id function. Thanks to this, the syntax can be used
> >> both from a SEC macro (i.e. `SEC(fentry/module:function)`) and via the
> >> bpf_program__set_attach_target API call.
> >>
> >
> > how about function[module] syntax. This follows how modules are
> > reported in kallsyms and a bunch of other kernel-generated files. I've
> > been using this syntax in retsnoop for a while, and it feels very
> > natural. It's also distinctive enough to be recognizable and parseable
> > without any possible confusions.
> >
> > Can you please also check if we can/should support this for kprobes as well?
>
> For kprobes, it's a bit more complicated. The legacy kprobe attachment
> (via tracefs) supports the "module:function" syntax [1] (which is the
> reason I chose that syntax in the first place). On the other hand,
> kprobe attachment via the perf_event_open syscall eventually calls
> kallsyms_lookup_name for the passed function name which doesn't support
> specifying the module at all.
>
> So, to properly support this for kprobes, we'd have to extend
> kallsyms_lookup_name to handle the "function[module]" or
> "module:function" syntax (here, the former makes more sense).
>
> Since kallsyms_lookup_name is used in many places, I would prefer adding
> the kprobe support separately. In any case, the "function[module]"
> syntax feels more natural for non-legacy kprobes, so I'm ok with using
> it. libbpf will just have to do the transformation to "module:function"
> for legacy kprobe attachment.
>
> Let me know if that makes sense and I'll post v2.

Thinking some more, I think we should stick to "<module>:<function>"
for a) simplicity (it's slightly easier to parse) and b) consistency
with uprobe, where we have "<path>:<function>", where path is pointing
to ELF binary. Let me take a look at patches again.

But yes, I think it would be useful to add support to
kprobes/multi-kprobes as well for completeness, but it of course would
be a separate work thread.

>
> Thanks!
> Viktor
>
> [1] https://www.kernel.org/doc/Documentation/trace/kprobetrace.rst
>
> >
> >> Viktor Malik (2):
> >>   libbpf: support "module:function" syntax for tracing programs
> >>   selftests/bpf: add tests for the "module:function" syntax
> >>
> >>  tools/lib/bpf/libbpf.c                        | 33 ++++++++++++++-----
> >>  .../selftests/bpf/prog_tests/module_attach.c  |  6 ++++
> >>  .../selftests/bpf/progs/test_module_attach.c  | 23 +++++++++++++
> >>  3 files changed, 53 insertions(+), 9 deletions(-)
> >>
> >> --
> >> 2.44.0
> >>
> >
>