no_caller_saved_registers attribute for helper calls
Subsystems (4)
Eduard Zingerman
maintainerv4 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: exitHere 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:
- v3 -> v4:
- When nocsr spills/fills are removed in the subprogram, allow these spills/fills to reside in [-MAX_BPF_STACK-48..MAX_BPF_STACK) range (suggested by Alexei);
- Dropped patches with special handling for bpf_probe_read_kernel()
(requested by Alexei);
- Reset aux .nocsr_pattern and .nocsr_spills_num fields in check_nocsr_stack_contract() (requested by Andrii). Andrii, I have not added an additional flag to
struct bpf_subprog_info, it currently does not have holes and I really don't like adding a bool field there just as an alternative indicator that nocsr is disabled.
Indicator at the moment:
- nocsr_stack_off >= S16_MIN means that nocsr rewrite is enabled;
- nocsr_stack_off == S16_MIN means that nocsr rewrite is disabled.
- 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]/
- v3 https://lore.kernel.org/bpf/[email protected]/
Eduard Zingerman (10):
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
include/linux/bpf.h | 6 + include/linux/bpf_verifier.h | 14 + kernel/bpf/helpers.c | 1 + kernel/bpf/verifier.c | 351 +++++++- 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 | 796 ++++++++++++++++++ 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, 1432 insertions(+), 126 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
Eduard Zingerman
maintainerPATCH v4 1/10 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
@@ -10285,6 +10285,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) {
@@ -10301,18 +10314,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
Eduard Zingerman
maintainerPATCH v4 2/10 bpf: no_caller_saved_registers attribute for helper calls
In reply to Eduard ZingermanGCC 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 | 317 ++++++++++++++++++++++++++++++++++- 3 files changed, 334 insertions(+), 3 deletions(-)
include/linux/bpf.h +6 -0
@@ -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
@@ -576,6 +576,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 */
@@ -645,6 +653,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;
@@ -652,6 +664,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 +314 -3
387 lines not on this page · raw message
Signature · 1 line
2.45.2
Eduard Zingerman
maintainerPATCH v4 3/10 bpf, x86, riscv, arm: no_caller_saved_registers for bpf_get_smp_processor_id()
In reply to Eduard ZingermanThe 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
@@ -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
@@ -16043,7 +16043,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.
@@ -20747,7 +20754,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
Eduard Zingerman
maintainerPATCH v4 4/10 selftests/bpf: extract utility function for BPF disassembly
In reply to Eduard Zingermanstruct 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
@@ -657,6 +657,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
@@ -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
@@ -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
Eduard Zingerman
maintainerPATCH v4 5/10 selftests/bpf: print correct offset for pseudo calls in disasm_insn()
In reply to Eduard ZingermanAdjust 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
@@ -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
Eduard Zingerman
maintainerPATCH v4 6/10 selftests/bpf: no need to track next_match_pos in struct test_loader
In reply to Eduard ZingermanThe 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
@@ -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
@@ -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
Eduard Zingerman
maintainerPATCH v4 7/10 selftests/bpf: extract test_loader->expect_msgs as a data structure
In reply to Eduard ZingermanNon-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
@@ -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
Eduard Zingerman
maintainerPATCH v4 8/10 selftests/bpf: allow checking xlated programs in verifier_* tests
In reply to Eduard ZingermanAdd 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
@@ -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
Eduard Zingerman
maintainerPATCH v4 9/10 selftests/bpf: __arch_* macro to limit test cases to specific archs
In reply to Eduard ZingermanAdd 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
@@ -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
Eduard Zingerman
maintainerPATCH v4 10/10 selftests/bpf: test no_caller_saved_registers spill/fill removal
In reply to Eduard ZingermanTests 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;
- tests for nocsr stack spills below max stack depth.
--- .../selftests/bpf/prog_tests/verifier.c | 2 + .../selftests/bpf/progs/verifier_nocsr.c | 796 ++++++++++++++++++ 2 files changed, 798 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/verifier_nocsr.c
tools/testing/selftests/bpf/prog_tests/verifier.c +2 -0
@@ -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 +796 -0
796 lines not on this page · raw message
Signature · 1 line
2.45.2
CI has tested the following submission:
Dear patch submitter,
CI has tested the following submission:
Status: SUCCESS Name: [bpf-next,v4,00/10] no_caller_saved_registers attribute for helper calls Patchwork: https://patchwork.kernel.org/project/netdevbpf/list/?series=873083&state=* Matrix: https://github.com/kernel-patches/bpf/actions/runs/10050074658
No further action is necessary on your part.
Please note: this email is coming from an unmonitored mailbox. If you have questions or feedback, please reach out to the Meta Kernel CI team at [email protected].
CI has tested the following submission:
Dear patch submitter,
CI has tested the following submission:
Status: FAILURE Name: [bpf-next,v4,00/10] no_caller_saved_registers attribute for helper calls Patchwork: https://patchwork.kernel.org/project/netdevbpf/list/?series=873083&state=* Matrix: https://github.com/kernel-patches/bpf/actions/runs/10050709206
Failed jobs:
test_progs_no_alu32-aarch64-gcc: https://github.com/kernel-patches/bpf/actions/runs/10050709206/job/27779271465
test_progs_no_alu32-s390x-gcc: https://github.com/kernel-patches/bpf/actions/runs/10050709206/job/27779454667
test_progs_no_alu32-x86_64-gcc: https://github.com/kernel-patches/bpf/actions/runs/10050709206/job/27779437404
test_progs_no_alu32-x86_64-llvm-17: https://github.com/kernel-patches/bpf/actions/runs/10050709206/job/27779452223
test_progs_no_alu32-x86_64-llvm-18: https://github.com/kernel-patches/bpf/actions/runs/10050709206/job/27779453627
First test_progs failure (test_progs_no_alu32-aarch64-gcc):
#134 libbpf_get_fd_by_id_opts
libbpf: prog 'check_access': BPF program load failed: Invalid argument
libbpf: prog 'check_access': -- BEGIN PROG LOAD LOG --
0: R1=ctx() R10=fp0
; int BPF_PROG(check_access, struct bpf_map *map, fmode_t fmode) @ test_libbpf_get_fd_by_id_opts.c:27
0: (b7) r0 = 0 ; R0_w=0
1: (79) r2 = *(u64 *)(r1 +0)
func 'bpf_lsm_bpf_map' arg0 has btf_id 2072 type STRUCT 'bpf_map'
2: R1=ctx() R2_w=trusted_ptr_bpf_map()
; if (map != (struct bpf_map *)&data_input) @ test_libbpf_get_fd_by_id_opts.c:29
2: (18) r3 = 0xffff0000c0273a00 ; R3_w=map_ptr(map=data_input,ks=4,vs=4)
4: (5d) if r2 != r3 goto pc+4 ; R2_w=trusted_ptr_bpf_map() R3_w=map_ptr(map=data_input,ks=4,vs=4)
; int BPF_PROG(check_access, struct bpf_map *map, fmode_t fmode) @ test_libbpf_get_fd_by_id_opts.c:27
5: (79) r0 = *(u64 *)(r1 +8) ; R0_w=scalar() R1=ctx()
; if (fmode & FMODE_WRITE) @ test_libbpf_get_fd_by_id_opts.c:32
6: (67) r0 <<= 62 ; R0_w=scalar(smax=0x4000000000000000,umax=0xc000000000000000,smin32=0,smax32=umax32=0,var_off=(0x0; 0xc000000000000000))
7: (c7) r0 s>>= 63 ; R0_w=scalar(smin=smin32=-1,smax=smax32=0)
; @ test_libbpf_get_fd_by_id_opts.c:0
8: (57) r0 &= -13 ; R0_w=scalar(smax=0x7ffffffffffffff3,umax=0xfffffffffffffff3,smax32=0x7ffffff3,umax32=0xfffffff3,var_off=(0x0; 0xfffffffffffffff3))
; int BPF_PROG(check_access, struct bpf_map *map, fmode_t fmode) @ test_libbpf_get_fd_by_id_opts.c:27 9: (95) exit
At program exit the register R0 has smax=9223372036854775795 should have been in [-4095, 0] processed 9 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0 -- END PROG LOAD LOG --
libbpf: prog 'check_access': failed to load: -22
libbpf: failed to load object 'test_libbpf_get_fd_by_id_opts'
libbpf: failed to load BPF skeleton 'test_libbpf_get_fd_by_id_opts': -22 test_libbpf_get_fd_by_id_opts:FAIL:test_libbpf_get_fd_by_id_opts__open_and_load unexpected error: -22
Please note: this email is coming from an unmonitored mailbox. If you have questions or feedback, please reach out to the Meta Kernel CI team at [email protected].
CI has tested the following submission:
Dear patch submitter,
CI has tested the following submission:
Status: SUCCESS Name: [bpf-next,v4,00/10] no_caller_saved_registers attribute for helper calls Patchwork: https://patchwork.kernel.org/project/netdevbpf/list/?series=873083&state=* Matrix: https://github.com/kernel-patches/bpf/actions/runs/10051541692
No further action is necessary on your part.
Please note: this email is coming from an unmonitored mailbox. If you have questions or feedback, please reach out to the Meta Kernel CI team at [email protected].
Quoted 2 lines · Eduard Zingerman
Corresponding clang/llvm changes are available in [2].
..
Applied kernel bits. Thanks!
Pls cleanup llvm diff and prepare it for landing.
Though we simplified kernel support quite a bit I feel we may hit unexpected corner cases once llvm part lands. So the sooner the better.
This series was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <[email protected]>:
Hello:
This series was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <[email protected]>:
Quoted 11 lines · On Mon, 22 Jul 2024 16:38:34 -0700 you wrote:
This patch-set seeks to allow using
no_caller_saved_registersgcc/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.[...]
Here is the summary with links:
- [bpf-next,v4,01/10] bpf: add a get_helper_proto() utility function
https://git.kernel.org/bpf/bpf-next/c/19b0934f0b13 - [bpf-next,v4,02/10] bpf: no_caller_saved_registers attribute for helper calls https://git.kernel.org/bpf/bpf-next/c/c473f709550f - [bpf-next,v4,03/10] bpf, x86, riscv, arm: no_caller_saved_registers for bpf_get_smp_processor_id() https://git.kernel.org/bpf/bpf-next/c/a5a0f95ba4e9 - [bpf-next,v4,04/10] selftests/bpf: extract utility function for BPF disassembly https://git.kernel.org/bpf/bpf-next/c/67b1c158c54e - [bpf-next,v4,05/10] selftests/bpf: print correct offset for pseudo calls in disasm_insn() https://git.kernel.org/bpf/bpf-next/c/9f3e5a694b03 - [bpf-next,v4,06/10] selftests/bpf: no need to track next_match_pos in struct test_loader https://git.kernel.org/bpf/bpf-next/c/0bbaa40df698 - [bpf-next,v4,07/10] selftests/bpf: extract test_loader->expect_msgs as a data structure https://git.kernel.org/bpf/bpf-next/c/a70c32d65ec7 - [bpf-next,v4,08/10] selftests/bpf: allow checking xlated programs in verifier_* tests https://git.kernel.org/bpf/bpf-next/c/69c992268830 - [bpf-next,v4,09/10] selftests/bpf: __arch_* macro to limit test cases to specific archs https://git.kernel.org/bpf/bpf-next/c/c64d2f72bf2e - [bpf-next,v4,10/10] selftests/bpf: test no_caller_saved_registers spill/fill removal https://git.kernel.org/bpf/bpf-next/c/2daa48f6e400
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html