[bpf-next,v2,3/3] selftests/bpf: Test the inlining of bpf_kptr_xchg()

Commit Message

Hou Tao Dec. 23, 2023, 10:40 a.m. UTC

From: Hou Tao <[e-mail]>

The test uses bpf_prog_get_info_by_fd() to obtain the xlated
instructions of the program first. Since these instructions have
already been rewritten by the verifier, the tests then checks whether
the rewritten instructions are as expected.

Signed-off-by: Hou Tao <[e-mail]>
---
 .../bpf/prog_tests/kptr_xchg_inline.c         | 51 +++++++++++++++++++
 .../selftests/bpf/progs/kptr_xchg_inline.c    | 28 ++++++++++
 2 files changed, 79 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/kptr_xchg_inline.c
 create mode 100644 tools/testing/selftests/bpf/progs/kptr_xchg_inline.c

Comments

Eduard Zingerman Jan. 2, 2024, 6:41 p.m. UTC | #1

On Sat, 2023-12-23 at 18:40 +0800, Hou Tao wrote:
> From: Hou Tao <[e-mail]>
> 
> The test uses bpf_prog_get_info_by_fd() to obtain the xlated
> instructions of the program first. Since these instructions have
> already been rewritten by the verifier, the tests then checks whether
> the rewritten instructions are as expected.
> 
> Signed-off-by: Hou Tao <[e-mail]>

Thank you for adding this test, one nitpick below.

[...]

> +#define private(name) SEC(".bss." #name) __hidden __attribute__((aligned(8)))
> +private(kptr) struct bin_data __kptr * ptr;
> +
> +SEC("tc")
> +int kptr_xchg_inline(void *ctx)
> +{
> +	void *old;
> +
> +	old = bpf_kptr_xchg(&ptr, NULL);
> +	if (old)
> +		bpf_obj_drop(old);
> +
> +	return 0;
> +}

This is highly unlikely, but in theory nothing guarantees that LLVM
would generate code exactly as expected by pattern in test_kptr_xchg_inline().
It would be more fail-proof to use inline assembly and a naked
function instead of C code, e.g.:

SEC("tc")
__naked int kptr_xchg_inline(void)
{
	asm volatile (
		"r1 = %[ptr] ll;"
		"r2 = 0;"
		"call %[bpf_kptr_xchg];"
		"if r0 == 0 goto 1f;"
		"r1 = r0;"
		"r2 = 0;"
		"call %[bpf_obj_drop_impl];"
"1:"
		"r0 = 0;"
		"exit;"
		:
		: __imm_addr(ptr),
		  __imm(bpf_kptr_xchg),
		  __imm(bpf_obj_drop_impl)
		: __clobber_all
	);
}

/* BTF FUNC records are not generated for kfuncs referenced
 * from inline assembly. These records are necessary for
 * libbpf to link the program. The function below is a hack
 * to ensure that BTF FUNC records are generated.
 */
void __btf_root(void)
{
	bpf_obj_drop(NULL);
}

wdyt?

Hou Tao Jan. 3, 2024, 1:20 a.m. UTC | #2

Hi,

On 1/3/2024 2:41 AM, Eduard Zingerman wrote:
> On Sat, 2023-12-23 at 18:40 +0800, Hou Tao wrote:
>> From: Hou Tao <[e-mail]>
>>
>> The test uses bpf_prog_get_info_by_fd() to obtain the xlated
>> instructions of the program first. Since these instructions have
>> already been rewritten by the verifier, the tests then checks whether
>> the rewritten instructions are as expected.
>>
>> Signed-off-by: Hou Tao <[e-mail]>
> Thank you for adding this test, one nitpick below.
>
> [...]
>
>> +#define private(name) SEC(".bss." #name) __hidden __attribute__((aligned(8)))
>> +private(kptr) struct bin_data __kptr * ptr;
>> +
>> +SEC("tc")
>> +int kptr_xchg_inline(void *ctx)
>> +{
>> +	void *old;
>> +
>> +	old = bpf_kptr_xchg(&ptr, NULL);
>> +	if (old)
>> +		bpf_obj_drop(old);
>> +
>> +	return 0;
>> +}
> This is highly unlikely, but in theory nothing guarantees that LLVM
> would generate code exactly as expected by pattern in test_kptr_xchg_inline().
> It would be more fail-proof to use inline assembly and a naked
> function instead of C code, e.g.:
>
> SEC("tc")
> __naked int kptr_xchg_inline(void)
> {
> 	asm volatile (
> 		"r1 = %[ptr] ll;"
> 		"r2 = 0;"
> 		"call %[bpf_kptr_xchg];"
> 		"if r0 == 0 goto 1f;"
> 		"r1 = r0;"
> 		"r2 = 0;"
> 		"call %[bpf_obj_drop_impl];"
> "1:"
> 		"r0 = 0;"
> 		"exit;"
> 		:
> 		: __imm_addr(ptr),
> 		  __imm(bpf_kptr_xchg),
> 		  __imm(bpf_obj_drop_impl)
> 		: __clobber_all
> 	);
> }
>
> /* BTF FUNC records are not generated for kfuncs referenced
>  * from inline assembly. These records are necessary for
>  * libbpf to link the program. The function below is a hack
>  * to ensure that BTF FUNC records are generated.
>  */
> void __btf_root(void)
> {
> 	bpf_obj_drop(NULL);
> }
>
> wdyt?

