Skip to contentSkip to conversation

no_caller_saved_registers attribute for helper calls

PATCH Superseded Eduard Zingerman 2 participants 22 messages Jul 15, 2024 – Jul 22, 2024

Subsystems (4)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Eduard Zingerman

author maintainer
#0 lore raw

v3 cover no_caller_saved_registers attribute for helper calls

This patch-set seeks to allow using no_caller_saved_registers gcc/clang attribute with some BPF helper functions (and kfuncs in the future).

As documented in [1], this attribute means that function scratches only some of the caller saved registers defined by ABI. For BPF the set of such registers could be defined as follows:
- R0 is scratched only if function is non-void;
- R1-R5 are scratched only if corresponding parameter type is defined in the function prototype.

The goal of the patch-set is to implement no_caller_saved_registers (nocsr for short) in a backwards compatible manner:
- for kernels that support the feature, gain some performance boost from better register allocation;
- for kernels that don't support the feature, allow programs execution with minor performance losses.

To achieve this, use a scheme suggested by Alexei Starovoitov:
- for nocsr calls clang allocates registers as-if relevant r0-r5 registers are not scratched by the call;
- as a post-processing step, clang visits each nocsr call and adds spill/fill for every live r0-r5;
- stack offsets used for spills/fills are allocated as lowest stack offsets in whole function and are not used for any other purpose;
- when kernel loads a program, it looks for such patterns (nocsr function surrounded by spills/fills) and checks if spill/fill stack offsets are used exclusively in nocsr patterns;
- if so, and if current JIT inlines the call to the nocsr function (e.g. a helper call), kernel removes unnecessary spill/fill pairs;
- when old kernel loads a program, presence of spill/fill pairs keeps BPF program valid, albeit slightly less efficient.

Corresponding clang/llvm changes are available in [2].

The patch-set uses bpf_get_smp_processor_id() function as a canary, making it the first helper with nocsr attribute.

For example, consider the following program:

  #define __no_csr __attribute__((no_caller_saved_registers))
  #define SEC(name) __attribute__((section(name), used))
  #define bpf_printk(fmt, ...) bpf_trace_printk((fmt), sizeof(fmt), __VA_ARGS__)

typedef unsigned int __u32;

  static long (* const bpf_trace_printk)(const char *fmt, __u32 fmt_size, ...) = (void *) 6;
  static __u32 (*const bpf_get_smp_processor_id)(void) __no_csr = (void *)8;

  SEC("raw_tp")
  int test(void *ctx)
  {
          __u32 task = bpf_get_smp_processor_id();
  	bpf_printk("ctx=%p, smp=%d", ctx, task);
  	return 0;
  }

  char _license[] SEC("license") = "GPL";

Compiled (using [2]) as follows:

  $ clang --target=bpf -O2 -g -c -o nocsr.bpf.o nocsr.bpf.c
  $ llvm-objdump --no-show-raw-insn -Sd nocsr.bpf.o
    ...
  3rd parameter for printk call     removable spill/fill pair
  .--- 0:       r3 = r1                             |
; |       __u32 task = bpf_get_smp_processor_id();  |
  |    1:       *(u64 *)(r10 - 0x8) = r3 <----------|
  |    2:       call 0x8                            |
  |    3:       r3 = *(u64 *)(r10 - 0x8) <----------'
; |     bpf_printk("ctx=%p, smp=%d", ctx, task);
  |    4:       r1 = 0x0 ll
  |    6:       r2 = 0xf
  |    7:       r4 = r0
  '--> 8:       call 0x6
;       return 0;
       9:       r0 = 0x0
      10:       exit

Here is how the program looks after verifier processing:

  # bpftool prog load ./nocsr.bpf.o /sys/fs/bpf/nocsr-test
  # bpftool prog dump xlated pinned /sys/fs/bpf/nocsr-test

int test(void * ctx):
0: (bf) r3 = r1 <--- 3rd printk parameter

  ; __u32 task = bpf_get_smp_processor_id();
     1: (b4) w0 = 197324                     <--. inlined helper call,
     2: (bf) r0 = &(void __percpu *)(r0)     <--- spill/fill
     3: (61) r0 = *(u32 *)(r0 +0)            <--' pair removed
  ; bpf_printk("ctx=%p, smp=%d", ctx, task);
     4: (18) r1 = map[id:5][0]+0
     6: (b7) r2 = 15
     7: (bf) r4 = r0
     8: (85) call bpf_trace_printk#-125920
  ; return 0;
     9: (b7) r0 = 0
    10: (95) exit

[1] https://clang.llvm.org/docs/AttributeReference.html#no-caller-saved-registers
[2] https://github.com/eddyz87/llvm-project/tree/bpf-no-caller-saved-registers

Change list:
- v2 -> v3:
- As suggested by Andrii, 'nocsr_stack_off' is no longer checked at rewrite time, instead mark_nocsr_patterns() now does two passes over BPF program:
- on a first pass it computes the lowest stack spill offset for the subprogram;
- on a second pass this offset is used to recognize nocsr pattern.
- As suggested by Alexei, a new mechanic is added to work around a situation mentioned by Andrii, when more helper functions are marked as nocsr at compile time than current kernel supports:
- all {spill*,helper call,fill*} patterns are now marked as insn_aux_data[*].nocsr_pattern, thus relaxing failure condition for check_nocsr_stack_contract();
- spill/fill pairs are not removed for patterns where helper can't be inlined;
- see mark_nocsr_pattern_for_call() for details an example.
- As suggested by Alexei, subprogram stack depth is now adjusted if all spill/fill pairs could be removed. This adjustment has to take place before optimize_bpf_loop(), hence the rewrite is moved from do_misc_fixups() to remove_nocsr_spills_fills() (again).
- As suggested by Andrii, special measures are taken to work around bpf_probe_read_kernel() access to BPF stack, see patches 11, 12. Patch #11 is very simplistic, a more comprehensive solution would be to change the type of the third parameter of the bpf_probe_read_kernel() from ARG_ANYTHING to something else and not only check nocsr contract, but also propagate stack slot liveness information. However, such change would require update in struct bpf_call_arg_meta processing, which currently implies that every memory parameter is followed by a size parameter. I can work on these changes, please comment.
- Stylistic changes suggested by Andrii.
- Added acks from Andrii.
- Dropped RFC tag.
- v1 -> v2:
- assume that functions inlined by either jit or verifier

    conform to no_caller_saved_registers contract (Andrii, Puranjay);
  - allow nocsr rewrite for bpf_get_smp_processor_id()
    on arm64 and riscv64 architectures (Puranjay);
  - __arch_{x86_64,arm64,riscv64} macro for test_loader;
  - moved remove_nocsr_spills_fills() inside do_misc_fixups() (Andrii);
  - moved nocsr pattern detection from check_cfg() to a separate pass
    (Andrii);

- various stylistic/correctness changes according to Andrii's comments.

Revisions:
- v1 https://lore.kernel.org/bpf/[email protected]/
- v2 https://lore.kernel.org/bpf/[email protected]/

