[PATCH bpf-next 0/4] __jited_x86 test tag to check x86 assembly after jit

[PATCH bpf-next 0/4] __jited_x86 test tag to check x86 assembly after jit

[Eduard Zingerman]

Some of the logic in the BPF jits might be non-trivial.
It might be useful to allow testing this logic by comparing
generated native code with expected code template.
This patch set adds a macro __jit_x86() that could be used for
test_loader based tests in a following manner:

    __success
    __jit_x86("endbr64")
    __jit_x86("nopl	(%rax,%rax)")
    __jit_x86("xorq	%rax, %rax")
    __jit_x86("pushq %rbp")
    ...
    SEC("tc")
    __naked int main(void) { ... }

The last patch in a set adds a test for jit code generated for tail
calls handling to demonstrate the feature.

The feature uses LLVM libraries to do the disassembly.
At selftests compilation time Makefile detects if these libraries are
available. When libraries are not available tests using __jit_x86()
are skipped. 
Current CI environment does not include llvm development libraries,
but changes to add these are trivial.

This was previously discussed here:
https://lore.kernel.org/bpf/20240718205158.3651529-1-yonghong.song@linux.dev/

Eduard Zingerman (4):
  selftests/bpf: less spam in the log for message matching
  selftests/bpf: utility function to get program disassembly after jit
  selftests/bpf: __jited_x86 test tag to check x86 assembly after jit
  selftests/bpf: validate jit behaviour for tail calls

 tools/testing/selftests/bpf/.gitignore        |   1 +
 tools/testing/selftests/bpf/Makefile          |  51 +++-
 .../selftests/bpf/jit_disasm_helpers.c        | 228 ++++++++++++++++++
 .../selftests/bpf/jit_disasm_helpers.h        |  10 +
 .../selftests/bpf/prog_tests/verifier.c       |   2 +
 tools/testing/selftests/bpf/progs/bpf_misc.h  |   2 +
 .../bpf/progs/verifier_tailcall_jit.c         | 103 ++++++++
 tools/testing/selftests/bpf/test_loader.c     | 159 ++++++++----
 8 files changed, 507 insertions(+), 49 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/jit_disasm_helpers.c
 create mode 100644 tools/testing/selftests/bpf/jit_disasm_helpers.h
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c

-- 
2.45.2

[PATCH bpf-next 1/4] selftests/bpf: less spam in the log for message matching

[Eduard Zingerman]

When running test_loader based tests in the verbose mode each matched
message leaves a trace in the stderr, e.g.:

    ./test_progs -vvv -t ...
    validate_msgs:PASS:expect_msg 0 nsec
    validate_msgs:PASS:expect_msg 0 nsec
    validate_msgs:PASS:expect_msg 0 nsec
    validate_msgs:PASS:expect_msg 0 nsec
    validate_msgs:PASS:expect_msg 0 nsec

This is not very helpful when debugging such tests and clobbers the
log a lot.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/testing/selftests/bpf/test_loader.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 12b0c41e8d64..1b1290e090e7 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -531,7 +531,8 @@ static void validate_msgs(char *log_buf, struct expected_msgs *msgs,
 			}
 		}
 
