[PATCH bpf-next 0/3] follow up for __jited test tag

[PATCH bpf-next 0/3] follow up for __jited test tag

[Eduard Zingerman]

This patch-set is a collection of follow-ups for
"__jited test tag to check disassembly after jit" series (see [1]).

First patch is most important:
as it turns out, I broke all test_loader based tests for s390 CI.
E.g. see log [2] for s390 execution of test_progs,
note all 'verivier_*' tests being skipped.
This happens because of incorrect handling of corner case when
get_current_arch() does not know which architecture to return.

Second patch makes matching of function return sequence in
verifier_tailcall_jit more flexible:

    -__jited("	retq")
    +__jited("	{{(retq|jmp	0x)}}")

The difference could be seen with and w/o mitigations=off boot
parameter for test VM (CI runs with mitigations=off, hence it
generates retq).

Third patch addresses Alexei's request to add #define and a comment in
jit_disasm_helpers.c.

[1] https://lore.kernel.org/bpf/20240820102357.3372779-1-eddyz87@gmail.com/
[2] https://github.com/kernel-patches/bpf/actions/runs/10518445973/job/29144511595

Eduard Zingerman (3):
  selftests/bpf: test_loader.c:get_current_arch() should not return 0
  selftests/bpf: match both retq/rethunk in verifier_tailcall_jit
  selftests/bpf: #define LOCAL_LABEL_LEN for jit_disasm_helpers.c

 .../testing/selftests/bpf/jit_disasm_helpers.c  | 17 ++++++++++++++---
 .../selftests/bpf/progs/verifier_tailcall_jit.c |  4 ++--
 tools/testing/selftests/bpf/test_loader.c       |  9 +++++----
 3 files changed, 21 insertions(+), 9 deletions(-)

-- 
2.46.0

[PATCH bpf-next 1/3] selftests/bpf: test_loader.c:get_current_arch() should not return 0

[Eduard Zingerman]

At the moment, when test_loader.c:get_current_arch() can't determine
the arch, it returns 0. The arch check in run_subtest() looks as
follows:

	if ((get_current_arch() & spec->arch_mask) == 0) {
		test__skip();
		return;
	}

Which means that all test_loader based tests would be skipped if arch
could not be determined. get_current_arch() recognizes x86_64, arm64
and riscv64. Which means that CI skips test_loader tests for s390.

Fix this by making sure that get_current_arch() always returns
non-zero value. In combination with default spec->arch_mask == -1 this
should cover all possibilities.

Fixes: f406026fefa7 ("selftests/bpf: by default use arch mask allowing all archs")
Fixes: 7d743e4c759c ("selftests/bpf: __jited test tag to check disassembly after jit")
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/testing/selftests/bpf/test_loader.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 2ca9b73e5a6b..4223cffc090e 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -336,9 +336,10 @@ static const char *skip_dynamic_pfx(const char *s, const char *pfx)
 }
 
 enum arch {
-	ARCH_X86_64	= 0x1,
-	ARCH_ARM64	= 0x2,
-	ARCH_RISCV64	= 0x4,
+	ARCH_UNKNOWN	= 0x1,
+	ARCH_X86_64	= 0x2,
+	ARCH_ARM64	= 0x4,
+	ARCH_RISCV64	= 0x8,
 };
 
 static int get_current_arch(void)
@@ -350,7 +351,7 @@ static int get_current_arch(void)
 #elif defined(__riscv) && __riscv_xlen == 64
 	return ARCH_RISCV64;
 #endif
-	return 0;
+	return ARCH_UNKNOWN;
 }
 
 /* Uses btf_decl_tag attributes to describe the expected test
-- 
2.46.0

[PATCH bpf-next 2/3] selftests/bpf: match both retq/rethunk in verifier_tailcall_jit

[Eduard Zingerman]

Depending on kernel parameters, x86 jit generates either retq or jump
to rethunk for 'exit' instruction. The difference could be seen when
kernel is booted with and without mitigations=off parameter.
Relax the verifier_tailcall_jit test case to match both variants.

Fixes: e5bdd6a8be78 ("selftests/bpf: validate jit behaviour for tail calls")
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c b/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c
index 06d327cf1e1f..8d60c634a114 100644
--- a/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c
+++ b/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c
@@ -59,7 +59,7 @@ __jited("	movq	-0x10(%rbp), %rax")
 __jited("	callq	0x{{.*}}")		/* call to sub()          */
 __jited("	xorl	%eax, %eax")
 __jited("	leave")