Eduard Zingerman (12):
bpf: add a get_helper_proto() utility function
bpf: no_caller_saved_registers attribute for helper calls bpf, x86, riscv, arm: no_caller_saved_registers for
bpf_get_smp_processor_id()
selftests/bpf: extract utility function for BPF disassembly selftests/bpf: print correct offset for pseudo calls in disasm_insn() selftests/bpf: no need to track next_match_pos in struct test_loader selftests/bpf: extract test_loader->expect_msgs as a data structure selftests/bpf: allow checking xlated programs in verifier_* tests selftests/bpf: __arch_* macro to limit test cases to specific archs selftests/bpf: test no_caller_saved_registers spill/fill removal
bpf: do check_nocsr_stack_contract() for ARG_ANYTHING helper params selftests/bpf: check nocsr contract for bpf_probe_read_kernel()

 include/linux/bpf.h                           |   6 +
 include/linux/bpf_verifier.h                  |  14 +
 kernel/bpf/helpers.c                          |   1 +
 kernel/bpf/verifier.c                         | 333 +++++++-
 tools/testing/selftests/bpf/Makefile          |   1 +
 tools/testing/selftests/bpf/disasm_helpers.c  |  69 ++
 tools/testing/selftests/bpf/disasm_helpers.h  |  12 +
 .../selftests/bpf/prog_tests/ctx_rewrite.c    |  74 +-
 .../selftests/bpf/prog_tests/verifier.c       |   2 +
 tools/testing/selftests/bpf/progs/bpf_misc.h  |  13 +
 .../selftests/bpf/progs/verifier_nocsr.c      | 794 ++++++++++++++++++
 tools/testing/selftests/bpf/test_loader.c     | 217 +++--
 tools/testing/selftests/bpf/test_progs.h      |   1 -
 tools/testing/selftests/bpf/testing_helpers.c |   1 +
 14 files changed, 1414 insertions(+), 124 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/disasm_helpers.c
 create mode 100644 tools/testing/selftests/bpf/disasm_helpers.h
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_nocsr.c
Signature · 1 line
2.45.2
author maintainer
#1 lore raw

PATCH v3 1/12 bpf: add a get_helper_proto() utility function

Extract the part of check_helper_call() as a utility function allowing to query 'struct bpf_func_proto' for a specific helper function id.

---
 kernel/bpf/verifier.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)
kernel/bpf/verifier.c +18 -7
index c0263fb5ca4b..3cef46134a51 100644
@@ -10265,6 +10265,19 @@ static void update_loop_inline_state(struct bpf_verifier_env *env, u32 subprogno
 				 state->callback_subprogno == subprogno);
 }
 
