On Thu, Jan 4, 2024 at 6:23 AM Leon Hwang <hffilwlqm@xxxxxxxxx> wrote:
>
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index fe30b9ebb8de4..67fa337fc2e0c 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -259,7 +259,7 @@ struct jit_context {
> /* Number of bytes emit_patch() needs to generate instructions */
> #define X86_PATCH_SIZE 5
> /* Number of bytes that will be skipped on tailcall */
> -#define X86_TAIL_CALL_OFFSET (11 + ENDBR_INSN_SIZE)
> +#define X86_TAIL_CALL_OFFSET (22 + ENDBR_INSN_SIZE)
>
> static void push_r12(u8 **pprog)
> {
> @@ -406,14 +406,21 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf,
> */
> emit_nops(&prog, X86_PATCH_SIZE);
> if (!ebpf_from_cbpf) {
> - if (tail_call_reachable && !is_subprog)
> + if (tail_call_reachable && !is_subprog) {
> /* When it's the entry of the whole tailcall context,
> * zeroing rax means initialising tail_call_cnt.
> */
> - EMIT2(0x31, 0xC0); /* xor eax, eax */
> - else
> - /* Keep the same instruction layout. */
> - EMIT2(0x66, 0x90); /* nop2 */
> + EMIT2(0x31, 0xC0); /* xor eax, eax */
> + EMIT1(0x50); /* push rax */
> + /* Make rax as ptr that points to tail_call_cnt. */
> + EMIT3(0x48, 0x89, 0xE0); /* mov rax, rsp */
> + EMIT1_off32(0xE8, 2); /* call main prog */
> + EMIT1(0x59); /* pop rcx, get rid of tail_call_cnt */
> + EMIT1(0xC3); /* ret */
> + } else {
> + /* Keep the same instruction size. */
> + emit_nops(&prog, 13);
> + }
I'm afraid the extra call breaks stack unwinding and many other things.
The proper frame needs to be setup (push rbp; etc)
and 'leave' + emit_return() is used.
Plain 'ret' is not ok.
x86_call_depth_emit_accounting() needs to be used too.
That will make X86_TAIL_CALL_OFFSET adjustment very complicated.
Also the fix doesn't address the stack size issue.
We shouldn't allow all the extra frames at run-time.
The tail_cnt_ptr approach is interesting but too heavy,
since arm64, s390 and other JITs would need to repeat it with equally
complicated calculations in TAIL_CALL_OFFSET.
The fix should really be thought through for all JITs. Not just x86.
I'm thinking whether we should do the following instead:
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 0bdbbbeab155..0b45571559be 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -910,7 +910,7 @@ static void *prog_fd_array_get_ptr(struct bpf_map *map,
if (IS_ERR(prog))
return prog;
- if (!bpf_prog_map_compatible(map, prog)) {
+ if (!bpf_prog_map_compatible(map, prog) || prog->aux->func_cnt) {
bpf_prog_put(prog);
return ERR_PTR(-EINVAL);
}
This will stop stack growth, but it will break a few existing tests.
I feel it's a price worth paying.
John, Daniel,
do you see anything breaking on cilium side if we disallow
progs with subprogs to be inserted in prog_array ?
Other alternatives?