-__jited("	retq")
+__jited("	{{(retq|jmp	0x)}}")		/* return or jump to rethunk */
 __jited("...")
 /* subprogram entry for sub(), regular function prologue */
 __jited("	endbr64")
@@ -89,7 +89,7 @@ __jited("	popq	%rax")
 __jited("	popq	%rax")
 __jited("	jmp	{{.*}}")		/* jump to tail call tgt   */
 __jited("L0:	leave")
-__jited("	retq")
+__jited("	{{(retq|jmp	0x)}}")		/* return or jump to rethunk */
 SEC("tc")
 __naked int main(void)
 {
-- 
2.46.0

[PATCH bpf-next 3/3] selftests/bpf: #define LOCAL_LABEL_LEN for jit_disasm_helpers.c

[Eduard Zingerman]

Extract local label length as a #define directive and
elaborate why 'i % MAX_LOCAL_LABELS' expression is needed
for local labels array initialization.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../testing/selftests/bpf/jit_disasm_helpers.c  | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/jit_disasm_helpers.c b/tools/testing/selftests/bpf/jit_disasm_helpers.c
index 1b0f1fd267c0..febd6b12e372 100644
--- a/tools/testing/selftests/bpf/jit_disasm_helpers.c
+++ b/tools/testing/selftests/bpf/jit_disasm_helpers.c
@@ -16,6 +16,11 @@
  */
 #define MAX_LOCAL_LABELS 32
 
+/* Local labels are encoded as 'L42', this requires 4 bytes of storage:
+ * 3 characters + zero byte
+ */
+#define LOCAL_LABEL_LEN 4
+
 static bool llvm_initialized;
 
 struct local_labels {
@@ -23,7 +28,7 @@ struct local_labels {
 	__u32 prog_len;
 	__u32 cnt;
 	__u32 pcs[MAX_LOCAL_LABELS];
-	char names[MAX_LOCAL_LABELS][4];
+	char names[MAX_LOCAL_LABELS][LOCAL_LABEL_LEN];
 };
 
 static const char *lookup_symbol(void *data, uint64_t ref_value, uint64_t *ref_type,
@@ -118,8 +123,14 @@ static int disasm_one_func(FILE *text_out, uint8_t *image, __u32 len)
 	}
 	qsort(labels.pcs, labels.cnt, sizeof(*labels.pcs), cmp_u32);
 	for (i = 0; i < labels.cnt; ++i)
-		/* use (i % 100) to avoid format truncation warning */
-		snprintf(labels.names[i], sizeof(labels.names[i]), "L%d", i % 100);
+		/* gcc is unable to infer upper bound for labels.cnt and assumes
+		 * it to be U32_MAX. U32_MAX takes 10 decimal digits.
+		 * snprintf below prints into labels.names[*],
+		 * which has space only for two digits and a letter.
+		 * To avoid truncation warning use (i % MAX_LOCAL_LABELS),
+		 * which informs gcc about printed value upper bound.
+		 */
+		snprintf(labels.names[i], sizeof(labels.names[i]), "L%d", i % MAX_LOCAL_LABELS);
 
 	/* now print with labels */
 	labels.print_phase = true;
-- 
2.46.0

Re: [PATCH bpf-next 0/3] follow up for __jited test tag

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Fri, 23 Aug 2024 01:06:41 -0700 you wrote:
> This patch-set is a collection of follow-ups for
> "__jited test tag to check disassembly after jit" series (see [1]).
> 
> First patch is most important:
> as it turns out, I broke all test_loader based tests for s390 CI.
> E.g. see log [2] for s390 execution of test_progs,
> note all 'verivier_*' tests being skipped.
> This happens because of incorrect handling of corner case when
> get_current_arch() does not know which architecture to return.
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/3] selftests/bpf: test_loader.c:get_current_arch() should not return 0
    https://git.kernel.org/bpf/bpf-next/c/ec1f77f6557b
  - [bpf-next,2/3] selftests/bpf: match both retq/rethunk in verifier_tailcall_jit
    https://git.kernel.org/bpf/bpf-next/c/c52a1e6eb74f
  - [bpf-next,3/3] selftests/bpf: #define LOCAL_LABEL_LEN for jit_disasm_helpers.c
    https://git.kernel.org/bpf/bpf-next/c/21a56fc503fa

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html