[bpf-next,v3,3/3] bpf/selftests: Test fentry attachment to shadowed functions

Commit Message

Viktor Malik Dec. 5, 2022, 3:26 p.m. UTC

Adds a new test that tries to attach a program to fentry of two
functions of the same name, one located in vmlinux and the other in
bpf_testmod.

To avoid conflicts with existing tests, a new function
"bpf_fentry_shadow_test" was created both in vmlinux and in bpf_testmod.

The previous commit fixed a bug which caused this test to fail. The
verifier would always use the vmlinux function's address as the target
trampoline address, hence trying to attach two programs to the same
trampoline.

Signed-off-by: Viktor Malik <[e-mail]>
---
 net/bpf/test_run.c                            |   5 +
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   7 +
 .../bpf/prog_tests/module_attach_shadow.c     | 124 ++++++++++++++++++
 3 files changed, 136 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c

Comments

Jiri Olsa Dec. 5, 2022, 8:49 p.m. UTC | #1

On Mon, Dec 05, 2022 at 04:26:06PM +0100, Viktor Malik wrote:
> Adds a new test that tries to attach a program to fentry of two
> functions of the same name, one located in vmlinux and the other in
> bpf_testmod.
> 
> To avoid conflicts with existing tests, a new function
> "bpf_fentry_shadow_test" was created both in vmlinux and in bpf_testmod.
> 
> The previous commit fixed a bug which caused this test to fail. The
> verifier would always use the vmlinux function's address as the target
> trampoline address, hence trying to attach two programs to the same
> trampoline.

hi
looks good, few nits below