Sure, will update the C code to the inline assembly in v3. And thanks
for the review and the suggestions.

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/kptr_xchg_inline.c b/tools/testing/selftests/bpf/prog_tests/kptr_xchg_inline.c
new file mode 100644
index 000000000000..5a4bee1cf970
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/kptr_xchg_inline.c
@@ -0,0 +1,51 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2023. Huawei Technologies Co., Ltd */
+#include <test_progs.h>
+
+#include "linux/filter.h"
+#include "kptr_xchg_inline.skel.h"
+
+void test_kptr_xchg_inline(void)
+{
+	struct kptr_xchg_inline *skel;
+	struct bpf_insn *insn = NULL;
+	struct bpf_insn exp;
+	unsigned int cnt;
+	int err;
+
+#if !defined(__x86_64__)
+	test__skip();
+	return;
+#endif
+
+	skel = kptr_xchg_inline__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_load"))
+		return;
+
+	err = get_xlated_program(bpf_program__fd(skel->progs.kptr_xchg_inline), &insn, &cnt);
+	if (!ASSERT_OK(err, "prog insn"))
+		goto out;
+
+	/* The original instructions are:
+	 * r1 = map[id:xxx][0]+0
+	 * r2 = 0
+	 * call bpf_kptr_xchg#yyy
+	 *
+	 * call bpf_kptr_xchg#yyy will be inlined as:
+	 * r0 = r2
+	 * r0 = atomic64_xchg((u64 *)(r1 +0), r0)
+	 */
+	if (!ASSERT_GT(cnt, 5, "insn cnt"))
+		goto out;
+
+	exp = BPF_MOV64_REG(BPF_REG_0, BPF_REG_2);
+	if (!ASSERT_OK(memcmp(&insn[3], &exp, sizeof(exp)), "mov"))
+		goto out;
+
+	exp = BPF_ATOMIC_OP(BPF_DW, BPF_XCHG, BPF_REG_1, BPF_REG_0, 0);
+	if (!ASSERT_OK(memcmp(&insn[4], &exp, sizeof(exp)), "xchg"))
+		goto out;
+out:
+	free(insn);
+	kptr_xchg_inline__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/kptr_xchg_inline.c b/tools/testing/selftests/bpf/progs/kptr_xchg_inline.c
new file mode 100644
index 000000000000..1db7909828d1
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/kptr_xchg_inline.c
@@ -0,0 +1,28 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2023. Huawei Technologies Co., Ltd */
+#include <linux/types.h>
+#include <bpf/bpf_helpers.h>
+
+#include "bpf_experimental.h"
+#include "bpf_misc.h"
+
+char _license[] SEC("license") = "GPL";
+
+struct bin_data {
+	char blob[32];
+};
+
+#define private(name) SEC(".bss." #name) __hidden __attribute__((aligned(8)))
+private(kptr) struct bin_data __kptr * ptr;
+
+SEC("tc")
+int kptr_xchg_inline(void *ctx)
+{
+	void *old;
+
+	old = bpf_kptr_xchg(&ptr, NULL);
+	if (old)
+		bpf_obj_drop(old);
+
+	return 0;
+}