+static int get_helper_proto(struct bpf_verifier_env *env, int func_id,
+			    const struct bpf_func_proto **ptr)
+{
+	if (func_id < 0 || func_id >= __BPF_FUNC_MAX_ID)
+		return -ERANGE;
+
+	if (!env->ops->get_func_proto)
+		return -EINVAL;
+
+	*ptr = env->ops->get_func_proto(func_id, env->prog);
+	return *ptr ? 0 : -EINVAL;
+}
+
 static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 			     int *insn_idx_p)
 {
@@ -10281,18 +10294,16 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 
 	/* find function prototype */
 	func_id = insn->imm;
-	if (func_id < 0 || func_id >= __BPF_FUNC_MAX_ID) {
-		verbose(env, "invalid func %s#%d\n", func_id_name(func_id),
-			func_id);
+	err = get_helper_proto(env, insn->imm, &fn);
+	if (err == -ERANGE) {
+		verbose(env, "invalid func %s#%d\n", func_id_name(func_id), func_id);
 		return -EINVAL;
 	}
 
-	if (env->ops->get_func_proto)
-		fn = env->ops->get_func_proto(func_id, env->prog);
-	if (!fn) {
+	if (err) {
 		verbose(env, "program of this type cannot use helper %s#%d\n",
 			func_id_name(func_id), func_id);
-		return -EINVAL;
+		return err;
 	}
 
 	/* eBPF programs must be GPL compatible to use GPL-ed functions */
Signature · 1 line
2.45.2
author maintainer
#2 lore raw

PATCH v3 2/12 bpf: no_caller_saved_registers attribute for helper calls

In reply to Eduard Zingerman

GCC and LLVM define a no_caller_saved_registers function attribute. This attribute means that function scratches only some of the caller saved registers defined by ABI.
For BPF the set of such registers could be defined as follows:
- R0 is scratched only if function is non-void;
- R1-R5 are scratched only if corresponding parameter type is defined in the function prototype.

This commit introduces flag bpf_func_prot->allow_nocsr. If this flag is set for some helper function, verifier assumes that it follows no_caller_saved_registers calling convention.

The contract between kernel and clang allows to simultaneously use such functions and maintain backwards compatibility with old kernels that don't understand no_caller_saved_registers calls (nocsr for short):

- clang generates a simple pattern for nocsr calls, e.g.:

    r1 = 1;
    r2 = 2;
    *(u64 *)(r10 - 8)  = r1;
    *(u64 *)(r10 - 16) = r2;
    call %[to_be_inlined]
    r2 = *(u64 *)(r10 - 16);
    r1 = *(u64 *)(r10 - 8);
    r0 = r1;
    r0 += r2;
    exit;

- kernel removes unnecessary spills and fills, if called function is inlined by verifier or current JIT (with assumption that patch inserted by verifier or JIT honors nocsr contract, e.g. does not scratch r3-r5 for the example above), e.g. the code above would be transformed to:

    r1 = 1;
    r2 = 2;
    call %[to_be_inlined]
    r0 = r1;
    r0 += r2;
    exit;

Technically, the transformation is split into the following phases:
- function mark_nocsr_patterns(), called from bpf_check() searches and marks potential patterns in instruction auxiliary data;
- upon stack read or write access,
function check_nocsr_stack_contract() is used to verify if stack offsets, presumably reserved for nocsr patterns, are used only from those patterns;
- function remove_nocsr_spills_fills(), called from bpf_check(), applies the rewrite for valid patterns.

See comment in mark_nocsr_pattern_for_call() for more details.

---
 include/linux/bpf.h          |   6 +
 include/linux/bpf_verifier.h |  14 ++
 kernel/bpf/verifier.c        | 295 ++++++++++++++++++++++++++++++++++-
 3 files changed, 314 insertions(+), 1 deletion(-)
include/linux/bpf.h +6 -0
index 4f1d4a97b9d1..7640ab047188 100644
@@ -807,6 +807,12 @@ struct bpf_func_proto {
 	bool gpl_only;
 	bool pkt_access;
 	bool might_sleep;
+	/* set to true if helper follows contract for gcc/llvm
+	 * attribute no_caller_saved_registers:
+	 * - void functions do not scratch r0
+	 * - functions taking N arguments scratch only registers r1-rN
+	 */
+	bool allow_nocsr;
 	enum bpf_return_type ret_type;
 	union {
 		struct {
include/linux/bpf_verifier.h +14 -0
index e98ba5a5cf79..1c0543c64ccd 100644
@@ -572,6 +572,14 @@ struct bpf_insn_aux_data {
 	bool is_iter_next; /* bpf_iter_<type>_next() kfunc call */
 	bool call_with_percpu_alloc_ptr; /* {this,per}_cpu_ptr() with prog percpu alloc */
 	u8 alu_state; /* used in combination with alu_limit */
+	/* true if STX or LDX instruction is a part of a spill/fill
+	 * pattern for a no_caller_saved_registers call.
+	 */
+	u8 nocsr_pattern:1;
+	/* for CALL instructions, a number of spill/fill pairs in the
+	 * no_caller_saved_registers pattern.
+	 */
+	u8 nocsr_spills_num:3;
 
 	/* below fields are initialized once */
 	unsigned int orig_idx; /* original instruction index */
@@ -641,6 +649,10 @@ struct bpf_subprog_info {
 	u32 linfo_idx; /* The idx to the main_prog->aux->linfo */
 	u16 stack_depth; /* max. stack depth used by this function */
 	u16 stack_extra;
+	/* offsets in range [stack_depth .. nocsr_stack_off)
+	 * are used for no_caller_saved_registers spills and fills.
+	 */
+	s16 nocsr_stack_off;
 	bool has_tail_call: 1;
 	bool tail_call_reachable: 1;
 	bool has_ld_abs: 1;
@@ -648,6 +660,8 @@ struct bpf_subprog_info {
 	bool is_async_cb: 1;
 	bool is_exception_cb: 1;
 	bool args_cached: 1;
+	/* true if nocsr stack region is used by functions that can't be inlined */
+	bool keep_nocsr_stack: 1;
 
 	u8 arg_cnt;
 	struct bpf_subprog_arg_info args[MAX_BPF_FUNC_REG_ARGS];
kernel/bpf/verifier.c +294 -1

357 lines not on this page · raw message

Signature · 1 line
2.45.2
#3 lore raw

Re: [bpf-next v3 02/12] bpf: no_caller_saved_registers attribute for helper calls

kernel/bpf/verifier.c int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3 · line 22065

Quoted 10 lines · Eduard Zingerman
kernel/bpf/verifier.c +5 -0
@@ -21771,6 +22058,12 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
        if (ret == 0)
                ret = check_max_stack_depth(env);
 
+       /* might decrease stack depth, keep it before passes that
+        * allocate additional slots.
+        */
+       if (ret == 0)
+               ret = remove_nocsr_spills_fills(env);

Probably should be before check_max_stack_depth() above :)

author maintainer
#4 lore raw

Re: [bpf-next v3 02/12] bpf: no_caller_saved_registers attribute for helper calls

Quoted 6 lines · Alexei Starovoitov
Quoted 5 lines · Eduard Zingerman
kernel/bpf/verifier.c +0 -0
@@ -21771,6 +22058,12 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
        if (ret == 0)
                ret = check_max_stack_depth(env);
 
Quoted 7 lines

kernel/bpf/verifier.c int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3 · line 22065

Quoted 5 lines
kernel/bpf/verifier.c +5 -0
+       /* might decrease stack depth, keep it before passes that
+        * allocate additional slots.
+        */
+       if (ret == 0)
+               ret = remove_nocsr_spills_fills(env);

Probably should be before check_max_stack_depth() above :)

I thought about it, unfortunately, that would be a half-measure. There are two places where verifier reports stack depth errors:
- check_stack_access_within_bounds() checks for access outside [-MAX_BPF_STACK..0) region within one subprogram;
- check_max_stack_depth() checks accumulated stack depth across subprogram calls.

It is possible to move remove_nocsr_spills_fills() before check_max_stack_depth(), but check_stack_access_within_bounds() would still report errors for nocsr stack slots, because check_nocsr_stack_contract() and check_stack_access_within_bounds() are both invoked during main verification pass and contract validation is not yet finished.

#5 lore raw

Re: [bpf-next v3 02/12] bpf: no_caller_saved_registers attribute for helper calls

Quoted 23 lines · Eduard Zingerman
Quoted 6 lines · Alexei Starovoitov
Quoted 5 lines · Eduard Zingerman
kernel/bpf/verifier.c +0 -0
@@ -21771,6 +22058,12 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
        if (ret == 0)
                ret = check_max_stack_depth(env);
 
Quoted 7 lines

kernel/bpf/verifier.c int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3 · line 22065

Quoted 5 lines
kernel/bpf/verifier.c +5 -0
+       /* might decrease stack depth, keep it before passes that
+        * allocate additional slots.
+        */
+       if (ret == 0)
+               ret = remove_nocsr_spills_fills(env);

Probably should be before check_max_stack_depth() above :)

I thought about it, unfortunately, that would be a half-measure. There are two places where verifier reports stack depth errors:
- check_stack_access_within_bounds() checks for access outside [-MAX_BPF_STACK..0) region within one subprogram;
- check_max_stack_depth() checks accumulated stack depth across subprogram calls.

Quoted 6 lines

It is possible to move remove_nocsr_spills_fills() before check_max_stack_depth(), but check_stack_access_within_bounds() would still report errors for nocsr stack slots, because check_nocsr_stack_contract() and check_stack_access_within_bounds() are both invoked during main verification pass and contract validation is not yet finished.

Agree that it's a half measure, but it's still better than doing it after check_max_stack_depth().

We can also allow check_stack_access_within_bounds() to go above 512 for nocsr pattern. If spill/fill is later removed then great, if not then it's not a big deal to go slightly above 512 especially considering that private stack is coming in soon.

author maintainer
#6 lore raw

Re: [bpf-next v3 02/12] bpf: no_caller_saved_registers attribute for helper calls

On Fri, 2024-07-19 at 19:00 -0700, Alexei Starovoitov wrote:

[...]

Quoted 7 lines
Quoted 6 lines

It is possible to move remove_nocsr_spills_fills() before check_max_stack_depth(), but check_stack_access_within_bounds() would still report errors for nocsr stack slots, because check_nocsr_stack_contract() and check_stack_access_within_bounds() are both invoked during main verification pass and contract validation is not yet finished.

Quoted 7 lines

Agree that it's a half measure, but it's still better than doing it after check_max_stack_depth().

We can also allow check_stack_access_within_bounds() to go above 512 for nocsr pattern. If spill/fill is later removed then great, if not then it's not a big deal to go slightly above 512 especially considering that private stack is coming in soon.

Ok, I'm going to update check_stack_slot_within_bounds() to allow access for up to 512+48 (to account for all 6 nocsr registers) if accessing instruction is a spill/fill marked as nocsr pattern.

It is a speculative thing, because nocsr contract might be found void at some later point during program verification.
However check_max_stack_depth_subprog() would still catch access below -512 if nocsr spills are not removed. So, the worst case -- error report for stack usage would be less user friendly.

Don't think any other uses of MAX_BPF_STACK need an update, as all the rest seem to deal with registers representing stack pointers.

author maintainer
#7 lore raw

PATCH v3 3/12 bpf, x86, riscv, arm: no_caller_saved_registers for bpf_get_smp_processor_id()

In reply to Eduard Zingerman

The function bpf_get_smp_processor_id() is processed in a different way, depending on the arch:
- on x86 verifier replaces call to bpf_get_smp_processor_id() with a sequence of instructions that modify only r0;
- on riscv64 jit replaces call to bpf_get_smp_processor_id() with a sequence of instructions that modify only r0;
- on arm64 jit replaces call to bpf_get_smp_processor_id() with a sequence of instructions that modify only r0 and tmp registers.

These rewrites satisfy attribute no_caller_saved_registers contract. Allow rewrite of no_caller_saved_registers patterns for bpf_get_smp_processor_id() in order to use this function as a canary for no_caller_saved_registers tests.

---
 kernel/bpf/helpers.c  |  1 +
 kernel/bpf/verifier.c | 11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)
kernel/bpf/helpers.c +1 -0
index 5241ba671c5a..e7b4c059ebaf 100644
@@ -158,6 +158,7 @@ const struct bpf_func_proto bpf_get_smp_processor_id_proto = {
 	.func		= bpf_get_smp_processor_id,
 	.gpl_only	= false,
 	.ret_type	= RET_INTEGER,
+	.allow_nocsr	= true,
 };
 
 BPF_CALL_0(bpf_get_numa_node_id)
kernel/bpf/verifier.c +9 -2
index 163b6b0f2fa7..438daf36a694 100644
@@ -16014,7 +16014,14 @@ static u32 helper_nocsr_clobber_mask(const struct bpf_func_proto *fn)
  */
 static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm)
 {
-	return false;
+	switch (imm) {
+#ifdef CONFIG_X86_64
+	case BPF_FUNC_get_smp_processor_id:
+		return env->prog->jit_requested && bpf_jit_supports_percpu_insn();
+#endif
+	default:
+		return false;
+	}
 }
 
 /* GCC and LLVM define a no_caller_saved_registers function attribute.
@@ -20716,7 +20723,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 #if defined(CONFIG_X86_64) && !defined(CONFIG_UML)
 		/* Implement bpf_get_smp_processor_id() inline. */
 		if (insn->imm == BPF_FUNC_get_smp_processor_id &&
-		    prog->jit_requested && bpf_jit_supports_percpu_insn()) {
+		    verifier_inlines_helper_call(env, insn->imm)) {
 			/* BPF_FUNC_get_smp_processor_id inlining is an
 			 * optimization, so if pcpu_hot.cpu_number is ever
 			 * changed in some incompatible and hard to support
Signature · 1 line
2.45.2
author maintainer
#8 lore raw

PATCH v3 4/12 selftests/bpf: extract utility function for BPF disassembly

In reply to Eduard Zingerman

struct bpf_insn *disasm_insn(struct bpf_insn *insn, char *buf, size_t buf_sz);

Disassembles instruction 'insn' to a text buffer 'buf'. Removes insn->code hex prefix added by kernel disassembly routine. Returns a pointer to the next instruction
(increments insn by either 1 or 2).

---
 tools/testing/selftests/bpf/Makefile          |  1 +
 tools/testing/selftests/bpf/disasm_helpers.c  | 51 +++++++++++++
 tools/testing/selftests/bpf/disasm_helpers.h  | 12 +++
 .../selftests/bpf/prog_tests/ctx_rewrite.c    | 74 +++----------------
 tools/testing/selftests/bpf/testing_helpers.c |  1 +
 5 files changed, 75 insertions(+), 64 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/disasm_helpers.c
 create mode 100644 tools/testing/selftests/bpf/disasm_helpers.h
tools/testing/selftests/bpf/Makefile +1 -0
index dd49c1d23a60..fcacc693ed8a 100644
@@ -636,6 +636,7 @@ TRUNNER_EXTRA_SOURCES := test_progs.c		\
 			 test_loader.c		\
 			 xsk.c			\
 			 disasm.c		\
+			 disasm_helpers.c	\
 			 json_writer.c 		\
 			 flow_dissector_load.h	\
 			 ip_check_defrag_frags.h
tools/testing/selftests/bpf/disasm_helpers.c new file +51 -0

51 lines not on this page · raw message

tools/testing/selftests/bpf/disasm_helpers.h new file +12 -0
new file mode 100644
index 000000000000..7b26cab70099
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
+
+#ifndef __DISASM_HELPERS_H
+#define __DISASM_HELPERS_H
+
+#include <stdlib.h>
+
+struct bpf_insn;
+
+struct bpf_insn *disasm_insn(struct bpf_insn *insn, char *buf, size_t buf_sz);
+
+#endif /* __DISASM_HELPERS_H */
tools/testing/selftests/bpf/prog_tests/ctx_rewrite.c +10 -64

101 lines not on this page · raw message

tools/testing/selftests/bpf/testing_helpers.c +1 -0
index d5379a0e6da8..ac7c66f4fc7b 100644
@@ -7,6 +7,7 @@
 #include <errno.h>
 #include <bpf/bpf.h>
 #include <bpf/libbpf.h>
+#include "disasm.h"
 #include "test_progs.h"
 #include "testing_helpers.h"
 #include <linux/membarrier.h>
Signature · 1 line
2.45.2
author maintainer
#9 lore raw

PATCH v3 5/12 selftests/bpf: print correct offset for pseudo calls in disasm_insn()

In reply to Eduard Zingerman

Adjust disasm_helpers.c:disasm_insn() to account for the following part of the verifier.c:jit_subprogs:

  for (i = 0, insn = prog->insnsi; i < prog->len; i++, insn++) {
        /* ... */
        if (!bpf_pseudo_call(insn))
                continue;
        insn->off = env->insn_aux_data[i].call_imm;
        subprog = find_subprog(env, i + insn->off + 1);
        insn->imm = subprog;
  }

Where verifier moves offset of the subprogram to the insn->off field.

---
 tools/testing/selftests/bpf/disasm_helpers.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
tools/testing/selftests/bpf/disasm_helpers.c +18 -0
index 96b1f2ffe438..f529f1c8c171 100644
@@ -4,6 +4,7 @@
 #include "disasm.h"
 
 struct print_insn_context {
+	char scratch[16];
 	char *buf;
 	size_t sz;
 };
@@ -18,6 +19,22 @@ static void print_insn_cb(void *private_data, const char *fmt, ...)
 	va_end(args);
 }
 
+static const char *print_call_cb(void *private_data, const struct bpf_insn *insn)
+{
+	struct print_insn_context *ctx = private_data;
+
+	/* For pseudo calls verifier.c:jit_subprogs() hides original
+	 * imm to insn->off and changes insn->imm to be an index of
+	 * the subprog instead.
+	 */
+	if (insn->src_reg == BPF_PSEUDO_CALL) {
+		snprintf(ctx->scratch, sizeof(ctx->scratch), "%+d", insn->off);
+		return ctx->scratch;
+	}
+
+	return NULL;
+}
+
 struct bpf_insn *disasm_insn(struct bpf_insn *insn, char *buf, size_t buf_sz)
 {
 	struct print_insn_context ctx = {
@@ -26,6 +43,7 @@ struct bpf_insn *disasm_insn(struct bpf_insn *insn, char *buf, size_t buf_sz)
 	};
 	struct bpf_insn_cbs cbs = {
 		.cb_print	= print_insn_cb,
+		.cb_call	= print_call_cb,
 		.private_data	= &ctx,
 	};
 	char *tmp, *pfx_end, *sfx_start;
Signature · 1 line
2.45.2
author maintainer
#10 lore raw

PATCH v3 6/12 selftests/bpf: no need to track next_match_pos in struct test_loader

In reply to Eduard Zingerman

The call stack for validate_case() function looks as follows:
- test_loader__run_subtests()
- process_subtest()
- run_subtest()
- prepare_case(), which does 'tester->next_match_pos = 0';
- validate_case(), which increments tester->next_match_pos.

Hence, each subtest is run with next_match_pos freshly set to zero. Meaning that there is no need to persist this variable in the struct test_loader, use local variable instead.

---
 tools/testing/selftests/bpf/test_loader.c | 19 ++++++++-----------
 tools/testing/selftests/bpf/test_progs.h  |  1 -
 2 files changed, 8 insertions(+), 12 deletions(-)
tools/testing/selftests/bpf/test_loader.c +8 -11
index f14e10b0de96..47508cf66e89 100644
@@ -434,7 +434,6 @@ static void prepare_case(struct test_loader *tester,
 	bpf_program__set_flags(prog, prog_flags | spec->prog_flags);
 
 	tester->log_buf[0] = '\0';
-	tester->next_match_pos = 0;
 }
 
 static void emit_verifier_log(const char *log_buf, bool force)
@@ -450,25 +449,23 @@ static void validate_case(struct test_loader *tester,
 			  struct bpf_program *prog,
 			  int load_err)
 {
-	int i, j, err;
-	char *match;
 	regmatch_t reg_match[1];
+	const char *log = tester->log_buf;
+	int i, j, err;
 
 	for (i = 0; i < subspec->expect_msg_cnt; i++) {
 		struct expect_msg *msg = &subspec->expect_msgs[i];
+		const char *match = NULL;
 
 		if (msg->substr) {
-			match = strstr(tester->log_buf + tester->next_match_pos, msg->substr);
+			match = strstr(log, msg->substr);
 			if (match)
-				tester->next_match_pos = match - tester->log_buf + strlen(msg->substr);
+				log += strlen(msg->substr);
 		} else {
-			err = regexec(&msg->regex,
-				      tester->log_buf + tester->next_match_pos, 1, reg_match, 0);
+			err = regexec(&msg->regex, log, 1, reg_match, 0);
 			if (err == 0) {
-				match = tester->log_buf + tester->next_match_pos + reg_match[0].rm_so;
-				tester->next_match_pos += reg_match[0].rm_eo;
-			} else {
-				match = NULL;
+				match = log + reg_match[0].rm_so;
+				log += reg_match[0].rm_eo;
 			}
 		}
 
tools/testing/selftests/bpf/test_progs.h +0 -1
index 51341d50213b..b1e949fb16cf 100644
@@ -447,7 +447,6 @@ typedef int (*pre_execution_cb)(struct bpf_object *obj);
 struct test_loader {
 	char *log_buf;
 	size_t log_buf_sz;
-	size_t next_match_pos;
 	pre_execution_cb pre_execution_cb;
 
 	struct bpf_object *obj;
Signature · 1 line
2.45.2
author maintainer
#11 lore raw

PATCH v3 7/12 selftests/bpf: extract test_loader->expect_msgs as a data structure

In reply to Eduard Zingerman

Non-functional change: use a separate data structure to represented expected messages in test_loader.
This would allow to use the same functionality for expected set of disassembled instructions in the follow-up commit.

---
 tools/testing/selftests/bpf/test_loader.c | 81 ++++++++++++-----------
 1 file changed, 41 insertions(+), 40 deletions(-)
tools/testing/selftests/bpf/test_loader.c +41 -40
index 47508cf66e89..3f84903558dd 100644
@@ -55,11 +55,15 @@ struct expect_msg {
 	regex_t regex;
 };
 
+struct expected_msgs {
+	struct expect_msg *patterns;
+	size_t cnt;
+};
+
 struct test_subspec {
 	char *name;
 	bool expect_failure;
-	struct expect_msg *expect_msgs;
-	size_t expect_msg_cnt;
+	struct expected_msgs expect_msgs;
 	int retval;
 	bool execute;
 };
@@ -96,44 +100,45 @@ void test_loader_fini(struct test_loader *tester)
 	free(tester->log_buf);
 }
 
-static void free_test_spec(struct test_spec *spec)
+static void free_msgs(struct expected_msgs *msgs)
 {
 	int i;
 
+	for (i = 0; i < msgs->cnt; i++)
+		if (msgs->patterns[i].regex_str)
+			regfree(&msgs->patterns[i].regex);
+	free(msgs->patterns);
+	msgs->patterns = NULL;
+	msgs->cnt = 0;
+}
+
+static void free_test_spec(struct test_spec *spec)
+{
 	/* Deallocate expect_msgs arrays. */
-	for (i = 0; i < spec->priv.expect_msg_cnt; i++)
-		if (spec->priv.expect_msgs[i].regex_str)
-			regfree(&spec->priv.expect_msgs[i].regex);
-	for (i = 0; i < spec->unpriv.expect_msg_cnt; i++)
-		if (spec->unpriv.expect_msgs[i].regex_str)
-			regfree(&spec->unpriv.expect_msgs[i].regex);
+	free_msgs(&spec->priv.expect_msgs);
+	free_msgs(&spec->unpriv.expect_msgs);
 
 	free(spec->priv.name);
 	free(spec->unpriv.name);
-	free(spec->priv.expect_msgs);
-	free(spec->unpriv.expect_msgs);
-
 	spec->priv.name = NULL;
 	spec->unpriv.name = NULL;
-	spec->priv.expect_msgs = NULL;
-	spec->unpriv.expect_msgs = NULL;
 }
 
-static int push_msg(const char *substr, const char *regex_str, struct test_subspec *subspec)
+static int push_msg(const char *substr, const char *regex_str, struct expected_msgs *msgs)
 {
 	void *tmp;
 	int regcomp_res;
 	char error_msg[100];
 	struct expect_msg *msg;
 
-	tmp = realloc(subspec->expect_msgs,
-		      (1 + subspec->expect_msg_cnt) * sizeof(struct expect_msg));
+	tmp = realloc(msgs->patterns,
+		      (1 + msgs->cnt) * sizeof(struct expect_msg));
 	if (!tmp) {
 		ASSERT_FAIL("failed to realloc memory for messages\n");
 		return -ENOMEM;
 	}
-	subspec->expect_msgs = tmp;
-	msg = &subspec->expect_msgs[subspec->expect_msg_cnt];
+	msgs->patterns = tmp;
+	msg = &msgs->patterns[msgs->cnt];
 
 	if (substr) {
 		msg->substr = substr;
@@ -150,7 +155,7 @@ static int push_msg(const char *substr, const char *regex_str, struct test_subsp
 		}
 	}
 
-	subspec->expect_msg_cnt += 1;
+	msgs->cnt += 1;
 	return 0;
 }
 
@@ -272,25 +277,25 @@ 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);
+			err = push_msg(msg, NULL, &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);
+			err = push_msg(msg, NULL, &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);
+			err = push_msg(NULL, 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);
+			err = push_msg(NULL, msg, &spec->unpriv.expect_msgs);
 			if (err)
 				goto cleanup;
 			spec->mode_mask |= UNPRIV;
@@ -387,11 +392,12 @@ static int parse_test_spec(struct test_loader *tester,
 			spec->unpriv.execute = spec->priv.execute;
 		}
 
-		if (!spec->unpriv.expect_msgs) {
-			for (i = 0; i < spec->priv.expect_msg_cnt; i++) {
-				struct expect_msg *msg = &spec->priv.expect_msgs[i];
+		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);
+				err = push_msg(msg->substr, msg->regex_str,
+					       &spec->unpriv.expect_msgs);
 				if (err)
 					goto cleanup;
 			}
@@ -443,18 +449,14 @@ static void emit_verifier_log(const char *log_buf, bool force)
 	fprintf(stdout, "VERIFIER LOG:\n=============\n%s=============\n", log_buf);
 }
 
-static void validate_case(struct test_loader *tester,
-			  struct test_subspec *subspec,
-			  struct bpf_object *obj,
-			  struct bpf_program *prog,
-			  int load_err)
+static void validate_msgs(char *log_buf, struct expected_msgs *msgs)
 {
 	regmatch_t reg_match[1];
-	const char *log = tester->log_buf;
+	const char *log = log_buf;
 	int i, j, err;
 
-	for (i = 0; i < subspec->expect_msg_cnt; i++) {
-		struct expect_msg *msg = &subspec->expect_msgs[i];
+	for (i = 0; i < msgs->cnt; i++) {
+		struct expect_msg *msg = &msgs->patterns[i];
 		const char *match = NULL;
 
 		if (msg->substr) {
@@ -471,9 +473,9 @@ static void validate_case(struct test_loader *tester,
 
 		if (!ASSERT_OK_PTR(match, "expect_msg")) {
 			if (env.verbosity == VERBOSE_NONE)
-				emit_verifier_log(tester->log_buf, true /*force*/);
+				emit_verifier_log(log_buf, true /*force*/);
 			for (j = 0; j <= i; j++) {
-				msg = &subspec->expect_msgs[j];
+				msg = &msgs->patterns[j];
 				fprintf(stderr, "%s %s: '%s'\n",
 					j < i ? "MATCHED " : "EXPECTED",
 					msg->substr ? "SUBSTR" : " REGEX",
@@ -692,9 +694,8 @@ void run_subtest(struct test_loader *tester,
 			goto tobj_cleanup;
 		}
 	}
-
 	emit_verifier_log(tester->log_buf, false /*force*/);
-	validate_case(tester, subspec, tobj, tprog, err);
+	validate_msgs(tester->log_buf, &subspec->expect_msgs);
 
 	if (should_do_test_run(spec, subspec)) {
 		/* For some reason test_verifier executes programs
Signature · 1 line
2.45.2
author maintainer
#12 lore raw

PATCH v3 8/12 selftests/bpf: allow checking xlated programs in verifier_* tests

In reply to Eduard Zingerman

Add a macro __xlated("...") for use with test_loader tests.

When such annotations are present for the test case:
- bpf_prog_get_info_by_fd() is used to get BPF program after all rewrites are applied by verifier.
- the program is disassembled and patterns specified in __xlated are searched for in the disassembly text.

__xlated matching follows the same mechanics as __msg:
each subsequent pattern is matched from the point where previous pattern ended.

This allows to write tests like below, where the goal is to verify the behavior of one of the of the transformations applied by verifier:

    SEC("raw_tp")
    __xlated("1: w0 = ")
    __xlated("2: r0 = &(void __percpu *)(r0)")
    __xlated("3: r0 = *(u32 *)(r0 +0)")
    __xlated("4: exit")
    __success __naked void simple(void)
    {
            asm volatile (
            "call %[bpf_get_smp_processor_id];"
            "exit;"
            :
            : __imm(bpf_get_smp_processor_id)
            : __clobber_all);
    }
---
 tools/testing/selftests/bpf/progs/bpf_misc.h |  5 ++
 tools/testing/selftests/bpf/test_loader.c    | 82 +++++++++++++++++++-
 2 files changed, 84 insertions(+), 3 deletions(-)
tools/testing/selftests/bpf/progs/bpf_misc.h +5 -0
index 81097a3f15eb..a70939c7bc26 100644
@@ -26,6 +26,9 @@
  *
  * __regex           Same as __msg, but using a regular expression.
  * __regex_unpriv    Same as __msg_unpriv but using a regular expression.
+ * __xlated          Expect a line in a disassembly log after verifier applies rewrites.
+ *                   Multiple __xlated attributes could be specified.
+ * __xlated_unpriv   Same as __xlated but for unprivileged mode.
  *
  * __success         Expect program load success in privileged mode.
  * __success_unpriv  Expect program load success in unprivileged mode.
@@ -63,11 +66,13 @@
  */
 #define __msg(msg)		__attribute__((btf_decl_tag("comment:test_expect_msg=" msg)))
 #define __regex(regex)		__attribute__((btf_decl_tag("comment:test_expect_regex=" regex)))
+#define __xlated(msg)		__attribute__((btf_decl_tag("comment:test_expect_xlated=" msg)))
 #define __failure		__attribute__((btf_decl_tag("comment:test_expect_failure")))
 #define __success		__attribute__((btf_decl_tag("comment:test_expect_success")))
 #define __description(desc)	__attribute__((btf_decl_tag("comment:test_description=" desc)))
 #define __msg_unpriv(msg)	__attribute__((btf_decl_tag("comment:test_expect_msg_unpriv=" msg)))
 #define __regex_unpriv(regex)	__attribute__((btf_decl_tag("comment:test_expect_regex_unpriv=" regex)))
+#define __xlated_unpriv(msg)	__attribute__((btf_decl_tag("comment:test_expect_xlated_unpriv=" msg)))
 #define __failure_unpriv	__attribute__((btf_decl_tag("comment:test_expect_failure_unpriv")))
 #define __success_unpriv	__attribute__((btf_decl_tag("comment:test_expect_success_unpriv")))
 #define __log_level(lvl)	__attribute__((btf_decl_tag("comment:test_log_level="#lvl)))
tools/testing/selftests/bpf/test_loader.c +79 -3

146 lines not on this page · raw message

Signature · 1 line
2.45.2
author maintainer
#13 lore raw

PATCH v3 9/12 selftests/bpf: __arch_* macro to limit test cases to specific archs

In reply to Eduard Zingerman

Add annotations __arch_x86_64, __arch_arm64, __arch_riscv64 to specify on which architecture the test case should be tested. Several __arch_* annotations could be specified at once. When test case is not run on current arch it is marked as skipped.

For example, the following would be tested only on arm64 and riscv64:

  SEC("raw_tp")
  __arch_arm64
  __arch_riscv64
  __xlated("1: *(u64 *)(r10 - 16) = r1")
  __xlated("2: call")
  __xlated("3: r1 = *(u64 *)(r10 - 16);")
  __success
  __naked void canary_arm64_riscv64(void)
  {
  	asm volatile (
  	"r1 = 1;"
  	"*(u64 *)(r10 - 16) = r1;"
  	"call %[bpf_get_smp_processor_id];"
  	"r1 = *(u64 *)(r10 - 16);"
  	"exit;"
  	:
  	: __imm(bpf_get_smp_processor_id)
  	: __clobber_all);
  }

On x86 it would be skipped:

#467/2 verifier_nocsr/canary_arm64_riscv64:SKIP

---
 tools/testing/selftests/bpf/progs/bpf_misc.h |  8 ++++
 tools/testing/selftests/bpf/test_loader.c    | 43 ++++++++++++++++++++
 2 files changed, 51 insertions(+)
tools/testing/selftests/bpf/progs/bpf_misc.h +8 -0
index a70939c7bc26..a225cd87897c 100644
@@ -63,6 +63,10 @@
  * __auxiliary         Annotated program is not a separate test, but used as auxiliary
  *                     for some other test cases and should always be loaded.
  * __auxiliary_unpriv  Same, but load program in unprivileged mode.
+ *
+ * __arch_*          Specify on which architecture the test case should be tested.
+ *                   Several __arch_* annotations could be specified at once.
+ *                   When test case is not run on current arch it is marked as skipped.
  */
 #define __msg(msg)		__attribute__((btf_decl_tag("comment:test_expect_msg=" msg)))
 #define __regex(regex)		__attribute__((btf_decl_tag("comment:test_expect_regex=" regex)))
@@ -82,6 +86,10 @@
 #define __auxiliary		__attribute__((btf_decl_tag("comment:test_auxiliary")))
 #define __auxiliary_unpriv	__attribute__((btf_decl_tag("comment:test_auxiliary_unpriv")))
 #define __btf_path(path)	__attribute__((btf_decl_tag("comment:test_btf_path=" path)))
+#define __arch(arch)		__attribute__((btf_decl_tag("comment:test_arch=" arch)))
+#define __arch_x86_64		__arch("X86_64")
+#define __arch_arm64		__arch("ARM64")
+#define __arch_riscv64		__arch("RISCV64")
 
 /* Convenience macro for use with 'asm volatile' blocks */
 #define __naked __attribute__((naked))
tools/testing/selftests/bpf/test_loader.c +43 -0

90 lines not on this page · raw message

Signature · 1 line
2.45.2
author maintainer
#14 lore raw

PATCH v3 10/12 selftests/bpf: test no_caller_saved_registers spill/fill removal

In reply to Eduard Zingerman

Tests for no_caller_saved_registers processing logic
(see verifier.c:match_and_mark_nocsr_pattern()):
- a canary positive test case;
- a canary test case for arm64 and riscv64;
- various tests with broken patterns;
- tests with read/write fixed/varying stack access that violate nocsr stack access contract;
- tests with multiple subprograms;
- tests using nocsr in combination with may_goto/bpf_loop, as all of these features affect stack depth.

---
 .../selftests/bpf/prog_tests/verifier.c       |   2 +
 .../selftests/bpf/progs/verifier_nocsr.c      | 705 ++++++++++++++++++
 2 files changed, 707 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_nocsr.c
tools/testing/selftests/bpf/prog_tests/verifier.c +2 -0
index 9dc3687bc406..a3c2c5da3e0e 100644
@@ -53,6 +53,7 @@
 #include "verifier_movsx.skel.h"
 #include "verifier_netfilter_ctx.skel.h"
 #include "verifier_netfilter_retcode.skel.h"
+#include "verifier_nocsr.skel.h"
 #include "verifier_or_jmp32_k.skel.h"
 #include "verifier_precision.skel.h"
 #include "verifier_prevent_map_lookup.skel.h"
@@ -172,6 +173,7 @@ void test_verifier_meta_access(void)          { RUN(verifier_meta_access); }
 void test_verifier_movsx(void)                 { RUN(verifier_movsx); }
 void test_verifier_netfilter_ctx(void)        { RUN(verifier_netfilter_ctx); }
 void test_verifier_netfilter_retcode(void)    { RUN(verifier_netfilter_retcode); }
+void test_verifier_nocsr(void)                { RUN(verifier_nocsr); }
 void test_verifier_or_jmp32_k(void)           { RUN(verifier_or_jmp32_k); }
 void test_verifier_precision(void)            { RUN(verifier_precision); }
 void test_verifier_prevent_map_lookup(void)   { RUN(verifier_prevent_map_lookup); }
tools/testing/selftests/bpf/progs/verifier_nocsr.c new file +705 -0

705 lines not on this page · raw message

Signature · 1 line
2.45.2
author maintainer
#15 lore raw

PATCH v3 11/12 bpf: do check_nocsr_stack_contract() for ARG_ANYTHING helper params

In reply to Eduard Zingerman

There is a number of BPF helper functions that use ARG_ANYTHING to mark parameters that are used as memory addresses.
An address of BPF stack slot could be passed as such parameter for two such helper functions:
- bpf_probe_read_kernel
- bpf_probe_read_kernel_str

This might lead to a surprising behavior in combination with nocsr rewrites, e.g. consider the program below:

     1: r1 = 1;
        /* nocsr pattern with stack offset -16 */
     2: *(u64 *)(r10 - 16) = r1;
     3: call %[bpf_get_smp_processor_id];
     4: r1 = *(u64 *)(r10 - 16);
     5: r1 = r10;
     6: r1 += -8;
     7: r2 = 1;
     8: r3 = r10;
     9: r3 += -16;
        /* bpf_probe_read_kernel(dst: &fp[-8], size: 1, src: &fp[-16]) */
    10: call %[bpf_probe_read_kernel];
    11: exit;

Here nocsr rewrite logic would remove instructions (2) and (4). However, (2) writes a value that is later read by a call at (10).

Function check_func_arg() is called from check_helper_call() and is responsible for memory access checks, when helper argument type is declared as ARG_PTR_TO_... .

However, for ARG_ANYTHING this function returns early and does not call check_stack_{read,write}().

This patch opts to add a check_nocsr_stack_contract() to the ARG_ANYTHING return path if passed parameter is a pointer to stack.

---
 kernel/bpf/verifier.c | 4 ++++
 1 file changed, 4 insertions(+)
kernel/bpf/verifier.c +4 -0
index 438daf36a694..77affc563a64 100644
@@ -8684,11 +8684,15 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 		return err;
 
 	if (arg_type == ARG_ANYTHING) {
+		/* return value depends on env->allow_ptr_leaks */
 		if (is_pointer_value(env, regno)) {
 			verbose(env, "R%d leaks addr into helper function\n",
 				regno);
 			return -EACCES;
 		}
+		if (reg->type == PTR_TO_STACK)
+			check_nocsr_stack_contract(env, cur_func(env), insn_idx,
+						   reg->smin_value + reg->off);
 		return 0;
 	}
 
Signature · 1 line
2.45.2
#16 lore raw

Re: [bpf-next v3 11/12] bpf: do check_nocsr_stack_contract() for ARG_ANYTHING helper params

Quoted 25 lines · Eduard Zingerman

There is a number of BPF helper functions that use ARG_ANYTHING to mark parameters that are used as memory addresses.
An address of BPF stack slot could be passed as such parameter for two such helper functions:
- bpf_probe_read_kernel
- bpf_probe_read_kernel_str

This might lead to a surprising behavior in combination with nocsr rewrites, e.g. consider the program below:

     1: r1 = 1;
        /* nocsr pattern with stack offset -16 */
     2: *(u64 *)(r10 - 16) = r1;
     3: call %[bpf_get_smp_processor_id];
     4: r1 = *(u64 *)(r10 - 16);
     5: r1 = r10;
     6: r1 += -8;
     7: r2 = 1;
     8: r3 = r10;
     9: r3 += -16;
        /* bpf_probe_read_kernel(dst: &fp[-8], size: 1, src: &fp[-16]) */
    10: call %[bpf_probe_read_kernel];
    11: exit;
Quoted 2 lines

Here nocsr rewrite logic would remove instructions (2) and (4). However, (2) writes a value that is later read by a call at (10).

This makes no sense to me.
This bpf prog is broken.
If probe_read is used to read stack it will read garbage. JITs and the verifier are allowed to do any transformation that keeps the program semantics and safety.

pw-bot: cr

author maintainer
#17 lore raw

Re: [bpf-next v3 11/12] bpf: do check_nocsr_stack_contract() for ARG_ANYTHING helper params

Quoted 1 line · Alexei Starovoitov

On Mon, Jul 15, 2024 at 4:02 PM Eduard Zingerman <[email protected]> wrote:

[...]

Quoted 17 lines
Quoted 17 lines

This might lead to a surprising behavior in combination with nocsr rewrites, e.g. consider the program below:

     1: r1 = 1;
        /* nocsr pattern with stack offset -16 */
     2: *(u64 *)(r10 - 16) = r1;
     3: call %[bpf_get_smp_processor_id];
     4: r1 = *(u64 *)(r10 - 16);
     5: r1 = r10;
     6: r1 += -8;
     7: r2 = 1;
     8: r3 = r10;
     9: r3 += -16;
        /* bpf_probe_read_kernel(dst: &fp[-8], size: 1, src: &fp[-16]) */
    10: call %[bpf_probe_read_kernel];
    11: exit;
Quoted 8 lines
Quoted 2 lines

Here nocsr rewrite logic would remove instructions (2) and (4). However, (2) writes a value that is later read by a call at (10).

This makes no sense to me.
This bpf prog is broken.
If probe_read is used to read stack it will read garbage. JITs and the verifier are allowed to do any transformation that keeps the program semantics and safety.

I tried to run the following program
(should have run it earlier):

SEC("raw_tp")
__retval(42)
__success
int bpf_probe_read_kernel_stack_ptr(void *ctx)

{
	unsigned long a = 17;
	unsigned long b = 42;
	int err;

	err = bpf_probe_read_kernel(&a, 8, &b);
	if (err)
		return -1;
	return a;
}

And indeed, it does not produce expected result,
the retval varies around 22079.
However, I don't really understand why this program is broken. E.g. from C compiler pov pointer &b escapes, and compiler is not really allowed to replace object at that offset with garbage. Is it a limitation of the bpf_probe_read_kernel() that it cannot read BPF program stack?

author maintainer
#18 lore raw

Re: [bpf-next v3 11/12] bpf: do check_nocsr_stack_contract() for ARG_ANYTHING helper params

Quoted 22 lines · Eduard Zingerman
Quoted 1 line · Alexei Starovoitov

On Mon, Jul 15, 2024 at 4:02 PM Eduard Zingerman <[email protected]> wrote:

[...]

Quoted 17 lines
Quoted 17 lines

This might lead to a surprising behavior in combination with nocsr rewrites, e.g. consider the program below:

     1: r1 = 1;
        /* nocsr pattern with stack offset -16 */
     2: *(u64 *)(r10 - 16) = r1;
     3: call %[bpf_get_smp_processor_id];
     4: r1 = *(u64 *)(r10 - 16);
     5: r1 = r10;
     6: r1 += -8;
     7: r2 = 1;
     8: r3 = r10;
     9: r3 += -16;
        /* bpf_probe_read_kernel(dst: &fp[-8], size: 1, src: &fp[-16]) */
    10: call %[bpf_probe_read_kernel];
    11: exit;
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines

Here nocsr rewrite logic would remove instructions (2) and (4). However, (2) writes a value that is later read by a call at (10).

This makes no sense to me.
This bpf prog is broken.
If probe_read is used to read stack it will read garbage. JITs and the verifier are allowed to do any transformation that keeps the program semantics and safety.

Ok, my bad, the following program works at the moment:

SEC("socket") // <---- used wrong program type
__retval(42)
__success
int bpf_probe_read_kernel_stack_ptr(void *ctx)

{
	unsigned long a = 17;
	unsigned long b = 42;
	int err;

	err = bpf_probe_read_kernel(&a, 8, &b);
	if (err)
		return 1;
	return a;
}

And it is compiled to BPF as one would expect:

       ... fp[-8,-16] setup ...
       4:	r1 = r10
       5:	r1 += -0x8
       6:	r3 = r10
       7:	r3 += -0x10
       8:	w2 = 0x8
       9:	call 0x71
       ... return check ...

So, the point stands: from C compiler pov pointer &b escapes, and compiler is not really allowed to replace object at that offset with garbage. Why do you think the program is broken?

I don't mind dropping the patch in question, but I agree with Andrii's viewpoint that there is nothing wrong with this use case.

#19 lore raw

Re: [bpf-next v3 11/12] bpf: do check_nocsr_stack_contract() for ARG_ANYTHING helper params

Quoted 61 lines · Eduard Zingerman
Quoted 22 lines · Eduard Zingerman
Quoted 1 line · Alexei Starovoitov

On Mon, Jul 15, 2024 at 4:02 PM Eduard Zingerman <[email protected]> wrote:

[...]

Quoted 17 lines
Quoted 17 lines

This might lead to a surprising behavior in combination with nocsr rewrites, e.g. consider the program below:

     1: r1 = 1;
        /* nocsr pattern with stack offset -16 */
     2: *(u64 *)(r10 - 16) = r1;
     3: call %[bpf_get_smp_processor_id];
     4: r1 = *(u64 *)(r10 - 16);
     5: r1 = r10;
     6: r1 += -8;
     7: r2 = 1;
     8: r3 = r10;
     9: r3 += -16;
        /* bpf_probe_read_kernel(dst: &fp[-8], size: 1, src: &fp[-16]) */
    10: call %[bpf_probe_read_kernel];
    11: exit;
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines

Here nocsr rewrite logic would remove instructions (2) and (4). However, (2) writes a value that is later read by a call at (10).

This makes no sense to me.
This bpf prog is broken.
If probe_read is used to read stack it will read garbage. JITs and the verifier are allowed to do any transformation that keeps the program semantics and safety.

Ok, my bad, the following program works at the moment:

SEC("socket") // <---- used wrong program type
__retval(42)
__success
int bpf_probe_read_kernel_stack_ptr(void *ctx)

{
        unsigned long a = 17;
        unsigned long b = 42;
        int err;

        err = bpf_probe_read_kernel(&a, 8, &b);
        if (err)
                return 1;
        return a;
}

And it is compiled to BPF as one would expect:

       ... fp[-8,-16] setup ...
       4:       r1 = r10
       5:       r1 += -0x8
       6:       r3 = r10
       7:       r3 += -0x10
       8:       w2 = 0x8
       9:       call 0x71
       ... return check ...
Quoted 3 lines

So, the point stands: from C compiler pov pointer &b escapes, and compiler is not really allowed to replace object at that offset with garbage. Why do you think the program is broken?

This is apples to oranges.
Compiler sees that the address of 'b' is taken and passed into a function with side effect.
Whether 3rd arg of bpf_probe_read_kernel() is void * or long is irrelevant. Compilers will do it, because it's a C language requirement.

Quoted 2 lines

I don't mind dropping the patch in question, but I agree with Andrii's viewpoint that there is nothing wrong with this use case.

bpf_probe_read_kernel() is not special and it's 3rd argument is some kernel address. Whether it's stack pointer or anything else is irrelevant.
JITs and verifier are allowed to do any optimizations on stack and any other memory completely ignoring presence of bpf_probe_read_kernel() and what is being passed into it.

Tomorrow we will teach arm64 JIT to replace stack spill/fill with spare register read/write. There is no way we're going to special case a particular fp-16 slot because fp-16 was passed into probe_read.

author maintainer
#20 lore raw

Re: [bpf-next v3 11/12] bpf: do check_nocsr_stack_contract() for ARG_ANYTHING helper params

On Fri, 2024-07-19 at 18:54 -0700, Alexei Starovoitov wrote:

[...]

Quoted 21 lines
Quoted 3 lines

So, the point stands: from C compiler pov pointer &b escapes, and compiler is not really allowed to replace object at that offset with garbage. Why do you think the program is broken?

This is apples to oranges.
Compiler sees that the address of 'b' is taken and passed into a function with side effect.
Whether 3rd arg of bpf_probe_read_kernel() is void * or long is irrelevant. Compilers will do it, because it's a C language requirement.

Quoted 2 lines

I don't mind dropping the patch in question, but I agree with Andrii's viewpoint that there is nothing wrong with this use case.

bpf_probe_read_kernel() is not special and it's 3rd argument is some kernel address. Whether it's stack pointer or anything else is irrelevant.
JITs and verifier are allowed to do any optimizations on stack and any other memory completely ignoring presence of bpf_probe_read_kernel() and what is being passed into it.

Quoted 3 lines

Tomorrow we will teach arm64 JIT to replace stack spill/fill with spare register read/write. There is no way we're going to special case a particular fp-16 slot because fp-16 was passed into probe_read.

Ok, I will re-submit w/o these two patches.
Andrii also requested to re-structure the check contract function to reset .nocsr_pattern and .spills_num marks to 0 upon contract violation.

author maintainer
#21 lore raw

PATCH v3 12/12 selftests/bpf: check nocsr contract for bpf_probe_read_kernel()

In reply to Eduard Zingerman

Check that nocsr contract is enforced for bpf_probe_read_kernel() and bpf_probe_read_kernel_str(). These functions access memory via parameter with type ARG_ANYTHING.

---
 .../selftests/bpf/progs/verifier_nocsr.c      | 89 +++++++++++++++++++
 1 file changed, 89 insertions(+)
tools/testing/selftests/bpf/progs/verifier_nocsr.c +89 -0
index 84f76f850e9a..8b789f56c9e4 100644
@@ -462,6 +462,95 @@ __naked void bad_helper_write(void)
 	: __clobber_all);
 }
 
+SEC("raw_tp")
+__arch_x86_64
+__xlated("1: *(u64 *)(r10 -16) = r1")
+__xlated("3: r0 = &(void __percpu *)(r0)")
+__xlated("5: r1 = *(u64 *)(r10 -16)")
+__success
+__naked void bad_probe_read_kernel_fixed_off(void)
+{
+	asm volatile (
+	"r1 = 1;"
+	/* nocsr pattern with stack offset -24 */
+	"*(u64 *)(r10 - 16) = r1;"
+	"call %[bpf_get_smp_processor_id];"
+	"r1 = *(u64 *)(r10 - 16);"
+	"r1 = r10;"
+	"r1 += -8;"
+	"r2 = 1;"
+	"r3 = r10;"
+	"r3 += -16;"
+	/* read src is fp[-16], thus nocsr rewrite not applied */
+	"call %[bpf_probe_read_kernel];"
+	"exit;"
+	:
+	: __imm(bpf_get_smp_processor_id),
+	  __imm(bpf_probe_read_kernel)
+	: __clobber_all);
+}
+
+SEC("raw_tp")
+__arch_x86_64
+__xlated("2: r0 = &(void __percpu *)(r0)")
+__success
+__naked void good_probe_read_kernel_fixed_off(void)
+{
+	asm volatile (
+	"r1 = 1;"
+	/* nocsr pattern with stack offset -24 */
+	"*(u64 *)(r10 - 24) = r1;"
+	"call %[bpf_get_smp_processor_id];"
+	"r1 = *(u64 *)(r10 - 24);"
+	"r1 = r10;"
+	"r1 += -8;"
+	"r2 = 1;"
+	"r3 = r10;"
+	"r3 += -16;"
+	/* read src is fp[-16], nocsr rewrite should be ok */
+	"call %[bpf_probe_read_kernel];"
+	"exit;"
+	:
+	: __imm(bpf_get_smp_processor_id),
+	  __imm(bpf_probe_read_kernel)
+	: __clobber_all);
+}
+
+SEC("raw_tp")
+__arch_x86_64
+__xlated("6: *(u64 *)(r10 -16) = r1")
+__xlated("8: r0 = &(void __percpu *)(r0)")
+__xlated("10: r1 = *(u64 *)(r10 -16)")
+__success
+__naked void bad_probe_read_kernel_var_off(void)
+{
+	asm volatile (
+	"r6 = *(u64 *)(r1 + 0);" /* random scalar value */
+	"r6 &= 0x7;"		 /* r6 range [0..7] */
+	"r6 += 0x2;"		 /* r6 range [2..9] */
+	"r7 = 0;"
+	"r7 -= r6;"		 /* r7 range [-9..-2] */
+	"r1 = 1;"
+	/* nocsr pattern with stack offset -24 */
+	"*(u64 *)(r10 - 16) = r1;"
+	"call %[bpf_get_smp_processor_id];"
+	"r1 = *(u64 *)(r10 - 16);"
+	"r1 = r10;"
+	"r1 += -8;"
+	"r2 = 1;"
+	"r3 = r10;"
+	"r3 += r7;"
+	/* read src is fp[-9..-2],
+	 * which touches range [-16..-9] reserved for nocsr rewrite
+	 */
+	"call %[bpf_probe_read_kernel_str];"
+	"exit;"
+	:
+	: __imm(bpf_get_smp_processor_id),
+	  __imm(bpf_probe_read_kernel_str)
+	: __clobber_all);
+}
+
 SEC("raw_tp")
 __arch_x86_64
 /* main, not patched */
Signature · 1 line
2.45.2