> 
> Signed-off-by: Viktor Malik <[e-mail]>
> ---
>  net/bpf/test_run.c                            |   5 +
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   7 +
>  .../bpf/prog_tests/module_attach_shadow.c     | 124 ++++++++++++++++++
>  3 files changed, 136 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
> 
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 6094ef7cffcd..71e36a85573b 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -536,6 +536,11 @@ int noinline bpf_modify_return_test(int a, int *b)
>  	return a + *b;
>  }
>  
> +int noinline bpf_fentry_shadow_test(int a)
> +{
> +	return a + 1;
> +}
> +
>  u64 noinline bpf_kfunc_call_test1(struct sock *sk, u32 a, u64 b, u32 c, u64 d)
>  {
>  	return a + b + c + d;
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index 5085fea3cac5..d23127a5ec68 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -229,6 +229,13 @@ static const struct btf_kfunc_id_set bpf_testmod_kfunc_set = {
>  	.set   = &bpf_testmod_check_kfunc_ids,
>  };
>  
> +noinline int bpf_fentry_shadow_test(int a)
> +{
> +	return a + 2;
> +}
> +EXPORT_SYMBOL_GPL(bpf_fentry_shadow_test);
> +ALLOW_ERROR_INJECTION(bpf_fentry_shadow_test, ERRNO);

why marked as ALLOW_ERROR_INJECTION?

> +
>  extern int bpf_fentry_test1(int a);
>  
>  static int bpf_testmod_init(void)
> diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c b/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
> new file mode 100644
> index 000000000000..bf511e61ec1f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Red Hat */
> +#include <test_progs.h>
> +#include <bpf/btf.h>
> +#include "bpf/libbpf_internal.h"
> +#include "cgroup_helpers.h"
> +
> +static const char *module_name = "bpf_testmod";
> +static const char *symbol_name = "bpf_fentry_shadow_test";
> +
> +int get_bpf_testmod_btf_fd(void)

should be static?

> +{
> +	struct bpf_btf_info info;
> +	char name[64];
> +	__u32 id = 0, len;
> +	int err, fd;
> +
> +	while (true) {
> +		err = bpf_btf_get_next_id(id, &id);
> +		if (err) {
> +			log_err("failed to iterate BTF objects");
> +			return err;
> +		}
> +
> +		fd = bpf_btf_get_fd_by_id(id);
> +		if (fd < 0) {

I was checking how's libbpf doing this and found load_module_btfs,
which seems similar.. and it has one additional check in here:

                        if (errno == ENOENT)
                                continue; /* expected race: BTF was unloaded */

I guess it's not likely, but it's better to have it


SNIP

> +	btf_id[0] = btf__find_by_name_kind(vmlinux_btf, symbol_name, BTF_KIND_FUNC);
> +	if (!ASSERT_GT(btf_id[0], 0, "btf_find_by_name"))
> +		goto out;
> +
> +	btf_id[1] = btf__find_by_name_kind(mod_btf, symbol_name, BTF_KIND_FUNC);
> +	if (!ASSERT_GT(btf_id[1], 0, "btf_find_by_name"))
> +		goto out;
> +
> +	for (i = 0; i < 2; i++) {
> +		load_opts.attach_btf_id = btf_id[i];
> +		load_opts.attach_btf_obj_fd = btf_fd[i];
> +		prog_fd[i] = bpf_prog_load(BPF_PROG_TYPE_TRACING, NULL, "GPL",
> +					   trace_program,
> +					   sizeof(trace_program) / sizeof(struct bpf_insn),
> +					   &load_opts);
> +		if (!ASSERT_GE(prog_fd[i], 0, "bpf_prog_load"))
> +			goto out;
> +
> +		link_fd[i] = bpf_link_create(prog_fd[i], 0, BPF_TRACE_FENTRY, NULL);
> +		if (!ASSERT_GE(link_fd[i], 0, "bpf_link_create"))
> +			goto out;

so IIUC the issue is that without the previous fix this will create
2 separate trampolines pointing to single address.. and we can have
just one trampoline for address.. so the 2nd trampoline update will
fail, because the trampoline location is already changed/taken ?

could you please put some description like that in the comment or
changelog?

thanks,
jirka

> +	}
> +
> +	err = bpf_prog_test_run_opts(prog_fd[0], &test_opts);
> +	ASSERT_OK(err, "running test");
> +
> +out:
> +	if (vmlinux_btf)
> +		btf__free(vmlinux_btf);
> +	if (mod_btf)
> +		btf__free(mod_btf);
> +	for (i = 0; i < 2; i++) {
> +		if (btf_fd[i])
> +			close(btf_fd[i]);
> +		if (prog_fd[i])
> +			close(prog_fd[i]);
> +		if (link_fd[i])
> +			close(link_fd[i]);
> +	}
> +}
> -- 
> 2.38.1
>

Viktor Malik Dec. 6, 2022, 6:15 a.m. UTC | #2

On 12/5/22 21:49, Jiri Olsa wrote:
> On Mon, Dec 05, 2022 at 04:26:06PM +0100, Viktor Malik wrote:
>> Adds a new test that tries to attach a program to fentry of two
>> functions of the same name, one located in vmlinux and the other in
>> bpf_testmod.
>>
>> To avoid conflicts with existing tests, a new function
>> "bpf_fentry_shadow_test" was created both in vmlinux and in bpf_testmod.
>>
>> The previous commit fixed a bug which caused this test to fail. The
>> verifier would always use the vmlinux function's address as the target
>> trampoline address, hence trying to attach two programs to the same
>> trampoline.
> 
> hi
> looks good, few nits below
> 
>>
>> Signed-off-by: Viktor Malik <[e-mail]>
>> ---
>>   net/bpf/test_run.c                            |   5 +
>>   .../selftests/bpf/bpf_testmod/bpf_testmod.c   |   7 +
>>   .../bpf/prog_tests/module_attach_shadow.c     | 124 ++++++++++++++++++
>>   3 files changed, 136 insertions(+)
>>   create mode 100644 tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
>>
>> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
>> index 6094ef7cffcd..71e36a85573b 100644
>> --- a/net/bpf/test_run.c
>> +++ b/net/bpf/test_run.c
>> @@ -536,6 +536,11 @@ int noinline bpf_modify_return_test(int a, int *b)
>>   	return a + *b;
>>   }
>>   
>> +int noinline bpf_fentry_shadow_test(int a)
>> +{
>> +	return a + 1;
>> +}
>> +
>>   u64 noinline bpf_kfunc_call_test1(struct sock *sk, u32 a, u64 b, u32 c, u64 d)
>>   {
>>   	return a + b + c + d;
>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> index 5085fea3cac5..d23127a5ec68 100644
>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> @@ -229,6 +229,13 @@ static const struct btf_kfunc_id_set bpf_testmod_kfunc_set = {
>>   	.set   = &bpf_testmod_check_kfunc_ids,
>>   };
>>   
>> +noinline int bpf_fentry_shadow_test(int a)
>> +{
>> +	return a + 2;
>> +}
>> +EXPORT_SYMBOL_GPL(bpf_fentry_shadow_test);
>> +ALLOW_ERROR_INJECTION(bpf_fentry_shadow_test, ERRNO);
> 
> why marked as ALLOW_ERROR_INJECTION?

Right, not necessary, will remove.

> 
>> +
>>   extern int bpf_fentry_test1(int a);
>>   
>>   static int bpf_testmod_init(void)
>> diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c b/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
>> new file mode 100644
>> index 000000000000..bf511e61ec1f
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
>> @@ -0,0 +1,124 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2022 Red Hat */
>> +#include <test_progs.h>
>> +#include <bpf/btf.h>
>> +#include "bpf/libbpf_internal.h"
>> +#include "cgroup_helpers.h"
>> +
>> +static const char *module_name = "bpf_testmod";
>> +static const char *symbol_name = "bpf_fentry_shadow_test";
>> +
>> +int get_bpf_testmod_btf_fd(void)
> 
> should be static?

Yes, I believe.

> 
>> +{
>> +	struct bpf_btf_info info;
>> +	char name[64];
>> +	__u32 id = 0, len;
>> +	int err, fd;
>> +
>> +	while (true) {
>> +		err = bpf_btf_get_next_id(id, &id);
>> +		if (err) {
>> +			log_err("failed to iterate BTF objects");
>> +			return err;
>> +		}
>> +
>> +		fd = bpf_btf_get_fd_by_id(id);
>> +		if (fd < 0) {
> 
> I was checking how's libbpf doing this and found load_module_btfs,
> which seems similar.. and it has one additional check in here:
> 
>                          if (errno == ENOENT)
>                                  continue; /* expected race: BTF was unloaded */
> 
> I guess it's not likely, but it's better to have it

Sure, will add it. You're right, the implementation is mostly taken from
libbpf's load_module_btfs.

> 
> 
> SNIP
> 
>> +	btf_id[0] = btf__find_by_name_kind(vmlinux_btf, symbol_name, BTF_KIND_FUNC);
>> +	if (!ASSERT_GT(btf_id[0], 0, "btf_find_by_name"))
>> +		goto out;
>> +
>> +	btf_id[1] = btf__find_by_name_kind(mod_btf, symbol_name, BTF_KIND_FUNC);
>> +	if (!ASSERT_GT(btf_id[1], 0, "btf_find_by_name"))
>> +		goto out;
>> +
>> +	for (i = 0; i < 2; i++) {
>> +		load_opts.attach_btf_id = btf_id[i];
>> +		load_opts.attach_btf_obj_fd = btf_fd[i];
>> +		prog_fd[i] = bpf_prog_load(BPF_PROG_TYPE_TRACING, NULL, "GPL",
>> +					   trace_program,
>> +					   sizeof(trace_program) / sizeof(struct bpf_insn),
>> +					   &load_opts);
>> +		if (!ASSERT_GE(prog_fd[i], 0, "bpf_prog_load"))
>> +			goto out;
>> +
>> +		link_fd[i] = bpf_link_create(prog_fd[i], 0, BPF_TRACE_FENTRY, NULL);
>> +		if (!ASSERT_GE(link_fd[i], 0, "bpf_link_create"))
>> +			goto out;
> 
> so IIUC the issue is that without the previous fix this will create
> 2 separate trampolines pointing to single address.. and we can have
> just one trampoline for address.. so the 2nd trampoline update will
> fail, because the trampoline location is already changed/taken ?
> 
> could you please put some description like that in the comment or
> changelog?

The description is already in the commit message and in the series
changelog, although it may be a bit inaccurate (stating that two
programs are attached to the same trampoline rather that two trampolines
attached to the same address).

I'll fix it and add it to the comment, it could be useful to have it
there as well.

Thanks!
Viktor

> 
> thanks,
> jirka
> 
>> +	}
>> +
>> +	err = bpf_prog_test_run_opts(prog_fd[0], &test_opts);
>> +	ASSERT_OK(err, "running test");
>> +
>> +out:
>> +	if (vmlinux_btf)
>> +		btf__free(vmlinux_btf);
>> +	if (mod_btf)
>> +		btf__free(mod_btf);
>> +	for (i = 0; i < 2; i++) {
>> +		if (btf_fd[i])
>> +			close(btf_fd[i]);
>> +		if (prog_fd[i])
>> +			close(prog_fd[i]);
>> +		if (link_fd[i])
>> +			close(link_fd[i]);
>> +	}
>> +}
>> -- 
>> 2.38.1
>>
>

Patch

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 6094ef7cffcd..71e36a85573b 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -536,6 +536,11 @@  int noinline bpf_modify_return_test(int a, int *b)
 	return a + *b;
 }
 
+int noinline bpf_fentry_shadow_test(int a)
+{
+	return a + 1;
+}
+
 u64 noinline bpf_kfunc_call_test1(struct sock *sk, u32 a, u64 b, u32 c, u64 d)
 {
 	return a + b + c + d;
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 5085fea3cac5..d23127a5ec68 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -229,6 +229,13 @@  static const struct btf_kfunc_id_set bpf_testmod_kfunc_set = {
 	.set   = &bpf_testmod_check_kfunc_ids,
 };
 
+noinline int bpf_fentry_shadow_test(int a)
+{
+	return a + 2;
+}
+EXPORT_SYMBOL_GPL(bpf_fentry_shadow_test);
+ALLOW_ERROR_INJECTION(bpf_fentry_shadow_test, ERRNO);
+
 extern int bpf_fentry_test1(int a);
 
 static int bpf_testmod_init(void)
diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c b/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
new file mode 100644
index 000000000000..bf511e61ec1f
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/module_attach_shadow.c
@@ -0,0 +1,124 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Red Hat */
+#include <test_progs.h>
+#include <bpf/btf.h>
+#include "bpf/libbpf_internal.h"
+#include "cgroup_helpers.h"
+
+static const char *module_name = "bpf_testmod";
+static const char *symbol_name = "bpf_fentry_shadow_test";
+
+int get_bpf_testmod_btf_fd(void)
+{
+	struct bpf_btf_info info;
+	char name[64];
+	__u32 id = 0, len;
+	int err, fd;
+
+	while (true) {
+		err = bpf_btf_get_next_id(id, &id);
+		if (err) {
+			log_err("failed to iterate BTF objects");
+			return err;
+		}
+
+		fd = bpf_btf_get_fd_by_id(id);
+		if (fd < 0) {
+			err = -errno;
+			log_err("failed to get FD for BTF object #%d", id);
+			return err;
+		}
+
+		len = sizeof(info);
+		memset(&info, 0, sizeof(info));
+		info.name = ptr_to_u64(name);
+		info.name_len = sizeof(name);
+
+		err = bpf_obj_get_info_by_fd(fd, &info, &len);
+		if (err) {
+			err = -errno;
+			log_err("failed to get info for BTF object #%d", id);
+			close(fd);
+			return err;
+		}
+
+		if (strcmp(name, module_name) == 0)
+			return fd;
+
+		close(fd);
+	}
+	return -ENOENT;
+}
+
+void test_module_fentry_shadow(void)
+{
+	struct btf *vmlinux_btf = NULL, *mod_btf = NULL;
+	int err, i;
+	int btf_fd[2] = {};
+	int prog_fd[2] = {};
+	int link_fd[2] = {};
+	__s32 btf_id[2] = {};
+
+	const struct bpf_insn trace_program[] = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	};
+
+	LIBBPF_OPTS(bpf_prog_load_opts, load_opts,
+		.expected_attach_type = BPF_TRACE_FENTRY,
+	);
+
+	LIBBPF_OPTS(bpf_test_run_opts, test_opts);
+
+	vmlinux_btf = btf__load_vmlinux_btf();
+	if (!ASSERT_OK_PTR(vmlinux_btf, "load_vmlinux_btf"))
+		return;
+
+	btf_fd[1] = get_bpf_testmod_btf_fd();
+	if (!ASSERT_GT(btf_fd[1], 0, "get_bpf_testmod_btf_fd"))
+		goto out;
+
+	mod_btf = btf_get_from_fd(btf_fd[1], vmlinux_btf);
+	if (!ASSERT_OK_PTR(mod_btf, "btf_get_from_fd"))
+		goto out;
+
+	btf_id[0] = btf__find_by_name_kind(vmlinux_btf, symbol_name, BTF_KIND_FUNC);
+	if (!ASSERT_GT(btf_id[0], 0, "btf_find_by_name"))
+		goto out;
+
+	btf_id[1] = btf__find_by_name_kind(mod_btf, symbol_name, BTF_KIND_FUNC);
+	if (!ASSERT_GT(btf_id[1], 0, "btf_find_by_name"))
+		goto out;
+
+	for (i = 0; i < 2; i++) {
+		load_opts.attach_btf_id = btf_id[i];
+		load_opts.attach_btf_obj_fd = btf_fd[i];
+		prog_fd[i] = bpf_prog_load(BPF_PROG_TYPE_TRACING, NULL, "GPL",
+					   trace_program,
+					   sizeof(trace_program) / sizeof(struct bpf_insn),
+					   &load_opts);
+		if (!ASSERT_GE(prog_fd[i], 0, "bpf_prog_load"))
+			goto out;
+
+		link_fd[i] = bpf_link_create(prog_fd[i], 0, BPF_TRACE_FENTRY, NULL);
+		if (!ASSERT_GE(link_fd[i], 0, "bpf_link_create"))
+			goto out;
+	}
+
+	err = bpf_prog_test_run_opts(prog_fd[0], &test_opts);
+	ASSERT_OK(err, "running test");
+
+out:
+	if (vmlinux_btf)
+		btf__free(vmlinux_btf);
+	if (mod_btf)
+		btf__free(mod_btf);
+	for (i = 0; i < 2; i++) {
+		if (btf_fd[i])
+			close(btf_fd[i]);
+		if (prog_fd[i])
+			close(prog_fd[i]);
+		if (link_fd[i])
+			close(link_fd[i]);
+	}
+}