-		if (!ASSERT_OK_PTR(match, "expect_msg")) {
+		if (!match) {
+			PRINT_FAIL("expect_msg\n");
 			if (env.verbosity == VERBOSE_NONE)
 				emit_fn(log_buf, true /*force*/);
 			for (j = 0; j <= i; j++) {
-- 
2.45.2

[PATCH bpf-next 2/4] selftests/bpf: utility function to get program disassembly after jit

[Eduard Zingerman]

    int get_jited_program_text(int fd, char *text, size_t text_sz)

Loads and disassembles jited instructions for program pointed to by fd.
Much like 'bpftool prog dump jited ...'.

The code and makefile changes are inspired by jit_disasm.c from bpftool.
Use llvm libraries to disassemble BPF program instead of libbfd to avoid
issues with disassembly output stability pointed out in [1].

Selftests makefile uses Makefile.feature to detect if LLVM libraries
are available. If that is not the case selftests build proceeds but
the function returns -ENOTSUP at runtime.

[1] commit eb9d1acf634b ("bpftool: Add LLVM as default library for disassembling JIT-ed programs")

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/testing/selftests/bpf/.gitignore        |   1 +
 tools/testing/selftests/bpf/Makefile          |  51 +++-
 .../selftests/bpf/jit_disasm_helpers.c        | 228 ++++++++++++++++++
 .../selftests/bpf/jit_disasm_helpers.h        |  10 +
 4 files changed, 288 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/jit_disasm_helpers.c
 create mode 100644 tools/testing/selftests/bpf/jit_disasm_helpers.h

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index 8f14d8faeb0b..7de4108771a0 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -8,6 +8,7 @@ test_lru_map
 test_lpm_map
 test_tag
 FEATURE-DUMP.libbpf
+FEATURE-DUMP.selftests
 fixdep
 /test_progs
 /test_progs-no_alu32
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index f54185e96a95..b1a52739d9e7 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -33,6 +33,13 @@ OPT_FLAGS	?= $(if $(RELEASE),-O2,-O0)
 LIBELF_CFLAGS	:= $(shell $(PKG_CONFIG) libelf --cflags 2>/dev/null)
 LIBELF_LIBS	:= $(shell $(PKG_CONFIG) libelf --libs 2>/dev/null || echo -lelf)
 
+ifeq ($(srctree),)
+srctree := $(patsubst %/,%,$(dir $(CURDIR)))
+srctree := $(patsubst %/,%,$(dir $(srctree)))
+srctree := $(patsubst %/,%,$(dir $(srctree)))
+srctree := $(patsubst %/,%,$(dir $(srctree)))
+endif
+
 CFLAGS += -g $(OPT_FLAGS) -rdynamic					\
 	  -Wall -Werror -fno-omit-frame-pointer				\
 	  $(GENFLAGS) $(SAN_CFLAGS) $(LIBELF_CFLAGS)			\
@@ -55,6 +62,11 @@ progs/test_sk_lookup.c-CFLAGS := -fno-strict-aliasing
 progs/timer_crash.c-CFLAGS := -fno-strict-aliasing
 progs/test_global_func9.c-CFLAGS := -fno-strict-aliasing
 
+# Some utility functions use LLVM libraries
+jit_disasm_helpers.c-CFLAGS = $(LLVM_CFLAGS)
+test_progs-LDLIBS = $(LLVM_LDLIBS)
+test_progs-LDFLAGS = $(LLVM_LDFLAGS)
+
 ifneq ($(LLVM),)
 # Silence some warnings when compiled with clang
 CFLAGS += -Wno-unused-command-line-argument
@@ -164,6 +176,31 @@ endef
 
 include ../lib.mk
 
+NON_CHECK_FEAT_TARGETS := clean docs-clean
+CHECK_FEAT := $(filter-out $(NON_CHECK_FEAT_TARGETS),$(or $(MAKECMDGOALS), "none"))
+ifneq ($(CHECK_FEAT),)
+FEATURE_USER := .selftests
+FEATURE_TESTS := llvm
+FEATURE_DISPLAY := $(FEATURE_TESTS)
+
+# Makefile.feature expects OUTPUT to end with a slash
+$(let OUTPUT,$(OUTPUT)/,\
+	$(eval include ../../../build/Makefile.feature))
+endif
+
+ifeq ($(feature-llvm),1)
+  LLVM_CFLAGS  += -DHAVE_LLVM_SUPPORT
+  LLVM_CONFIG_LIB_COMPONENTS := mcdisassembler all-targets
+  # both llvm-config and lib.mk add -D_GNU_SOURCE, which ends up as conflict
+  LLVM_CFLAGS  += $(filter-out -D_GNU_SOURCE,$(shell $(LLVM_CONFIG) --cflags))
+  LLVM_LDLIBS  += $(shell $(LLVM_CONFIG) --libs $(LLVM_CONFIG_LIB_COMPONENTS))
+  ifeq ($(shell $(LLVM_CONFIG) --shared-mode),static)
+    LLVM_LDLIBS += $(shell $(LLVM_CONFIG) --system-libs $(LLVM_CONFIG_LIB_COMPONENTS))
+    LLVM_LDLIBS += -lstdc++
+  endif
+  LLVM_LDFLAGS += $(shell $(LLVM_CONFIG) --ldflags)
+endif
+
 SCRATCH_DIR := $(OUTPUT)/tools
 BUILD_DIR := $(SCRATCH_DIR)/build
 INCLUDE_DIR := $(SCRATCH_DIR)/include
@@ -488,6 +525,7 @@ define DEFINE_TEST_RUNNER
 
 TRUNNER_OUTPUT := $(OUTPUT)$(if $2,/)$2
 TRUNNER_BINARY := $1$(if $2,-)$2
+TRUNNER_BASE_NAME := $1
 TRUNNER_TEST_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.test.o,	\
 				 $$(notdir $$(wildcard $(TRUNNER_TESTS_DIR)/*.c)))
 TRUNNER_EXTRA_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o,		\
@@ -611,6 +649,10 @@ ifeq ($(filter clean docs-clean,$(MAKECMDGOALS)),)
 include $(wildcard $(TRUNNER_TEST_OBJS:.o=.d))
 endif
 
+# add per extra obj CFGLAGS definitions
+$(foreach N,$(patsubst $(TRUNNER_OUTPUT)/%.o,%,$(TRUNNER_EXTRA_OBJS)),	\
+	$(eval $(TRUNNER_OUTPUT)/$(N).o: CFLAGS += $($(N).c-CFLAGS)))
+
 $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o:				\
 		       %.c						\
 		       $(TRUNNER_EXTRA_HDRS)				\
@@ -627,6 +669,9 @@ ifneq ($2:$(OUTPUT),:$(shell pwd))
 	$(Q)rsync -aq $$^ $(TRUNNER_OUTPUT)/
 endif
 
+$(OUTPUT)/$(TRUNNER_BINARY): LDLIBS += $$($(TRUNNER_BASE_NAME)-LDLIBS)
+$(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $$($(TRUNNER_BASE_NAME)-LDFLAGS)
+
 # some X.test.o files have runtime dependencies on Y.bpf.o files
 $(OUTPUT)/$(TRUNNER_BINARY): | $(TRUNNER_BPF_OBJS)
 
@@ -636,7 +681,7 @@ $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)			\
 			     $(TRUNNER_BPFTOOL)				\
 			     | $(TRUNNER_BINARY)-extras
 	$$(call msg,BINARY,,$$@)
-	$(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) -o $$@
+	$(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) $$(LDFLAGS) -o $$@
 	$(Q)$(RESOLVE_BTFIDS) --btf $(TRUNNER_OUTPUT)/btf_data.bpf.o $$@
 	$(Q)ln -sf $(if $2,..,.)/tools/build/bpftool/$(USE_BOOTSTRAP)bpftool \
 		   $(OUTPUT)/$(if $2,$2/)bpftool
@@ -655,6 +700,7 @@ TRUNNER_EXTRA_SOURCES := test_progs.c		\
 			 cap_helpers.c		\
 			 unpriv_helpers.c 	\
 			 netlink_helpers.c	\
+			 jit_disasm_helpers.c	\
 			 test_loader.c		\
 			 xsk.c			\
 			 disasm.c		\
@@ -797,7 +843,8 @@ EXTRA_CLEAN := $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)			\
 	$(addprefix $(OUTPUT)/,*.o *.d *.skel.h *.lskel.h *.subskel.h	\
 			       no_alu32 cpuv4 bpf_gcc bpf_testmod.ko	\
 			       bpf_test_no_cfi.ko			\
-			       liburandom_read.so)
+			       liburandom_read.so)			\
+	$(OUTPUT)/FEATURE-DUMP.selftests
 
 .PHONY: docs docs-clean
 
diff --git a/tools/testing/selftests/bpf/jit_disasm_helpers.c b/tools/testing/selftests/bpf/jit_disasm_helpers.c
new file mode 100644
index 000000000000..ae704f7c5ee7
--- /dev/null
+++ b/tools/testing/selftests/bpf/jit_disasm_helpers.c
@@ -0,0 +1,228 @@
+// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+#include <test_progs.h>
+
+#ifdef HAVE_LLVM_SUPPORT
+
+#include <llvm-c/Core.h>
+#include <llvm-c/Disassembler.h>
+#include <llvm-c/Target.h>
+#include <llvm-c/TargetMachine.h>
+
+#define MAX_LOCAL_LABELS 32
+
+static bool llvm_initialized;
+
+struct local_labels {
+	bool print_phase;
+	__u32 prog_len;
+	__u32 cnt;
+	__u32 pcs[MAX_LOCAL_LABELS];
+	char names[MAX_LOCAL_LABELS][4];
+};
+
+/* Depending on labels->print_phase:
+ * - if false: record save jump labels within the program in labels->pcs;
+ * - if true: if ref_value is in labels->pcs, return corresponding labels->name.
+ */
+static const char *lookup_symbol(void *data, uint64_t ref_value, uint64_t *ref_type,
+				 uint64_t ref_pc, const char **ref_name)
+{
+	struct local_labels *labels = data;
+	uint64_t type = *ref_type;
+	int i;
+
+	*ref_type = LLVMDisassembler_ReferenceType_InOut_None;
+	*ref_name = NULL;
+	if (type != LLVMDisassembler_ReferenceType_In_Branch)
+		return NULL;
+	for (i = 0; i < labels->cnt; ++i)
+		if (labels->pcs[i] == ref_value)
+			return labels->names[i];
+	if (!labels->print_phase && labels->cnt < MAX_LOCAL_LABELS &&
+	    ref_value < labels->prog_len)
+		labels->pcs[labels->cnt++] = ref_value;
+	return NULL;
+}
+
+static int disasm_insn(LLVMDisasmContextRef ctx, uint8_t *image, __u32 len, __u32 pc,
+		       char *buf, __u32 buf_sz)
+{
+	int i, cnt;
+
+	cnt = LLVMDisasmInstruction(ctx, image + pc, len - pc, pc,
+				    buf, buf_sz);
+	if (cnt > 0)
+		return cnt;
+	PRINT_FAIL("Can't disasm instruction at offset %d:", pc);
+	for (i = 0; i < 16 && pc + i < len; ++i)
+		printf(" %02x", image[pc + i]);
+	printf("\n");
+	return -EINVAL;
+}
+
+static int cmp_u32(const void *_a, const void *_b)
+{
+	__u32 a = *(__u32 *)_a;
+	__u32 b = *(__u32 *)_b;
+
+	if (a < b)
+		return -1;
+	if (a > b)
+		return 1;
+	return 0;
+}
+
+static int disasm_one_func(FILE *text_out, uint8_t *image, __u32 len)
+{
+	char *label, *colon, *triple = NULL;
+	LLVMDisasmContextRef ctx = NULL;
+	struct local_labels labels = {};
+	__u32 *label_pc, pc;
+	int i, cnt, err = 0;
+	char buf[256];
+
+	triple = LLVMGetDefaultTargetTriple();
+	ctx = LLVMCreateDisasm(triple, &labels, 0, NULL, lookup_symbol);
+	if (!ASSERT_OK_PTR(ctx, "LLVMCreateDisasm")) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	cnt = LLVMSetDisasmOptions(ctx, LLVMDisassembler_Option_PrintImmHex);
+	if (!ASSERT_EQ(cnt, 1, "LLVMSetDisasmOptions")) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	/* discover labels */
+	labels.prog_len = len;
+	pc = 0;
+	while (pc < len) {
+		cnt = disasm_insn(ctx, image, len, pc, buf, 1);
+		if (cnt < 0) {
+			err = cnt;
+			goto out;
+		}
+		pc += cnt;
+	}
+	qsort(labels.pcs, labels.cnt, sizeof(*labels.pcs), cmp_u32);
+	/* GCC can't figure max bound for i and thus reports possible truncation */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wformat-truncation"
+	for (i = 0; i < labels.cnt; ++i)
+		snprintf(labels.names[i], sizeof(labels.names[i]), "L%d", i);
+#pragma GCC diagnostic pop
+
+	/* now print with labels */
+	labels.print_phase = true;
+	pc = 0;
+	while (pc < len) {
+		cnt = disasm_insn(ctx, image, len, pc, buf, sizeof(buf));
+		if (cnt < 0) {
+			err = cnt;
+			goto out;
+		}
+		label_pc = bsearch(&pc, labels.pcs, labels.cnt, sizeof(*labels.pcs), cmp_u32);
+		label = "";
+		colon = "";
+		if (label_pc) {
+			label = labels.names[label_pc - labels.pcs];
+			colon = ":";
+		}
+		fprintf(text_out, "%x:\t", pc);
+		for (i = 0; i < cnt; ++i)
+			fprintf(text_out, "%02x ", image[pc + i]);
+		for (i = cnt * 3; i < 12 * 3; ++i)
+			fputc(' ', text_out);
+		fprintf(text_out, "%s%s%s\n", label, colon, buf);
+		pc += cnt;
+	}
+
+out:
+	if (triple)
+		LLVMDisposeMessage(triple);
+	if (ctx)
+		LLVMDisasmDispose(ctx);
+	return err;
+}
+
+int get_jited_program_text(int fd, char *text, size_t text_sz)
+{
+	struct bpf_prog_info info = {};
+	__u32 info_len = sizeof(info);
+	__u32 jited_funcs, len, pc;
+	__u32 *func_lens = NULL;
+	FILE *text_out = NULL;
+	uint8_t *image = NULL;
+	int i, err = 0;
+
+	if (!llvm_initialized) {
+		LLVMInitializeAllTargetInfos();
+		LLVMInitializeAllTargetMCs();
+		LLVMInitializeAllDisassemblers();
+		llvm_initialized = 1;
+	}
+
+	text_out = fmemopen(text, text_sz, "w");
+	if (!ASSERT_OK_PTR(text_out, "open_memstream")) {
+		err = -errno;
+		goto out;
+	}
+
+	/* first call is to find out jited program len */
+	err = bpf_prog_get_info_by_fd(fd, &info, &info_len);
+	if (!ASSERT_OK(err, "bpf_prog_get_info_by_fd #1"))
+		goto out;
+
+	len = info.jited_prog_len;
+	image = malloc(len);
+	if (!ASSERT_OK_PTR(image, "malloc(info.jited_prog_len)")) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	jited_funcs = info.nr_jited_func_lens;
+	func_lens = malloc(jited_funcs * sizeof(__u32));
+	if (!ASSERT_OK_PTR(func_lens, "malloc(info.nr_jited_func_lens)")) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	memset(&info, 0, sizeof(info));
+	info.jited_prog_insns = (__u64)image;
+	info.jited_prog_len = len;
+	info.jited_func_lens = (__u64)func_lens;
+	info.nr_jited_func_lens = jited_funcs;
+	err = bpf_prog_get_info_by_fd(fd, &info, &info_len);
+	if (!ASSERT_OK(err, "bpf_prog_get_info_by_fd #2"))
+		goto out;
+
+	for (pc = 0, i = 0; i < jited_funcs; ++i) {
+		fprintf(text_out, "func #%d:\n", i);
+		disasm_one_func(text_out, image + pc, func_lens[i]);
+		fprintf(text_out, "\n");
+		pc += func_lens[i];
+	}
+
+out:
+	if (text_out)
+		fclose(text_out);
+	if (image)
+		free(image);
+	if (func_lens)
+		free(func_lens);
+	return err;
+}
+
+#else /* HAVE_LLVM_SUPPORT */
+
+int get_jited_program_text(int fd, char *text, size_t text_sz)
+{
+	if (env.verbosity >= VERBOSE_VERY)
+		printf("compiled w/o llvm development libraries, can't dis-assembly binary code");
+	return -ENOTSUP;
+}
+
+#endif /* HAVE_LLVM_SUPPORT */
diff --git a/tools/testing/selftests/bpf/jit_disasm_helpers.h b/tools/testing/selftests/bpf/jit_disasm_helpers.h
new file mode 100644
index 000000000000..e6924fd65ecf
--- /dev/null
+++ b/tools/testing/selftests/bpf/jit_disasm_helpers.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
+
+#ifndef __JIT_DISASM_HELPERS_H
+#define __JIT_DISASM_HELPERS_H
+
+#include <stddef.h>
+
+int get_jited_program_text(int fd, char *text, size_t text_sz);
+
+#endif /* __JIT_DISASM_HELPERS_H */
-- 
2.45.2

Re: [PATCH bpf-next 2/4] selftests/bpf: utility function to get program disassembly after jit

[Yonghong Song]

On 8/8/24 6:05 PM, Eduard Zingerman wrote:
>      int get_jited_program_text(int fd, char *text, size_t text_sz)
>
> Loads and disassembles jited instructions for program pointed to by fd.
> Much like 'bpftool prog dump jited ...'.
>
> The code and makefile changes are inspired by jit_disasm.c from bpftool.
> Use llvm libraries to disassemble BPF program instead of libbfd to avoid
> issues with disassembly output stability pointed out in [1].
>
> Selftests makefile uses Makefile.feature to detect if LLVM libraries
> are available. If that is not the case selftests build proceeds but
> the function returns -ENOTSUP at runtime.
>
> [1] commit eb9d1acf634b ("bpftool: Add LLVM as default library for disassembling JIT-ed programs")
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>   tools/testing/selftests/bpf/.gitignore        |   1 +
>   tools/testing/selftests/bpf/Makefile          |  51 +++-
>   .../selftests/bpf/jit_disasm_helpers.c        | 228 ++++++++++++++++++
>   .../selftests/bpf/jit_disasm_helpers.h        |  10 +
>   4 files changed, 288 insertions(+), 2 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/jit_disasm_helpers.c
>   create mode 100644 tools/testing/selftests/bpf/jit_disasm_helpers.h
>
[...]
> +static int disasm_one_func(FILE *text_out, uint8_t *image, __u32 len)
> +{
> +	char *label, *colon, *triple = NULL;
> +	LLVMDisasmContextRef ctx = NULL;
> +	struct local_labels labels = {};
> +	__u32 *label_pc, pc;
> +	int i, cnt, err = 0;
> +	char buf[256];
> +
> +	triple = LLVMGetDefaultTargetTriple();
> +	ctx = LLVMCreateDisasm(triple, &labels, 0, NULL, lookup_symbol);
> +	if (!ASSERT_OK_PTR(ctx, "LLVMCreateDisasm")) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	cnt = LLVMSetDisasmOptions(ctx, LLVMDisassembler_Option_PrintImmHex);
> +	if (!ASSERT_EQ(cnt, 1, "LLVMSetDisasmOptions")) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* discover labels */
> +	labels.prog_len = len;
> +	pc = 0;
> +	while (pc < len) {
> +		cnt = disasm_insn(ctx, image, len, pc, buf, 1);
> +		if (cnt < 0) {
> +			err = cnt;
> +			goto out;
> +		}
> +		pc += cnt;
> +	}
> +	qsort(labels.pcs, labels.cnt, sizeof(*labels.pcs), cmp_u32);
> +	/* GCC can't figure max bound for i and thus reports possible truncation */
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wformat-truncation"
> +	for (i = 0; i < labels.cnt; ++i)
> +		snprintf(labels.names[i], sizeof(labels.names[i]), "L%d", i);
> +#pragma GCC diagnostic pop

"-Wformat-truncation" is only available for llvm >= 18. One of my build with llvm15
has the following warning/error:

jit_disasm_helpers.c:113:32: error: unknown warning group '-Wformat-truncation', ignored [-Werror,-Wunknown-warning-option]
#pragma GCC diagnostic ignored "-Wformat-truncation"

Maybe you want to guard with proper clang version?
Not sure on gcc side when "-Wformat-truncation" is supported.

> +
> +	/* now print with labels */
> +	labels.print_phase = true;
> +	pc = 0;
> +	while (pc < len) {
> +		cnt = disasm_insn(ctx, image, len, pc, buf, sizeof(buf));
> +		if (cnt < 0) {
> +			err = cnt;
> +			goto out;
> +		}
> +		label_pc = bsearch(&pc, labels.pcs, labels.cnt, sizeof(*labels.pcs), cmp_u32);
> +		label = "";
> +		colon = "";
> +		if (label_pc) {
> +			label = labels.names[label_pc - labels.pcs];
> +			colon = ":";
> +		}
> +		fprintf(text_out, "%x:\t", pc);
> +		for (i = 0; i < cnt; ++i)
> +			fprintf(text_out, "%02x ", image[pc + i]);
> +		for (i = cnt * 3; i < 12 * 3; ++i)
> +			fputc(' ', text_out);
> +		fprintf(text_out, "%s%s%s\n", label, colon, buf);
> +		pc += cnt;
> +	}
> +
> +out:
> +	if (triple)
> +		LLVMDisposeMessage(triple);
> +	if (ctx)
> +		LLVMDisasmDispose(ctx);
> +	return err;
> +}
> +
[...]

Re: [PATCH bpf-next 2/4] selftests/bpf: utility function to get program disassembly after jit

[Eduard Zingerman]

On Tue, 2024-08-13 at 09:05 -0700, Yonghong Song wrote:

[...]

> > +	/* GCC can't figure max bound for i and thus reports possible truncation */
> > +#pragma GCC diagnostic push
> > +#pragma GCC diagnostic ignored "-Wformat-truncation"
> > +	for (i = 0; i < labels.cnt; ++i)
> > +		snprintf(labels.names[i], sizeof(labels.names[i]), "L%d", i);
> > +#pragma GCC diagnostic pop
> 
> "-Wformat-truncation" is only available for llvm >= 18. One of my build with llvm15
> has the following warning/error:
> 
> jit_disasm_helpers.c:113:32: error: unknown warning group '-Wformat-truncation', ignored [-Werror,-Wunknown-warning-option]
> #pragma GCC diagnostic ignored "-Wformat-truncation"
> 
> Maybe you want to guard with proper clang version?
> Not sure on gcc side when "-Wformat-truncation" is supported.

Thank you for catching this, I think I'll fix it as below in the v2.
Will wait for additional comments before submitting v2.

---

--- a/tools/testing/selftests/bpf/jit_disasm_helpers.c
+++ b/tools/testing/selftests/bpf/jit_disasm_helpers.c
@@ -108,12 +108,9 @@ static int disasm_one_func(FILE *text_out, uint8_t *image, __u32 len)
                pc += cnt;
        }
        qsort(labels.pcs, labels.cnt, sizeof(*labels.pcs), cmp_u32);
-       /* GCC can't figure max bound for i and thus reports possible truncation */
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wformat-truncation"
        for (i = 0; i < labels.cnt; ++i)
-               snprintf(labels.names[i], sizeof(labels.names[i]), "L%d", i);
-#pragma GCC diagnostic pop
+               /* use (i % 100) to avoid format truncation warning */
+               snprintf(labels.names[i], sizeof(labels.names[i]), "L%d", i % 100);
 
        /* now print with labels */
        labels.print_phase = true;

Re: [PATCH bpf-next 2/4] selftests/bpf: utility function to get program disassembly after jit

[Yonghong Song]

On 8/8/24 6:05 PM, Eduard Zingerman wrote:
>      int get_jited_program_text(int fd, char *text, size_t text_sz)
>
You need to give some context before the above function signature.

> Loads and disassembles jited instructions for program pointed to by fd.
> Much like 'bpftool prog dump jited ...'.
>
> The code and makefile changes are inspired by jit_disasm.c from bpftool.
> Use llvm libraries to disassemble BPF program instead of libbfd to avoid
> issues with disassembly output stability pointed out in [1].
>
> Selftests makefile uses Makefile.feature to detect if LLVM libraries
> are available. If that is not the case selftests build proceeds but
> the function returns -ENOTSUP at runtime.
>
> [1] commit eb9d1acf634b ("bpftool: Add LLVM as default library for disassembling JIT-ed programs")
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>

LGTM except a few nits below.

Acked-by: Yonghong Song <yonghong.song@linux.dev>

> ---
>   tools/testing/selftests/bpf/.gitignore        |   1 +
>   tools/testing/selftests/bpf/Makefile          |  51 +++-
>   .../selftests/bpf/jit_disasm_helpers.c        | 228 ++++++++++++++++++
>   .../selftests/bpf/jit_disasm_helpers.h        |  10 +
>   4 files changed, 288 insertions(+), 2 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/jit_disasm_helpers.c
>   create mode 100644 tools/testing/selftests/bpf/jit_disasm_helpers.h
>
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index 8f14d8faeb0b..7de4108771a0 100644
> --- a/tools/testing/selftests/bpf/.gitignore
> +++ b/tools/testing/selftests/bpf/.gitignore
> @@ -8,6 +8,7 @@ test_lru_map
>   test_lpm_map
>   test_tag
>   FEATURE-DUMP.libbpf
> +FEATURE-DUMP.selftests
>   fixdep
>   /test_progs
>   /test_progs-no_alu32
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index f54185e96a95..b1a52739d9e7 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -33,6 +33,13 @@ OPT_FLAGS	?= $(if $(RELEASE),-O2,-O0)
>   LIBELF_CFLAGS	:= $(shell $(PKG_CONFIG) libelf --cflags 2>/dev/null)
>   LIBELF_LIBS	:= $(shell $(PKG_CONFIG) libelf --libs 2>/dev/null || echo -lelf)
>   
> +ifeq ($(srctree),)
> +srctree := $(patsubst %/,%,$(dir $(CURDIR)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +endif
> +
>   CFLAGS += -g $(OPT_FLAGS) -rdynamic					\
>   	  -Wall -Werror -fno-omit-frame-pointer				\
>   	  $(GENFLAGS) $(SAN_CFLAGS) $(LIBELF_CFLAGS)			\
> @@ -55,6 +62,11 @@ progs/test_sk_lookup.c-CFLAGS := -fno-strict-aliasing
>   progs/timer_crash.c-CFLAGS := -fno-strict-aliasing
>   progs/test_global_func9.c-CFLAGS := -fno-strict-aliasing
>   
> +# Some utility functions use LLVM libraries
> +jit_disasm_helpers.c-CFLAGS = $(LLVM_CFLAGS)
> +test_progs-LDLIBS = $(LLVM_LDLIBS)
> +test_progs-LDFLAGS = $(LLVM_LDFLAGS)
> +
>   ifneq ($(LLVM),)
>   # Silence some warnings when compiled with clang
>   CFLAGS += -Wno-unused-command-line-argument
> @@ -164,6 +176,31 @@ endef
>   
>   include ../lib.mk
>   
> +NON_CHECK_FEAT_TARGETS := clean docs-clean
> +CHECK_FEAT := $(filter-out $(NON_CHECK_FEAT_TARGETS),$(or $(MAKECMDGOALS), "none"))
> +ifneq ($(CHECK_FEAT),)
> +FEATURE_USER := .selftests
> +FEATURE_TESTS := llvm
> +FEATURE_DISPLAY := $(FEATURE_TESTS)
> +
> +# Makefile.feature expects OUTPUT to end with a slash
> +$(let OUTPUT,$(OUTPUT)/,\
> +	$(eval include ../../../build/Makefile.feature))
> +endif
> +
> +ifeq ($(feature-llvm),1)
> +  LLVM_CFLAGS  += -DHAVE_LLVM_SUPPORT
> +  LLVM_CONFIG_LIB_COMPONENTS := mcdisassembler all-targets
> +  # both llvm-config and lib.mk add -D_GNU_SOURCE, which ends up as conflict
> +  LLVM_CFLAGS  += $(filter-out -D_GNU_SOURCE,$(shell $(LLVM_CONFIG) --cflags))
> +  LLVM_LDLIBS  += $(shell $(LLVM_CONFIG) --libs $(LLVM_CONFIG_LIB_COMPONENTS))
> +  ifeq ($(shell $(LLVM_CONFIG) --shared-mode),static)
> +    LLVM_LDLIBS += $(shell $(LLVM_CONFIG) --system-libs $(LLVM_CONFIG_LIB_COMPONENTS))
> +    LLVM_LDLIBS += -lstdc++
> +  endif
> +  LLVM_LDFLAGS += $(shell $(LLVM_CONFIG) --ldflags)
> +endif
> +
>   SCRATCH_DIR := $(OUTPUT)/tools
>   BUILD_DIR := $(SCRATCH_DIR)/build
>   INCLUDE_DIR := $(SCRATCH_DIR)/include
> @@ -488,6 +525,7 @@ define DEFINE_TEST_RUNNER
>   
>   TRUNNER_OUTPUT := $(OUTPUT)$(if $2,/)$2
>   TRUNNER_BINARY := $1$(if $2,-)$2
> +TRUNNER_BASE_NAME := $1
>   TRUNNER_TEST_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.test.o,	\
>   				 $$(notdir $$(wildcard $(TRUNNER_TESTS_DIR)/*.c)))
>   TRUNNER_EXTRA_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o,		\
> @@ -611,6 +649,10 @@ ifeq ($(filter clean docs-clean,$(MAKECMDGOALS)),)
>   include $(wildcard $(TRUNNER_TEST_OBJS:.o=.d))
>   endif
>   
> +# add per extra obj CFGLAGS definitions
> +$(foreach N,$(patsubst $(TRUNNER_OUTPUT)/%.o,%,$(TRUNNER_EXTRA_OBJS)),	\
> +	$(eval $(TRUNNER_OUTPUT)/$(N).o: CFLAGS += $($(N).c-CFLAGS)))
> +
>   $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o:				\
>   		       %.c						\
>   		       $(TRUNNER_EXTRA_HDRS)				\
> @@ -627,6 +669,9 @@ ifneq ($2:$(OUTPUT),:$(shell pwd))
>   	$(Q)rsync -aq $$^ $(TRUNNER_OUTPUT)/
>   endif
>   
> +$(OUTPUT)/$(TRUNNER_BINARY): LDLIBS += $$($(TRUNNER_BASE_NAME)-LDLIBS)
> +$(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $$($(TRUNNER_BASE_NAME)-LDFLAGS)
> +
>   # some X.test.o files have runtime dependencies on Y.bpf.o files
>   $(OUTPUT)/$(TRUNNER_BINARY): | $(TRUNNER_BPF_OBJS)
>   
> @@ -636,7 +681,7 @@ $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)			\
>   			     $(TRUNNER_BPFTOOL)				\
>   			     | $(TRUNNER_BINARY)-extras
>   	$$(call msg,BINARY,,$$@)
> -	$(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) -o $$@
> +	$(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) $$(LDFLAGS) -o $$@
>   	$(Q)$(RESOLVE_BTFIDS) --btf $(TRUNNER_OUTPUT)/btf_data.bpf.o $$@
>   	$(Q)ln -sf $(if $2,..,.)/tools/build/bpftool/$(USE_BOOTSTRAP)bpftool \
>   		   $(OUTPUT)/$(if $2,$2/)bpftool
> @@ -655,6 +700,7 @@ TRUNNER_EXTRA_SOURCES := test_progs.c		\
>   			 cap_helpers.c		\
>   			 unpriv_helpers.c 	\
>   			 netlink_helpers.c	\
> +			 jit_disasm_helpers.c	\
>   			 test_loader.c		\
>   			 xsk.c			\
>   			 disasm.c		\
> @@ -797,7 +843,8 @@ EXTRA_CLEAN := $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)			\
>   	$(addprefix $(OUTPUT)/,*.o *.d *.skel.h *.lskel.h *.subskel.h	\
>   			       no_alu32 cpuv4 bpf_gcc bpf_testmod.ko	\
>   			       bpf_test_no_cfi.ko			\
> -			       liburandom_read.so)
> +			       liburandom_read.so)			\
> +	$(OUTPUT)/FEATURE-DUMP.selftests
>   
>   .PHONY: docs docs-clean
>   
> diff --git a/tools/testing/selftests/bpf/jit_disasm_helpers.c b/tools/testing/selftests/bpf/jit_disasm_helpers.c
> new file mode 100644
> index 000000000000..ae704f7c5ee7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/jit_disasm_helpers.c
> @@ -0,0 +1,228 @@
> +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> +#include <bpf/bpf.h>
> +#include <bpf/libbpf.h>
> +#include <test_progs.h>
> +
> +#ifdef HAVE_LLVM_SUPPORT
> +
> +#include <llvm-c/Core.h>
> +#include <llvm-c/Disassembler.h>
> +#include <llvm-c/Target.h>
> +#include <llvm-c/TargetMachine.h>
> +
> +#define MAX_LOCAL_LABELS 32

Some bpf progs, e.g., pyperf600.bpf.o has thousands of lables.
But since this file is for selftest and we expect the test
case will be simpler, so the above number '32' should be okay.
But it would be good to add a comment for this.

> +
> +static bool llvm_initialized;
> +
> +struct local_labels {
> +	bool print_phase;
> +	__u32 prog_len;
> +	__u32 cnt;
> +	__u32 pcs[MAX_LOCAL_LABELS];
> +	char names[MAX_LOCAL_LABELS][4];
> +};
> +
> +/* Depending on labels->print_phase:
> + * - if false: record save jump labels within the program in labels->pcs;
> + * - if true: if ref_value is in labels->pcs, return corresponding labels->name
> + */
> +static const char *lookup_symbol(void *data, uint64_t ref_value, uint64_t *ref_type,
> +				 uint64_t ref_pc, const char **ref_name)
> +{
> +	struct local_labels *labels = data;
> +	uint64_t type = *ref_type;
> +	int i;
> +
> +	*ref_type = LLVMDisassembler_ReferenceType_InOut_None;
> +	*ref_name = NULL;
> +	if (type != LLVMDisassembler_ReferenceType_In_Branch)
> +		return NULL;
> +	for (i = 0; i < labels->cnt; ++i)
> +		if (labels->pcs[i] == ref_value)
> +			return labels->names[i];
> +	if (!labels->print_phase && labels->cnt < MAX_LOCAL_LABELS &&
> +	    ref_value < labels->prog_len)
> +		labels->pcs[labels->cnt++] = ref_value;

Maybe the following easy to understand?

	if (labels->print_phase) {
		for (i = 0; i < labels->cnt; ++i)
			if (labels->pcs[i] == ref_value)
				return labels->names[i];

	} else {
		if (labels->cnt < MAX_LOCAL_LABELS && ref_value < labels->prog_len)
			labels->pcs[labels->cnt++] = ref_value;
	}

> +	return NULL;
> +}
> +
> +static int disasm_insn(LLVMDisasmContextRef ctx, uint8_t *image, __u32 len, __u32 pc,
> +		       char *buf, __u32 buf_sz)
> +{
> +	int i, cnt;
> +
> +	cnt = LLVMDisasmInstruction(ctx, image + pc, len - pc, pc,
> +				    buf, buf_sz);
> +	if (cnt > 0)
> +		return cnt;
> +	PRINT_FAIL("Can't disasm instruction at offset %d:", pc);
> +	for (i = 0; i < 16 && pc + i < len; ++i)
> +		printf(" %02x", image[pc + i]);
> +	printf("\n");
> +	return -EINVAL;
> +}
> +
> +static int cmp_u32(const void *_a, const void *_b)
> +{
> +	__u32 a = *(__u32 *)_a;
> +	__u32 b = *(__u32 *)_b;
> +
> +	if (a < b)
> +		return -1;
> +	if (a > b)
> +		return 1;
> +	return 0;
> +}
> +
> +static int disasm_one_func(FILE *text_out, uint8_t *image, __u32 len)
> +{
> +	char *label, *colon, *triple = NULL;
> +	LLVMDisasmContextRef ctx = NULL;
> +	struct local_labels labels = {};
> +	__u32 *label_pc, pc;
> +	int i, cnt, err = 0;
> +	char buf[256];

buf[16]?

> +
> +	triple = LLVMGetDefaultTargetTriple();
> +	ctx = LLVMCreateDisasm(triple, &labels, 0, NULL, lookup_symbol);
> +	if (!ASSERT_OK_PTR(ctx, "LLVMCreateDisasm")) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	cnt = LLVMSetDisasmOptions(ctx, LLVMDisassembler_Option_PrintImmHex);
> +	if (!ASSERT_EQ(cnt, 1, "LLVMSetDisasmOptions")) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* discover labels */
> +	labels.prog_len = len;
> +	pc = 0;
> +	while (pc < len) {
> +		cnt = disasm_insn(ctx, image, len, pc, buf, 1);
> +		if (cnt < 0) {
> +			err = cnt;
> +			goto out;
> +		}
> +		pc += cnt;
> +	}
> +	qsort(labels.pcs, labels.cnt, sizeof(*labels.pcs), cmp_u32);

[...]

Re: [PATCH bpf-next 2/4] selftests/bpf: utility function to get program disassembly after jit

[Eduard Zingerman]

On Thu, 2024-08-15 at 12:27 -0700, Yonghong Song wrote:
> On 8/8/24 6:05 PM, Eduard Zingerman wrote:
> >      int get_jited_program_text(int fd, char *text, size_t text_sz)
> > 
> You need to give some context before the above function signature.

Will do.

> > Loads and disassembles jited instructions for program pointed to by fd.
> > Much like 'bpftool prog dump jited ...'.
> > 
> > The code and makefile changes are inspired by jit_disasm.c from bpftool.
> > Use llvm libraries to disassemble BPF program instead of libbfd to avoid
> > issues with disassembly output stability pointed out in [1].
> > 
> > Selftests makefile uses Makefile.feature to detect if LLVM libraries
> > are available. If that is not the case selftests build proceeds but
> > the function returns -ENOTSUP at runtime.
> > 
> > [1] commit eb9d1acf634b ("bpftool: Add LLVM as default library for disassembling JIT-ed programs")
> > 
> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> 
> LGTM except a few nits below.
> 
> Acked-by: Yonghong Song <yonghong.song@linux.dev>

Thank you for the review.
I agree with the nits, will fix as suggested.
Will send v2 shortly.

[...]

Re: [PATCH bpf-next 2/4] selftests/bpf: utility function to get program disassembly after jit

[Andrii Nakryiko]

On Thu, Aug 8, 2024 at 6:05 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
>     int get_jited_program_text(int fd, char *text, size_t text_sz)
>
> Loads and disassembles jited instructions for program pointed to by fd.
> Much like 'bpftool prog dump jited ...'.
>
> The code and makefile changes are inspired by jit_disasm.c from bpftool.
> Use llvm libraries to disassemble BPF program instead of libbfd to avoid
> issues with disassembly output stability pointed out in [1].
>
> Selftests makefile uses Makefile.feature to detect if LLVM libraries
> are available. If that is not the case selftests build proceeds but
> the function returns -ENOTSUP at runtime.
>
> [1] commit eb9d1acf634b ("bpftool: Add LLVM as default library for disassembling JIT-ed programs")
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  tools/testing/selftests/bpf/.gitignore        |   1 +
>  tools/testing/selftests/bpf/Makefile          |  51 +++-
>  .../selftests/bpf/jit_disasm_helpers.c        | 228 ++++++++++++++++++
>  .../selftests/bpf/jit_disasm_helpers.h        |  10 +
>  4 files changed, 288 insertions(+), 2 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/jit_disasm_helpers.c
>  create mode 100644 tools/testing/selftests/bpf/jit_disasm_helpers.h
>
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index 8f14d8faeb0b..7de4108771a0 100644
> --- a/tools/testing/selftests/bpf/.gitignore
> +++ b/tools/testing/selftests/bpf/.gitignore
> @@ -8,6 +8,7 @@ test_lru_map
>  test_lpm_map
>  test_tag
>  FEATURE-DUMP.libbpf
> +FEATURE-DUMP.selftests
>  fixdep
>  /test_progs
>  /test_progs-no_alu32
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index f54185e96a95..b1a52739d9e7 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -33,6 +33,13 @@ OPT_FLAGS    ?= $(if $(RELEASE),-O2,-O0)
>  LIBELF_CFLAGS  := $(shell $(PKG_CONFIG) libelf --cflags 2>/dev/null)
>  LIBELF_LIBS    := $(shell $(PKG_CONFIG) libelf --libs 2>/dev/null || echo -lelf)
>
> +ifeq ($(srctree),)
> +srctree := $(patsubst %/,%,$(dir $(CURDIR)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +endif
> +
>  CFLAGS += -g $(OPT_FLAGS) -rdynamic                                    \
>           -Wall -Werror -fno-omit-frame-pointer                         \
>           $(GENFLAGS) $(SAN_CFLAGS) $(LIBELF_CFLAGS)                    \
> @@ -55,6 +62,11 @@ progs/test_sk_lookup.c-CFLAGS := -fno-strict-aliasing
>  progs/timer_crash.c-CFLAGS := -fno-strict-aliasing
>  progs/test_global_func9.c-CFLAGS := -fno-strict-aliasing
>
> +# Some utility functions use LLVM libraries
> +jit_disasm_helpers.c-CFLAGS = $(LLVM_CFLAGS)
> +test_progs-LDLIBS = $(LLVM_LDLIBS)
> +test_progs-LDFLAGS = $(LLVM_LDFLAGS)
> +
>  ifneq ($(LLVM),)
>  # Silence some warnings when compiled with clang
>  CFLAGS += -Wno-unused-command-line-argument
> @@ -164,6 +176,31 @@ endef
>
>  include ../lib.mk
>
> +NON_CHECK_FEAT_TARGETS := clean docs-clean
> +CHECK_FEAT := $(filter-out $(NON_CHECK_FEAT_TARGETS),$(or $(MAKECMDGOALS), "none"))
> +ifneq ($(CHECK_FEAT),)
> +FEATURE_USER := .selftests
> +FEATURE_TESTS := llvm
> +FEATURE_DISPLAY := $(FEATURE_TESTS)
> +
> +# Makefile.feature expects OUTPUT to end with a slash
> +$(let OUTPUT,$(OUTPUT)/,\
> +       $(eval include ../../../build/Makefile.feature))
> +endif
> +
> +ifeq ($(feature-llvm),1)
> +  LLVM_CFLAGS  += -DHAVE_LLVM_SUPPORT
> +  LLVM_CONFIG_LIB_COMPONENTS := mcdisassembler all-targets
> +  # both llvm-config and lib.mk add -D_GNU_SOURCE, which ends up as conflict
> +  LLVM_CFLAGS  += $(filter-out -D_GNU_SOURCE,$(shell $(LLVM_CONFIG) --cflags))
> +  LLVM_LDLIBS  += $(shell $(LLVM_CONFIG) --libs $(LLVM_CONFIG_LIB_COMPONENTS))
> +  ifeq ($(shell $(LLVM_CONFIG) --shared-mode),static)
> +    LLVM_LDLIBS += $(shell $(LLVM_CONFIG) --system-libs $(LLVM_CONFIG_LIB_COMPONENTS))
> +    LLVM_LDLIBS += -lstdc++
> +  endif
> +  LLVM_LDFLAGS += $(shell $(LLVM_CONFIG) --ldflags)
> +endif
> +
>  SCRATCH_DIR := $(OUTPUT)/tools
>  BUILD_DIR := $(SCRATCH_DIR)/build
>  INCLUDE_DIR := $(SCRATCH_DIR)/include
> @@ -488,6 +525,7 @@ define DEFINE_TEST_RUNNER
>
>  TRUNNER_OUTPUT := $(OUTPUT)$(if $2,/)$2
>  TRUNNER_BINARY := $1$(if $2,-)$2
> +TRUNNER_BASE_NAME := $1
>  TRUNNER_TEST_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.test.o,      \
>                                  $$(notdir $$(wildcard $(TRUNNER_TESTS_DIR)/*.c)))
>  TRUNNER_EXTRA_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o,          \
> @@ -611,6 +649,10 @@ ifeq ($(filter clean docs-clean,$(MAKECMDGOALS)),)
>  include $(wildcard $(TRUNNER_TEST_OBJS:.o=.d))
>  endif
>
> +# add per extra obj CFGLAGS definitions
> +$(foreach N,$(patsubst $(TRUNNER_OUTPUT)/%.o,%,$(TRUNNER_EXTRA_OBJS)), \
> +       $(eval $(TRUNNER_OUTPUT)/$(N).o: CFLAGS += $($(N).c-CFLAGS)))
> +
>  $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o:                          \
>                        %.c                                              \
>                        $(TRUNNER_EXTRA_HDRS)                            \
> @@ -627,6 +669,9 @@ ifneq ($2:$(OUTPUT),:$(shell pwd))
>         $(Q)rsync -aq $$^ $(TRUNNER_OUTPUT)/
>  endif
>
> +$(OUTPUT)/$(TRUNNER_BINARY): LDLIBS += $$($(TRUNNER_BASE_NAME)-LDLIBS)
> +$(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $$($(TRUNNER_BASE_NAME)-LDFLAGS)

is there any reason why you need to have this blah-LDFLAGS convention
and then applying that with extra pass, instead of just writing

$(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $(LLVM_LDFLAGS)

I'm not sure I understand the need for extra logical hops to do this

> +
>  # some X.test.o files have runtime dependencies on Y.bpf.o files
>  $(OUTPUT)/$(TRUNNER_BINARY): | $(TRUNNER_BPF_OBJS)
>
> @@ -636,7 +681,7 @@ $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)                   \
>                              $(TRUNNER_BPFTOOL)                         \
>                              | $(TRUNNER_BINARY)-extras
>         $$(call msg,BINARY,,$$@)
> -       $(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) -o $$@
> +       $(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) $$(LDFLAGS) -o $$@
>         $(Q)$(RESOLVE_BTFIDS) --btf $(TRUNNER_OUTPUT)/btf_data.bpf.o $$@
>         $(Q)ln -sf $(if $2,..,.)/tools/build/bpftool/$(USE_BOOTSTRAP)bpftool \
>                    $(OUTPUT)/$(if $2,$2/)bpftool
> @@ -655,6 +700,7 @@ TRUNNER_EXTRA_SOURCES := test_progs.c               \
>                          cap_helpers.c          \
>                          unpriv_helpers.c       \
>                          netlink_helpers.c      \
> +                        jit_disasm_helpers.c   \
>                          test_loader.c          \
>                          xsk.c                  \
>                          disasm.c               \
> @@ -797,7 +843,8 @@ EXTRA_CLEAN := $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)                   \
>         $(addprefix $(OUTPUT)/,*.o *.d *.skel.h *.lskel.h *.subskel.h   \
>                                no_alu32 cpuv4 bpf_gcc bpf_testmod.ko    \
>                                bpf_test_no_cfi.ko                       \
> -                              liburandom_read.so)
> +                              liburandom_read.so)                      \
> +       $(OUTPUT)/FEATURE-DUMP.selftests
>
>  .PHONY: docs docs-clean

[...]

Re: [PATCH bpf-next 2/4] selftests/bpf: utility function to get program disassembly after jit

[Eduard Zingerman]

On Thu, 2024-08-15 at 14:06 -0700, Andrii Nakryiko wrote:

[...]

> > @@ -627,6 +669,9 @@ ifneq ($2:$(OUTPUT),:$(shell pwd))
> >         $(Q)rsync -aq $$^ $(TRUNNER_OUTPUT)/
> >  endif
> > 
> > +$(OUTPUT)/$(TRUNNER_BINARY): LDLIBS += $$($(TRUNNER_BASE_NAME)-LDLIBS)
> > +$(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $$($(TRUNNER_BASE_NAME)-LDFLAGS)
> 
> is there any reason why you need to have this blah-LDFLAGS convention
> and then applying that with extra pass, instead of just writing
> 
> $(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $(LLVM_LDFLAGS)
> 
> I'm not sure I understand the need for extra logical hops to do this

No real reason, that's how it is organized in bpftool makefile,
monkey see, monkey do. Will combine to have single LDFLAGS change.

[...]

Re: [PATCH bpf-next 2/4] selftests/bpf: utility function to get program disassembly after jit

[Andrii Nakryiko]

On Thu, Aug 15, 2024 at 2:50 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2024-08-15 at 14:06 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > > @@ -627,6 +669,9 @@ ifneq ($2:$(OUTPUT),:$(shell pwd))
> > >         $(Q)rsync -aq $$^ $(TRUNNER_OUTPUT)/
> > >  endif
> > >
> > > +$(OUTPUT)/$(TRUNNER_BINARY): LDLIBS += $$($(TRUNNER_BASE_NAME)-LDLIBS)
> > > +$(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $$($(TRUNNER_BASE_NAME)-LDFLAGS)
> >
> > is there any reason why you need to have this blah-LDFLAGS convention
> > and then applying that with extra pass, instead of just writing
> >
> > $(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $(LLVM_LDFLAGS)
> >
> > I'm not sure I understand the need for extra logical hops to do this
>
> No real reason, that's how it is organized in bpftool makefile,
> monkey see, monkey do. Will combine to have single LDFLAGS change.

I think such an approach makes sense for Linux kernel where there is a
Kbuild system of conventions and you mostly don't write real Makefile
statements (just declaratively specifying a few bits here and there).
But that's not the case for BPF selftests (and not for bpftool, but
that's a separate discussion), so extra hops just make everything
harder to follow, IMO.

>
> [...]
>

Re: [PATCH bpf-next 2/4] selftests/bpf: utility function to get program disassembly after jit

[Eduard Zingerman]

On Thu, 2024-08-15 at 14:06 -0700, Andrii Nakryiko wrote:

[...]

> > @@ -627,6 +669,9 @@ ifneq ($2:$(OUTPUT),:$(shell pwd))
> >         $(Q)rsync -aq $$^ $(TRUNNER_OUTPUT)/
> >  endif
> > 
> > +$(OUTPUT)/$(TRUNNER_BINARY): LDLIBS += $$($(TRUNNER_BASE_NAME)-LDLIBS)
> > +$(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $$($(TRUNNER_BASE_NAME)-LDFLAGS)
> 
> is there any reason why you need to have this blah-LDFLAGS convention
> and then applying that with extra pass, instead of just writing
> 
> $(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $(LLVM_LDFLAGS)
> 
> I'm not sure I understand the need for extra logical hops to do this

Sorry, I missed this detail on Thursday.
The $$($(TRUNNER_BASE_NAME)-LDLIBS) thing is needed to avoid linking
LLVM dependencies for test_maps. Still, I can remove it if you think
this does not matter.

> > +
> >  # some X.test.o files have runtime dependencies on Y.bpf.o files
> >  $(OUTPUT)/$(TRUNNER_BINARY): | $(TRUNNER_BPF_OBJS)
> > 
> > @@ -636,7 +681,7 @@ $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)                   \
> >                              $(TRUNNER_BPFTOOL)                         \
> >                              | $(TRUNNER_BINARY)-extras
> >         $$(call msg,BINARY,,$$@)
> > -       $(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) -o $$@
> > +       $(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) $$(LDFLAGS) -o $$@
> >         $(Q)$(RESOLVE_BTFIDS) --btf $(TRUNNER_OUTPUT)/btf_data.bpf.o $$@
> >         $(Q)ln -sf $(if $2,..,.)/tools/build/bpftool/$(USE_BOOTSTRAP)bpftool \
> >                    $(OUTPUT)/$(if $2,$2/)bpftool

[...]

Re: [PATCH bpf-next 2/4] selftests/bpf: utility function to get program disassembly after jit

[Andrii Nakryiko]

On Mon, Aug 19, 2024 at 12:45 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2024-08-15 at 14:06 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > > @@ -627,6 +669,9 @@ ifneq ($2:$(OUTPUT),:$(shell pwd))
> > >         $(Q)rsync -aq $$^ $(TRUNNER_OUTPUT)/
> > >  endif
> > >
> > > +$(OUTPUT)/$(TRUNNER_BINARY): LDLIBS += $$($(TRUNNER_BASE_NAME)-LDLIBS)
> > > +$(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $$($(TRUNNER_BASE_NAME)-LDFLAGS)
> >
> > is there any reason why you need to have this blah-LDFLAGS convention
> > and then applying that with extra pass, instead of just writing
> >
> > $(OUTPUT)/$(TRUNNER_BINARY): LDFLAGS += $(LLVM_LDFLAGS)
> >
> > I'm not sure I understand the need for extra logical hops to do this
>
> Sorry, I missed this detail on Thursday.
> The $$($(TRUNNER_BASE_NAME)-LDLIBS) thing is needed to avoid linking
> LLVM dependencies for test_maps. Still, I can remove it if you think
> this does not matter.
>

I don't think it does, let's keep it simple.

> > > +
> > >  # some X.test.o files have runtime dependencies on Y.bpf.o files
> > >  $(OUTPUT)/$(TRUNNER_BINARY): | $(TRUNNER_BPF_OBJS)
> > >
> > > @@ -636,7 +681,7 @@ $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)                   \
> > >                              $(TRUNNER_BPFTOOL)                         \
> > >                              | $(TRUNNER_BINARY)-extras
> > >         $$(call msg,BINARY,,$$@)
> > > -       $(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) -o $$@
> > > +       $(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) $$(LDFLAGS) -o $$@
> > >         $(Q)$(RESOLVE_BTFIDS) --btf $(TRUNNER_OUTPUT)/btf_data.bpf.o $$@
> > >         $(Q)ln -sf $(if $2,..,.)/tools/build/bpftool/$(USE_BOOTSTRAP)bpftool \
> > >                    $(OUTPUT)/$(if $2,$2/)bpftool
>
> [...]
>

[PATCH bpf-next 3/4] selftests/bpf: __jited_x86 test tag to check x86 assembly after jit

[Eduard Zingerman]

Allow to verify jit behaviour by writing tests as below:

    SEC("tp")
    __jit_x86("endbr64")
    __jit_x86("movabs $0x.*,%r9")
    __jit_x86("add    %gs:0x.*,%r9")
    __jit_x86("mov    $0x1,%edi")
    __jit_x86("mov    %rdi,-0x8(%r9)")
    __jit_x86("mov    -0x8(%r9),%rdi")
    __jit_x86("xor    %eax,%eax")
    __jit_x86("lock xchg %rax,-0x8(%r9)")
    __jit_x86("lock xadd %rax,-0x8(%r9)")
    __naked void stack_access_insns(void)
    {
    	asm volatile (... ::: __clobber_all);
    }

Use regular expressions by default, use basic regular expressions
class in order to avoid escaping symbols like $(), often used in AT&T
disassembly syntax.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/testing/selftests/bpf/progs/bpf_misc.h |   2 +
 tools/testing/selftests/bpf/test_loader.c    | 156 +++++++++++++------
 2 files changed, 112 insertions(+), 46 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
index a225cd87897c..06e353a0a5b1 100644
--- a/tools/testing/selftests/bpf/progs/bpf_misc.h
+++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
@@ -90,6 +90,8 @@
 #define __arch_x86_64		__arch("X86_64")
 #define __arch_arm64		__arch("ARM64")
 #define __arch_riscv64		__arch("RISCV64")
+#define __jit_x86(basic_regex)	__attribute__((btf_decl_tag("comment:test_jit_x86=" basic_regex)))
+#define __jit_x86_unpriv(basic_regex)	__attribute__((btf_decl_tag("comment:test_jit_x86_unpriv=" basic_regex)))
 
 /* Convenience macro for use with 'asm volatile' blocks */
 #define __naked __attribute__((naked))
diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 1b1290e090e7..7d8a0cf9904a 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -10,6 +10,7 @@
 #include "disasm_helpers.h"
 #include "unpriv_helpers.h"
 #include "cap_helpers.h"
+#include "jit_disasm_helpers.h"
 
 #define str_has_pfx(str, pfx) \
 	(strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx) - 1 : strlen(pfx)) == 0)
@@ -35,6 +36,8 @@
 #define TEST_TAG_AUXILIARY_UNPRIV "comment:test_auxiliary_unpriv"
 #define TEST_BTF_PATH "comment:test_btf_path="
 #define TEST_TAG_ARCH "comment:test_arch="
+#define TEST_TAG_JIT_X86_PFX "comment:test_jit_x86="
+#define TEST_TAG_JIT_X86_PFX_UNPRIV "comment:test_jit_x86_unpriv="
 
 /* Warning: duplicated in bpf_misc.h */
 #define POINTER_VALUE	0xcafe4all
@@ -53,10 +56,18 @@ enum mode {
 	UNPRIV = 2
 };
 
+enum arch {
+	ARCH_X86_64	= 1,
+	ARCH_ARM64	= 2,
+	ARCH_RISCV64	= 3,
+	ARCH_MAX
+};
+
 struct expect_msg {
 	const char *substr; /* substring match */
 	const char *regex_str; /* regex-based match */
 	regex_t regex;
+	int regex_flags;
 };
 
 struct expected_msgs {
@@ -69,6 +80,7 @@ struct test_subspec {
 	bool expect_failure;
 	struct expected_msgs expect_msgs;
 	struct expected_msgs expect_xlated;
+	struct expected_msgs jited[ARCH_MAX];
 	int retval;
 	bool execute;
 };
@@ -120,11 +132,17 @@ static void free_msgs(struct expected_msgs *msgs)
 
 static void free_test_spec(struct test_spec *spec)
 {
+	int i;
+
 	/* Deallocate expect_msgs arrays. */
 	free_msgs(&spec->priv.expect_msgs);
 	free_msgs(&spec->unpriv.expect_msgs);
 	free_msgs(&spec->priv.expect_xlated);
 	free_msgs(&spec->unpriv.expect_xlated);
+	for (i = 0; i < ARCH_MAX; ++i) {
+		free_msgs(&spec->priv.jited[i]);
+		free_msgs(&spec->unpriv.jited[i]);
+	}
 
 	free(spec->priv.name);
 	free(spec->unpriv.name);
@@ -132,7 +150,8 @@ static void free_test_spec(struct test_spec *spec)
 	spec->unpriv.name = NULL;
 }
 
-static int push_msg(const char *substr, const char *regex_str, struct expected_msgs *msgs)
+static int __push_msg(const char *substr, const char *regex_str, int regex_flags,
+		      struct expected_msgs *msgs)
 {
 	void *tmp;
 	int regcomp_res;
@@ -151,10 +170,12 @@ static int push_msg(const char *substr, const char *regex_str, struct expected_m
 	if (substr) {
 		msg->substr = substr;
 		msg->regex_str = NULL;
+		msg->regex_flags = 0;
 	} else {
 		msg->regex_str = regex_str;
 		msg->substr = NULL;
-		regcomp_res = regcomp(&msg->regex, regex_str, REG_EXTENDED|REG_NEWLINE);
+		msg->regex_flags = regex_flags;
+		regcomp_res = regcomp(&msg->regex, regex_str, regex_flags|REG_NEWLINE);
 		if (regcomp_res != 0) {
 			regerror(regcomp_res, &msg->regex, error_msg, sizeof(error_msg));
 			PRINT_FAIL("Regexp compilation error in '%s': '%s'\n",
@@ -167,6 +188,35 @@ static int push_msg(const char *substr, const char *regex_str, struct expected_m
 	return 0;
 }
 
+static int clone_msgs(struct expected_msgs *from, struct expected_msgs *to)
+{
+	struct expect_msg *msg;
+	int i, err;
+
+	for (i = 0; i < from->cnt; i++) {
+		msg = &from->patterns[i];
+		err = __push_msg(msg->substr, msg->regex_str, msg->regex_flags, to);
+		if (err)
+			return err;
+	}
+	return 0;
+}
+
+static int push_msg(const char *substr, struct expected_msgs *msgs)
+{
+	return __push_msg(substr, NULL, 0, msgs);
+}
+
+static int push_extended_regex(const char *regex_str, struct expected_msgs *msgs)
+{
+	return __push_msg(NULL, regex_str, REG_EXTENDED, msgs);
+}
+
+static int push_basic_regex(const char *regex_str, struct expected_msgs *msgs)
+{
+	return __push_msg(NULL, regex_str, 0, msgs);
+}
+
 static int parse_int(const char *str, int *val, const char *name)
 {
 	char *end;
@@ -215,12 +265,6 @@ static void update_flags(int *flags, int flag, bool clear)
 		*flags |= flag;
 }
 
-enum arch {
-	ARCH_X86_64	= 0x1,
-	ARCH_ARM64	= 0x2,
-	ARCH_RISCV64	= 0x4,
-};
-
 /* Uses btf_decl_tag attributes to describe the expected test
  * behavior, see bpf_misc.h for detailed description of each attribute
  * and attribute combinations.
@@ -292,37 +336,49 @@ static int parse_test_spec(struct test_loader *tester,
 			spec->mode_mask |= UNPRIV;
 		} else if (str_has_pfx(s, TEST_TAG_EXPECT_MSG_PFX)) {
 			msg = s + sizeof(TEST_TAG_EXPECT_MSG_PFX) - 1;
-			err = push_msg(msg, NULL, &spec->priv.expect_msgs);
+			err = push_msg(msg, &spec->priv.expect_msgs);
 			if (err)
 				goto cleanup;
 			spec->mode_mask |= PRIV;
 		} else if (str_has_pfx(s, TEST_TAG_EXPECT_MSG_PFX_UNPRIV)) {
 			msg = s + sizeof(TEST_TAG_EXPECT_MSG_PFX_UNPRIV) - 1;
-			err = push_msg(msg, NULL, &spec->unpriv.expect_msgs);
+			err = push_msg(msg, &spec->unpriv.expect_msgs);
 			if (err)
 				goto cleanup;
 			spec->mode_mask |= UNPRIV;
 		} else if (str_has_pfx(s, TEST_TAG_EXPECT_REGEX_PFX)) {
 			msg = s + sizeof(TEST_TAG_EXPECT_REGEX_PFX) - 1;
-			err = push_msg(NULL, msg, &spec->priv.expect_msgs);
+			err = push_extended_regex(msg, &spec->priv.expect_msgs);
 			if (err)
 				goto cleanup;
 			spec->mode_mask |= PRIV;
 		} else if (str_has_pfx(s, TEST_TAG_EXPECT_REGEX_PFX_UNPRIV)) {
 			msg = s + sizeof(TEST_TAG_EXPECT_REGEX_PFX_UNPRIV) - 1;
-			err = push_msg(NULL, msg, &spec->unpriv.expect_msgs);
+			err = push_extended_regex(msg, &spec->unpriv.expect_msgs);
+			if (err)
+				goto cleanup;
+			spec->mode_mask |= UNPRIV;
+		} else if (str_has_pfx(s, TEST_TAG_JIT_X86_PFX)) {
+			msg = s + sizeof(TEST_TAG_JIT_X86_PFX) - 1;
+			err = push_basic_regex(msg, &spec->priv.jited[ARCH_X86_64]);
+			if (err)
+				goto cleanup;
+			spec->mode_mask |= PRIV;
+		} else if (str_has_pfx(s, TEST_TAG_JIT_X86_PFX_UNPRIV)) {
+			msg = s + sizeof(TEST_TAG_JIT_X86_PFX_UNPRIV) - 1;
+			err = push_basic_regex(msg, &spec->unpriv.jited[ARCH_X86_64]);
 			if (err)
 				goto cleanup;
 			spec->mode_mask |= UNPRIV;
 		} else if (str_has_pfx(s, TEST_TAG_EXPECT_XLATED_PFX)) {
 			msg = s + sizeof(TEST_TAG_EXPECT_XLATED_PFX) - 1;
-			err = push_msg(msg, NULL, &spec->priv.expect_xlated);
+			err = push_msg(msg, &spec->priv.expect_xlated);
 			if (err)
 				goto cleanup;
 			spec->mode_mask |= PRIV;
 		} else if (str_has_pfx(s, TEST_TAG_EXPECT_XLATED_PFX_UNPRIV)) {
 			msg = s + sizeof(TEST_TAG_EXPECT_XLATED_PFX_UNPRIV) - 1;
-			err = push_msg(msg, NULL, &spec->unpriv.expect_xlated);
+			err = push_msg(msg, &spec->unpriv.expect_xlated);
 			if (err)
 				goto cleanup;
 			spec->mode_mask |= UNPRIV;
@@ -376,11 +432,11 @@ static int parse_test_spec(struct test_loader *tester,
 		} else if (str_has_pfx(s, TEST_TAG_ARCH)) {
 			val = s + sizeof(TEST_TAG_ARCH) - 1;
 			if (strcmp(val, "X86_64") == 0) {
-				arch_mask |= ARCH_X86_64;
+				arch_mask |= 1u << ARCH_X86_64;
 			} else if (strcmp(val, "ARM64") == 0) {
-				arch_mask |= ARCH_ARM64;
+				arch_mask |= 1u << ARCH_ARM64;
 			} else if (strcmp(val, "RISCV64") == 0) {
-				arch_mask |= ARCH_RISCV64;
+				arch_mask |= 1u << ARCH_RISCV64;
 			} else {
 				PRINT_FAIL("bad arch spec: '%s'", val);
 				err = -EINVAL;
@@ -434,26 +490,13 @@ static int parse_test_spec(struct test_loader *tester,
 			spec->unpriv.execute = spec->priv.execute;
 		}
 
-		if (spec->unpriv.expect_msgs.cnt == 0) {
-			for (i = 0; i < spec->priv.expect_msgs.cnt; i++) {
-				struct expect_msg *msg = &spec->priv.expect_msgs.patterns[i];
-
-				err = push_msg(msg->substr, msg->regex_str,
-					       &spec->unpriv.expect_msgs);
-				if (err)
-					goto cleanup;
-			}
-		}
-		if (spec->unpriv.expect_xlated.cnt == 0) {
-			for (i = 0; i < spec->priv.expect_xlated.cnt; i++) {
-				struct expect_msg *msg = &spec->priv.expect_xlated.patterns[i];
-
-				err = push_msg(msg->substr, msg->regex_str,
-					       &spec->unpriv.expect_xlated);
-				if (err)
-					goto cleanup;
-			}
-		}
+		if (spec->unpriv.expect_msgs.cnt == 0)
+			clone_msgs(&spec->priv.expect_msgs, &spec->unpriv.expect_msgs);
+		if (spec->unpriv.expect_xlated.cnt == 0)
+			clone_msgs(&spec->priv.expect_xlated, &spec->unpriv.expect_xlated);
+		for (i = 0; i < ARCH_MAX; ++i)
+			if (spec->unpriv.jited[i].cnt == 0)
+				clone_msgs(&spec->priv.jited[i], &spec->unpriv.jited[i]);
 	}
 
 	spec->valid = true;
@@ -508,6 +551,13 @@ static void emit_xlated(const char *xlated, bool force)
 	fprintf(stdout, "XLATED:\n=============\n%s=============\n", xlated);
 }
 
+static void emit_jited(const char *jited, bool force)
+{
+	if (!force && env.verbosity == VERBOSE_NONE)
+		return;
+	fprintf(stdout, "JITED:\n=============\n%s=============\n", jited);
+}
+
 static void validate_msgs(char *log_buf, struct expected_msgs *msgs,
 			  void (*emit_fn)(const char *buf, bool force))
 {
@@ -702,18 +752,16 @@ static int get_xlated_program_text(int prog_fd, char *text, size_t text_sz)
 	return err;
 }
 
-static bool run_on_current_arch(int arch_mask)
+static int get_current_arch(void)
 {
-	if (arch_mask == 0)
-		return true;
 #if defined(__x86_64__)
-	return arch_mask & ARCH_X86_64;
+	return ARCH_X86_64;
 #elif defined(__aarch64__)
-	return arch_mask & ARCH_ARM64;
+	return ARCH_ARM64;
 #elif defined(__riscv) && __riscv_xlen == 64
-	return arch_mask & ARCH_RISCV64;
+	return ARCH_RISCV64;
 #endif
-	return false;
+	return -1;
 }
 
 /* this function is forced noinline and has short generic name to look better
@@ -732,15 +780,16 @@ void run_subtest(struct test_loader *tester,
 	struct bpf_program *tprog = NULL, *tprog_iter;
 	struct test_spec *spec_iter;
 	struct cap_state caps = {};
+	int retval, err, i, arch;
 	struct bpf_object *tobj;
 	struct bpf_map *map;
-	int retval, err, i;
 	bool should_load;
 
 	if (!test__start_subtest(subspec->name))
 		return;
 
-	if (!run_on_current_arch(spec->arch_mask)) {
+	arch = get_current_arch();
+	if (spec->arch_mask && (arch < 0 || (spec->arch_mask & (1u << arch)) == 0)) {
 		test__skip();
 		return;
 	}
@@ -817,6 +866,21 @@ void run_subtest(struct test_loader *tester,
 		validate_msgs(tester->log_buf, &subspec->expect_xlated, emit_xlated);
 	}
 
+	if (arch > 0 && subspec->jited[arch].cnt) {
+		err = get_jited_program_text(bpf_program__fd(tprog),
+					     tester->log_buf, tester->log_buf_sz);
+		if (err == -ENOTSUP) {
+			printf("%s:SKIP: jited programs disassembly is not supported,\n", __func__);
+			printf("%s:SKIP: tests are built w/o LLVM development libs\n", __func__);
+			test__skip();
+			goto tobj_cleanup;
+		}
+		if (!ASSERT_EQ(err, 0, "get_jited_program_text"))
+			goto tobj_cleanup;
+		emit_jited(tester->log_buf, false /*force*/);
+		validate_msgs(tester->log_buf, &subspec->jited[arch], emit_jited);
+	}
+
 	if (should_do_test_run(spec, subspec)) {
 		/* For some reason test_verifier executes programs
 		 * with all capabilities restored. Do the same here.
-- 
2.45.2

Re: [PATCH bpf-next 3/4] selftests/bpf: __jited_x86 test tag to check x86 assembly after jit

[Andrii Nakryiko]

On Thu, Aug 8, 2024 at 6:05 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Allow to verify jit behaviour by writing tests as below:
>
>     SEC("tp")
>     __jit_x86("endbr64")
>     __jit_x86("movabs $0x.*,%r9")
>     __jit_x86("add    %gs:0x.*,%r9")
>     __jit_x86("mov    $0x1,%edi")
>     __jit_x86("mov    %rdi,-0x8(%r9)")
>     __jit_x86("mov    -0x8(%r9),%rdi")
>     __jit_x86("xor    %eax,%eax")
>     __jit_x86("lock xchg %rax,-0x8(%r9)")
>     __jit_x86("lock xadd %rax,-0x8(%r9)")
>     __naked void stack_access_insns(void)
>     {
>         asm volatile (... ::: __clobber_all);
>     }
>
> Use regular expressions by default, use basic regular expressions
> class in order to avoid escaping symbols like $(), often used in AT&T
> disassembly syntax.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  tools/testing/selftests/bpf/progs/bpf_misc.h |   2 +
>  tools/testing/selftests/bpf/test_loader.c    | 156 +++++++++++++------
>  2 files changed, 112 insertions(+), 46 deletions(-)
>

[...]

> @@ -817,6 +866,21 @@ void run_subtest(struct test_loader *tester,
>                 validate_msgs(tester->log_buf, &subspec->expect_xlated, emit_xlated);
>         }
>
> +       if (arch > 0 && subspec->jited[arch].cnt) {
> +               err = get_jited_program_text(bpf_program__fd(tprog),
> +                                            tester->log_buf, tester->log_buf_sz);
> +               if (err == -ENOTSUP) {

nit: let's use EOPNOTSUPP, ENOTSUP is internal Linux error

> +                       printf("%s:SKIP: jited programs disassembly is not supported,\n", __func__);
> +                       printf("%s:SKIP: tests are built w/o LLVM development libs\n", __func__);
> +                       test__skip();
> +                       goto tobj_cleanup;
> +               }
> +               if (!ASSERT_EQ(err, 0, "get_jited_program_text"))
> +                       goto tobj_cleanup;
> +               emit_jited(tester->log_buf, false /*force*/);
> +               validate_msgs(tester->log_buf, &subspec->jited[arch], emit_jited);
> +       }
> +
>         if (should_do_test_run(spec, subspec)) {
>                 /* For some reason test_verifier executes programs
>                  * with all capabilities restored. Do the same here.
> --
> 2.45.2
>

Re: [PATCH bpf-next 3/4] selftests/bpf: __jited_x86 test tag to check x86 assembly after jit

[Eduard Zingerman]

On Thu, 2024-08-15 at 14:11 -0700, Andrii Nakryiko wrote:

[...]

> > @@ -817,6 +866,21 @@ void run_subtest(struct test_loader *tester,
> >                 validate_msgs(tester->log_buf, &subspec->expect_xlated, emit_xlated);
> >         }
> > 
> > +       if (arch > 0 && subspec->jited[arch].cnt) {
> > +               err = get_jited_program_text(bpf_program__fd(tprog),
> > +                                            tester->log_buf, tester->log_buf_sz);
> > +               if (err == -ENOTSUP) {
> 
> nit: let's use EOPNOTSUPP, ENOTSUP is internal Linux error

Ok, will change.

[...]

[PATCH bpf-next 4/4] selftests/bpf: validate jit behaviour for tail calls

[Eduard Zingerman]

A program calling sub-program which does a tail call.
The idea is to verify instructions generated by jit for tail calls:
- in program and sub-program prologues;
- for subprogram call instruction;
- for tail call itself.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../selftests/bpf/prog_tests/verifier.c       |   2 +
 .../bpf/progs/verifier_tailcall_jit.c         | 103 ++++++++++++++++++
 2 files changed, 105 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c

diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index f8f546eba488..cf3662dbd24f 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -75,6 +75,7 @@
 #include "verifier_stack_ptr.skel.h"
 #include "verifier_subprog_precision.skel.h"
 #include "verifier_subreg.skel.h"
+#include "verifier_tailcall_jit.skel.h"
 #include "verifier_typedef.skel.h"
 #include "verifier_uninit.skel.h"
 #include "verifier_unpriv.skel.h"
@@ -198,6 +199,7 @@ void test_verifier_spin_lock(void)            { RUN(verifier_spin_lock); }
 void test_verifier_stack_ptr(void)            { RUN(verifier_stack_ptr); }
 void test_verifier_subprog_precision(void)    { RUN(verifier_subprog_precision); }
 void test_verifier_subreg(void)               { RUN(verifier_subreg); }
+void test_verifier_tailcall_jit(void)         { RUN(verifier_tailcall_jit); }
 void test_verifier_typedef(void)              { RUN(verifier_typedef); }
 void test_verifier_uninit(void)               { RUN(verifier_uninit); }
 void test_verifier_unpriv(void)               { RUN(verifier_unpriv); }
diff --git a/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c b/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c
new file mode 100644
index 000000000000..1a09c76d7be0
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c
@@ -0,0 +1,103 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_misc.h"
+
+int main(void);
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
+	__uint(max_entries, 1);
+	__uint(key_size, sizeof(__u32));
+	__array(values, void (void));
+} jmp_table SEC(".maps") = {
+	.values = {
+		[0] = (void *) &main,
+	},
+};
+
+__noinline __auxiliary
+static __naked int sub(void)
+{
+	asm volatile (
+	"r2 = %[jmp_table] ll;"
+	"r3 = 0;"
+	"call 12;"
+	"exit;"
+	:
+	: __imm_addr(jmp_table)
+	: __clobber_all);
+}
+
+__success
+/* program entry for main(), regular function prologue */
+__jit_x86("	endbr64")
+__jit_x86("	nopl	(%rax,%rax)")
+__jit_x86("	xorq	%rax, %rax")
+__jit_x86("	pushq	%rbp")
+__jit_x86("	movq	%rsp, %rbp")
+/* tail call prologue for program:
+ * - establish memory location for tail call counter at &rbp[-8];
+ * - spill tail_call_cnt_ptr at &rbp[-16];
+ * - expect tail call counter to be passed in rax;
+ * - for entry program rax is a raw counter, value < 33;
+ * - for tail called program rax is tail_call_cnt_ptr (value > 33).
+ */
+__jit_x86("	endbr64")
+__jit_x86("	cmpq	$0x21, %rax")
+__jit_x86("	ja	L0")
+__jit_x86("	pushq	%rax")
+__jit_x86("	movq	%rsp, %rax")
+__jit_x86("	jmp	L1")
+__jit_x86("L0:	pushq	%rax")			/* rbp[-8]  = rax         */
+__jit_x86("L1:	pushq	%rax")			/* rbp[-16] = rax         */
+/* on subprogram call restore rax to be tail_call_cnt_ptr from rbp[-16]
+ * (cause original rax might be clobbered by this point)
+ */
+__jit_x86("	movq	-0x10(%rbp), %rax")
+__jit_x86("	callq	0x[0-9a-f]\\+")		/* call to sub()          */
+__jit_x86("	xorl	%eax, %eax")
+__jit_x86("	leave")
+__jit_x86("	retq")
+/* subprogram entry for sub(), regular function prologue */
+__jit_x86("	endbr64")
+__jit_x86("	nopl	(%rax,%rax)")
+__jit_x86("	nopl	(%rax)")
+__jit_x86("	pushq	%rbp")
+__jit_x86("	movq	%rsp, %rbp")
+/* tail call prologue for subprogram address of tail call counter
+ * stored at rbp[-16].
+ */
+__jit_x86("	endbr64")
+__jit_x86("	pushq	%rax")			/* rbp[-8]  = rax          */
+__jit_x86("	pushq	%rax")			/* rbp[-16] = rax          */
+__jit_x86("	movabsq	$-0x[0-9a-f]\\+, %rsi")	/* r2 = &jmp_table         */
+__jit_x86("	xorl	%edx, %edx")		/* r3 = 0                  */
+/* bpf_tail_call implementation:
+ * - load tail_call_cnt_ptr from rbp[-16];
+ * - if *tail_call_cnt_ptr < 33, increment it and jump to target;
+ * - otherwise do nothing.
+ */
+__jit_x86("	movq	-0x10(%rbp), %rax")
+__jit_x86("	cmpq	$0x21, (%rax)")
+__jit_x86("	jae	L0")
+__jit_x86("	nopl	(%rax,%rax)")
+__jit_x86("	addq	$0x1, (%rax)")		/* *tail_call_cnt_ptr += 1 */
+__jit_x86("	popq	%rax")
+__jit_x86("	popq	%rax")
+__jit_x86("	jmp	0x[0-9a-f]\\+")		/* jump to tail call tgt   */
+__jit_x86("L0:	leave")
+__jit_x86("	retq")
+SEC("tc")
+__naked int main(void)
+{
+	asm volatile (
+	"call %[sub];"
+	"r0 = 0;"
+	"exit;"
+	:
+	: __imm(sub)
+	: __clobber_all);
+}
+
+char __license[] SEC("license") = "GPL";
-- 
2.45.2

Re: [PATCH bpf-next 4/4] selftests/bpf: validate jit behaviour for tail calls

[Andrii Nakryiko]

On Thu, Aug 8, 2024 at 6:05 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> A program calling sub-program which does a tail call.
> The idea is to verify instructions generated by jit for tail calls:
> - in program and sub-program prologues;
> - for subprogram call instruction;
> - for tail call itself.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  .../selftests/bpf/prog_tests/verifier.c       |   2 +
>  .../bpf/progs/verifier_tailcall_jit.c         | 103 ++++++++++++++++++
>  2 files changed, 105 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> index f8f546eba488..cf3662dbd24f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> @@ -75,6 +75,7 @@
>  #include "verifier_stack_ptr.skel.h"
>  #include "verifier_subprog_precision.skel.h"
>  #include "verifier_subreg.skel.h"
> +#include "verifier_tailcall_jit.skel.h"
>  #include "verifier_typedef.skel.h"
>  #include "verifier_uninit.skel.h"
>  #include "verifier_unpriv.skel.h"
> @@ -198,6 +199,7 @@ void test_verifier_spin_lock(void)            { RUN(verifier_spin_lock); }
>  void test_verifier_stack_ptr(void)            { RUN(verifier_stack_ptr); }
>  void test_verifier_subprog_precision(void)    { RUN(verifier_subprog_precision); }
>  void test_verifier_subreg(void)               { RUN(verifier_subreg); }
> +void test_verifier_tailcall_jit(void)         { RUN(verifier_tailcall_jit); }
>  void test_verifier_typedef(void)              { RUN(verifier_typedef); }
>  void test_verifier_uninit(void)               { RUN(verifier_uninit); }
>  void test_verifier_unpriv(void)               { RUN(verifier_unpriv); }
> diff --git a/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c b/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c
> new file mode 100644
> index 000000000000..1a09c76d7be0
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c
> @@ -0,0 +1,103 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_misc.h"
> +
> +int main(void);
> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> +       __uint(max_entries, 1);
> +       __uint(key_size, sizeof(__u32));
> +       __array(values, void (void));
> +} jmp_table SEC(".maps") = {
> +       .values = {
> +               [0] = (void *) &main,
> +       },
> +};
> +
> +__noinline __auxiliary
> +static __naked int sub(void)
> +{
> +       asm volatile (
> +       "r2 = %[jmp_table] ll;"
> +       "r3 = 0;"
> +       "call 12;"
> +       "exit;"
> +       :
> +       : __imm_addr(jmp_table)
> +       : __clobber_all);
> +}
> +
> +__success
> +/* program entry for main(), regular function prologue */
> +__jit_x86("    endbr64")
> +__jit_x86("    nopl    (%rax,%rax)")
> +__jit_x86("    xorq    %rax, %rax")
> +__jit_x86("    pushq   %rbp")
> +__jit_x86("    movq    %rsp, %rbp")

I'm a bit too lazy to fish it out of the code, so I'll just ask.
Does matching of __jit_x86() string behave in the same way as __msg().
I.e., there could be unexpected lines that would be skipped, as long
as we find a match for each __jit_x86() one?


Isn't that a bit counter-intuitive and potentially dangerous behavior
for checking disassembly? If my assumption is correct, maybe we should
add some sort of `__jit_x86("...")` placeholder to explicitly mark
that we allow some amount of lines to be skipped, but otherwise be
strict and require matching line-by-line?

> +/* tail call prologue for program:
> + * - establish memory location for tail call counter at &rbp[-8];
> + * - spill tail_call_cnt_ptr at &rbp[-16];
> + * - expect tail call counter to be passed in rax;
> + * - for entry program rax is a raw counter, value < 33;
> + * - for tail called program rax is tail_call_cnt_ptr (value > 33).
> + */
> +__jit_x86("    endbr64")
> +__jit_x86("    cmpq    $0x21, %rax")
> +__jit_x86("    ja      L0")
> +__jit_x86("    pushq   %rax")
> +__jit_x86("    movq    %rsp, %rax")
> +__jit_x86("    jmp     L1")
> +__jit_x86("L0: pushq   %rax")                  /* rbp[-8]  = rax         */
> +__jit_x86("L1: pushq   %rax")                  /* rbp[-16] = rax         */
> +/* on subprogram call restore rax to be tail_call_cnt_ptr from rbp[-16]
> + * (cause original rax might be clobbered by this point)
> + */
> +__jit_x86("    movq    -0x10(%rbp), %rax")
> +__jit_x86("    callq   0x[0-9a-f]\\+")         /* call to sub()          */
> +__jit_x86("    xorl    %eax, %eax")
> +__jit_x86("    leave")
> +__jit_x86("    retq")
> +/* subprogram entry for sub(), regular function prologue */
> +__jit_x86("    endbr64")
> +__jit_x86("    nopl    (%rax,%rax)")
> +__jit_x86("    nopl    (%rax)")
> +__jit_x86("    pushq   %rbp")
> +__jit_x86("    movq    %rsp, %rbp")
> +/* tail call prologue for subprogram address of tail call counter
> + * stored at rbp[-16].
> + */
> +__jit_x86("    endbr64")
> +__jit_x86("    pushq   %rax")                  /* rbp[-8]  = rax          */
> +__jit_x86("    pushq   %rax")                  /* rbp[-16] = rax          */
> +__jit_x86("    movabsq $-0x[0-9a-f]\\+, %rsi") /* r2 = &jmp_table         */
> +__jit_x86("    xorl    %edx, %edx")            /* r3 = 0                  */
> +/* bpf_tail_call implementation:
> + * - load tail_call_cnt_ptr from rbp[-16];
> + * - if *tail_call_cnt_ptr < 33, increment it and jump to target;
> + * - otherwise do nothing.
> + */
> +__jit_x86("    movq    -0x10(%rbp), %rax")
> +__jit_x86("    cmpq    $0x21, (%rax)")
> +__jit_x86("    jae     L0")
> +__jit_x86("    nopl    (%rax,%rax)")
> +__jit_x86("    addq    $0x1, (%rax)")          /* *tail_call_cnt_ptr += 1 */
> +__jit_x86("    popq    %rax")
> +__jit_x86("    popq    %rax")
> +__jit_x86("    jmp     0x[0-9a-f]\\+")         /* jump to tail call tgt   */
> +__jit_x86("L0: leave")
> +__jit_x86("    retq")
> +SEC("tc")
> +__naked int main(void)
> +{
> +       asm volatile (
> +       "call %[sub];"
> +       "r0 = 0;"
> +       "exit;"
> +       :
> +       : __imm(sub)
> +       : __clobber_all);
> +}
> +
> +char __license[] SEC("license") = "GPL";
> --
> 2.45.2
>

Re: [PATCH bpf-next 4/4] selftests/bpf: validate jit behaviour for tail calls

[Eduard Zingerman]

On Thu, 2024-08-15 at 14:15 -0700, Andrii Nakryiko wrote:

[...]

> > +/* program entry for main(), regular function prologue */
> > +__jit_x86("    endbr64")
> > +__jit_x86("    nopl    (%rax,%rax)")
> > +__jit_x86("    xorq    %rax, %rax")
> > +__jit_x86("    pushq   %rbp")
> > +__jit_x86("    movq    %rsp, %rbp")
> 
> I'm a bit too lazy to fish it out of the code, so I'll just ask.
> Does matching of __jit_x86() string behave in the same way as __msg().
> I.e., there could be unexpected lines that would be skipped, as long
> as we find a match for each __jit_x86() one?

Yes, behaves same way as __msg().
 
> Isn't that a bit counter-intuitive and potentially dangerous behavior
> for checking disassembly? If my assumption is correct, maybe we should
> add some sort of `__jit_x86("...")` placeholder to explicitly mark
> that we allow some amount of lines to be skipped, but otherwise be
> strict and require matching line-by-line?

This is a valid concern.
What you suggest with "..." looks good.
Another option is to follow llvm-lit conventions and add
__jit_x86_next("<whatever>"), which would only match if pattern
is on line below any previous match.
(And later add __jit_x86_next_not, and make these *_not, *_next
 variants accessible for every __msg-like macro).
 
Link: https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-next-directive

[...]

Re: [PATCH bpf-next 4/4] selftests/bpf: validate jit behaviour for tail calls

[Andrii Nakryiko]

On Thu, Aug 15, 2024 at 2:42 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2024-08-15 at 14:15 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > > +/* program entry for main(), regular function prologue */
> > > +__jit_x86("    endbr64")
> > > +__jit_x86("    nopl    (%rax,%rax)")
> > > +__jit_x86("    xorq    %rax, %rax")
> > > +__jit_x86("    pushq   %rbp")
> > > +__jit_x86("    movq    %rsp, %rbp")
> >
> > I'm a bit too lazy to fish it out of the code, so I'll just ask.
> > Does matching of __jit_x86() string behave in the same way as __msg().
> > I.e., there could be unexpected lines that would be skipped, as long
> > as we find a match for each __jit_x86() one?
>
> Yes, behaves same way as __msg().
>
> > Isn't that a bit counter-intuitive and potentially dangerous behavior
> > for checking disassembly? If my assumption is correct, maybe we should
> > add some sort of `__jit_x86("...")` placeholder to explicitly mark
> > that we allow some amount of lines to be skipped, but otherwise be
> > strict and require matching line-by-line?
>
> This is a valid concern.
> What you suggest with "..." looks good.

I'd add just that for now. _not and _next might be useful in the
future, but meh.

> Another option is to follow llvm-lit conventions and add
> __jit_x86_next("<whatever>"), which would only match if pattern
> is on line below any previous match.
> (And later add __jit_x86_next_not, and make these *_not, *_next
>  variants accessible for every __msg-like macro).
>
> Link: https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-next-directive
>
> [...]
>

Re: [PATCH bpf-next 4/4] selftests/bpf: validate jit behaviour for tail calls

[Eduard Zingerman]

On Thu, 2024-08-15 at 15:07 -0700, Andrii Nakryiko wrote:

[...]

> > > Isn't that a bit counter-intuitive and potentially dangerous behavior
> > > for checking disassembly? If my assumption is correct, maybe we should
> > > add some sort of `__jit_x86("...")` placeholder to explicitly mark
> > > that we allow some amount of lines to be skipped, but otherwise be
> > > strict and require matching line-by-line?
> > 
> > This is a valid concern.
> > What you suggest with "..." looks good.
> 
> I'd add just that for now. _not and _next might be useful in the
> future, but meh.

If we commit to "..." now and decide to add _not and _next in the
future this would make __jit macro special. Which is not ideal, imo.
(on the other hand, tests can always be adjusted).

[...]

Re: [PATCH bpf-next 4/4] selftests/bpf: validate jit behaviour for tail calls

[Andrii Nakryiko]

On Thu, Aug 15, 2024 at 3:11 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2024-08-15 at 15:07 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > > > Isn't that a bit counter-intuitive and potentially dangerous behavior
> > > > for checking disassembly? If my assumption is correct, maybe we should
> > > > add some sort of `__jit_x86("...")` placeholder to explicitly mark
> > > > that we allow some amount of lines to be skipped, but otherwise be
> > > > strict and require matching line-by-line?
> > >
> > > This is a valid concern.
> > > What you suggest with "..." looks good.
> >
> > I'd add just that for now. _not and _next might be useful in the
> > future, but meh.
>
> If we commit to "..." now and decide to add _not and _next in the
> future this would make __jit macro special. Which is not ideal, imo.
> (on the other hand, tests can always be adjusted).
>

It is already special with a different flavor of regex. And I assume
we won't have that many jit-testing tests, so yeah, could be adjusted,
if necessary. But just in general, while __msg() works with large
verifier logs, __jit() is much more narrow-focused, so even if it
behaves differently from __msg() I don't really see much difference.

But we also have __xlated() with similar semantics, so I'd say we
should keep __jit() and __xlated() behaving similarly.

> [...]
>

Re: [PATCH bpf-next 4/4] selftests/bpf: validate jit behaviour for tail calls

[Eduard Zingerman]

On Thu, 2024-08-15 at 15:14 -0700, Andrii Nakryiko wrote:

[...]

> It is already special with a different flavor of regex. And I assume
> we won't have that many jit-testing tests, so yeah, could be adjusted,
> if necessary. But just in general, while __msg() works with large
> verifier logs, __jit() is much more narrow-focused, so even if it
> behaves differently from __msg() I don't really see much difference.
> 
> But we also have __xlated() with similar semantics, so I'd say we
> should keep __jit() and __xlated() behaving similarly.

Ok, makes sense.

Re: [PATCH bpf-next 4/4] selftests/bpf: validate jit behaviour for tail calls

[Yonghong Song]

On 8/8/24 6:05 PM, Eduard Zingerman wrote:
> A program calling sub-program which does a tail call.
> The idea is to verify instructions generated by jit for tail calls:
> - in program and sub-program prologues;
> - for subprogram call instruction;
> - for tail call itself.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>   .../selftests/bpf/prog_tests/verifier.c       |   2 +
>   .../bpf/progs/verifier_tailcall_jit.c         | 103 ++++++++++++++++++
>   2 files changed, 105 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> index f8f546eba488..cf3662dbd24f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> @@ -75,6 +75,7 @@
>   #include "verifier_stack_ptr.skel.h"
>   #include "verifier_subprog_precision.skel.h"
>   #include "verifier_subreg.skel.h"
> +#include "verifier_tailcall_jit.skel.h"
>   #include "verifier_typedef.skel.h"
>   #include "verifier_uninit.skel.h"
>   #include "verifier_unpriv.skel.h"
> @@ -198,6 +199,7 @@ void test_verifier_spin_lock(void)            { RUN(verifier_spin_lock); }
>   void test_verifier_stack_ptr(void)            { RUN(verifier_stack_ptr); }
>   void test_verifier_subprog_precision(void)    { RUN(verifier_subprog_precision); }
>   void test_verifier_subreg(void)               { RUN(verifier_subreg); }
> +void test_verifier_tailcall_jit(void)         { RUN(verifier_tailcall_jit); }
>   void test_verifier_typedef(void)              { RUN(verifier_typedef); }
>   void test_verifier_uninit(void)               { RUN(verifier_uninit); }
>   void test_verifier_unpriv(void)               { RUN(verifier_unpriv); }
> diff --git a/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c b/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c
> new file mode 100644
> index 000000000000..1a09c76d7be0
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/verifier_tailcall_jit.c
> @@ -0,0 +1,103 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_misc.h"
> +
> +int main(void);
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> +	__uint(max_entries, 1);
> +	__uint(key_size, sizeof(__u32));
> +	__array(values, void (void));
> +} jmp_table SEC(".maps") = {
> +	.values = {
> +		[0] = (void *) &main,
> +	},
> +};
> +
> +__noinline __auxiliary
> +static __naked int sub(void)
> +{
> +	asm volatile (
> +	"r2 = %[jmp_table] ll;"
> +	"r3 = 0;"
> +	"call 12;"
> +	"exit;"
> +	:
> +	: __imm_addr(jmp_table)
> +	: __clobber_all);
> +}
> +
> +__success
> +/* program entry for main(), regular function prologue */
> +__jit_x86("	endbr64")
> +__jit_x86("	nopl	(%rax,%rax)")
> +__jit_x86("	xorq	%rax, %rax")
> +__jit_x86("	pushq	%rbp")
> +__jit_x86("	movq	%rsp, %rbp")

How do we hanble multi architectures (x86, arm64, riscv64)?

Do we support the following?

__jit_x86(...)
__jit_x86(...)
...

__jit_arm64(...)
__jit_arm64(...)
...

__jit_riscv64(...)
__jit_riscv64(...)
...

Or we can use macro like

#ifdef __TARGET_ARCH_x86
__jit(...)
...
#elif defined(__TARGET_ARCH_arm64)
__jit(...)
...
#elif defined(...)

Or we can have

__arch_x86_64
__jit(...) // code for x86
...

__arch_arm64
__jit(...) // code for arm64
...

__arch_riscv
__jit(...) // code for riscv
...

For xlated, different archs could share the same code.
Bot for jited code, different arch has different encoding,
so we need to figure out a format suitable for multiple
archs.

> +/* tail call prologue for program:
> + * - establish memory location for tail call counter at &rbp[-8];
> + * - spill tail_call_cnt_ptr at &rbp[-16];
> + * - expect tail call counter to be passed in rax;
> + * - for entry program rax is a raw counter, value < 33;
> + * - for tail called program rax is tail_call_cnt_ptr (value > 33).
> + */
> +__jit_x86("	endbr64")
> +__jit_x86("	cmpq	$0x21, %rax")
> +__jit_x86("	ja	L0")
> +__jit_x86("	pushq	%rax")
> +__jit_x86("	movq	%rsp, %rax")
> +__jit_x86("	jmp	L1")
> +__jit_x86("L0:	pushq	%rax")			/* rbp[-8]  = rax         */
> +__jit_x86("L1:	pushq	%rax")			/* rbp[-16] = rax         */
> +/* on subprogram call restore rax to be tail_call_cnt_ptr from rbp[-16]
> + * (cause original rax might be clobbered by this point)
> + */
[...]

Re: [PATCH bpf-next 4/4] selftests/bpf: validate jit behaviour for tail calls

[Eduard Zingerman]

On Thu, 2024-08-15 at 14:32 -0700, Yonghong Song wrote:

[...]

> > +__success
> > +/* program entry for main(), regular function prologue */
> > +__jit_x86("	endbr64")
> > +__jit_x86("	nopl	(%rax,%rax)")
> > +__jit_x86("	xorq	%rax, %rax")
> > +__jit_x86("	pushq	%rbp")
> > +__jit_x86("	movq	%rsp, %rbp")
> 
> How do we hanble multi architectures (x86, arm64, riscv64)?
> 
> Do we support the following?
> 
> __jit_x86(...)
> __jit_x86(...)
> ...
> 
> __jit_arm64(...)
> __jit_arm64(...)
> ...
> 
> __jit_riscv64(...)
> __jit_riscv64(...)
> ...

^^^^
I was thinking about this variant (and this is how things are now implemented).
Whenever there would be a need for that, just add one more arch
specific macro.

> 
> Or we can use macro like
> 
> #ifdef __TARGET_ARCH_x86
> __jit(...)
> ...
> #elif defined(__TARGET_ARCH_arm64)
> __jit(...)
> ...
> #elif defined(...)
> 
> Or we can have
> 
> __arch_x86_64
> __jit(...) // code for x86
> ...
> 
> __arch_arm64
> __jit(...) // code for arm64
> ...
> 
> __arch_riscv
> __jit(...) // code for riscv
> ...

This also looks good, and will work better with "*_next" and "*_not"
variants if we are going to borrow from llvm-lit/FileCheck.

> For xlated, different archs could share the same code.
> Bot for jited code, different arch has different encoding,
> so we need to figure out a format suitable for multiple
> archs.

I'll go with whatever way mailing list likes better.

[...]

Re: [PATCH bpf-next 4/4] selftests/bpf: validate jit behaviour for tail calls

[Andrii Nakryiko]

On Thu, Aug 15, 2024 at 2:47 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2024-08-15 at 14:32 -0700, Yonghong Song wrote:
>
> [...]
>
> > > +__success
> > > +/* program entry for main(), regular function prologue */
> > > +__jit_x86("        endbr64")
> > > +__jit_x86("        nopl    (%rax,%rax)")
> > > +__jit_x86("        xorq    %rax, %rax")
> > > +__jit_x86("        pushq   %rbp")
> > > +__jit_x86("        movq    %rsp, %rbp")
> >
> > How do we hanble multi architectures (x86, arm64, riscv64)?
> >
> > Do we support the following?
> >
> > __jit_x86(...)
> > __jit_x86(...)
> > ...
> >
> > __jit_arm64(...)
> > __jit_arm64(...)
> > ...
> >
> > __jit_riscv64(...)
> > __jit_riscv64(...)
> > ...
>
> ^^^^
> I was thinking about this variant (and this is how things are now implemented).
> Whenever there would be a need for that, just add one more arch
> specific macro.
>
> >
> > Or we can use macro like
> >
> > #ifdef __TARGET_ARCH_x86
> > __jit(...)
> > ...
> > #elif defined(__TARGET_ARCH_arm64)
> > __jit(...)
> > ...
> > #elif defined(...)
> >
> > Or we can have
> >
> > __arch_x86_64
> > __jit(...) // code for x86
> > ...
> >
> > __arch_arm64
> > __jit(...) // code for arm64
> > ...
> >
> > __arch_riscv
> > __jit(...) // code for riscv
> > ...
>
> This also looks good, and will work better with "*_next" and "*_not"
> variants if we are going to borrow from llvm-lit/FileCheck.
>

shorter __jit() and then arch-specific __arch_blah seems pretty clean,
so if it's not too hard, let's do this.

BTW, in your implementation you are collecting expected messages for
all specified architectures, but really there will always be just one
valid subset. So maybe just discard all non-host architectures upfront
during "parsing" of decl tags?

> > For xlated, different archs could share the same code.
> > Bot for jited code, different arch has different encoding,
> > so we need to figure out a format suitable for multiple
> > archs.
>
> I'll go with whatever way mailing list likes better.
>
> [...]
>

Re: [PATCH bpf-next 4/4] selftests/bpf: validate jit behaviour for tail calls

[Eduard Zingerman]

On Thu, 2024-08-15 at 15:09 -0700, Andrii Nakryiko wrote:

[...]

> > > Or we can use macro like
> > > 
> > > #ifdef __TARGET_ARCH_x86
> > > __jit(...)
> > > ...
> > > #elif defined(__TARGET_ARCH_arm64)
> > > __jit(...)
> > > ...
> > > #elif defined(...)
> > > 
> > > Or we can have
> > > 
> > > __arch_x86_64
> > > __jit(...) // code for x86
> > > ...
> > > 
> > > __arch_arm64
> > > __jit(...) // code for arm64
> > > ...
> > > 
> > > __arch_riscv
> > > __jit(...) // code for riscv
> > > ...
> > 
> > This also looks good, and will work better with "*_next" and "*_not"
> > variants if we are going to borrow from llvm-lit/FileCheck.
> > 
> 
> shorter __jit() and then arch-specific __arch_blah seems pretty clean,
> so if it's not too hard, let's do this.

Ok, let's go this way.

> BTW, in your implementation you are collecting expected messages for
> all specified architectures, but really there will always be just one
> valid subset. So maybe just discard all non-host architectures upfront
> during "parsing" of decl tags?

I kinda wanted to keep parsing logic separate from processing logic,
but yeah, makes sense.

[...]