On Mon, Jul 17, 2023 at 9:36 AM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote: > > On Sat, 15 Jul 2023 at 04:09, Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > On Thu, Jul 13, 2023 at 08:02:29AM +0530, Kumar Kartikeya Dwivedi wrote: > > > Now that we allow exception throwing using bpf_throw kfunc, it can > > > appear as the final instruction in a prog. When this happens, and we > > > begin to unwind the stack using arch_bpf_stack_walk, the instruction > > > pointer (IP) may appear to lie outside the JITed instructions. This > > > happens because the return address is the instruction following the > > > call, but the bpf_throw never returns to the program, so the JIT > > > considers instruction ending at the bpf_throw call as the final JITed > > > instruction and end of the jited_length for the program. > > > > > > This becomes a problem when we search the IP using is_bpf_text_address > > > and bpf_prog_ksym_find, both of which use bpf_ksym_find under the hood, > > > and it rightfully considers addr == ksym.end to be outside the program's > > > boundaries. > > > > > > Insert a dummy 'int3' instruction which will never be hit to bump the > > > jited_length and allow us to handle programs with their final > > > isntruction being a call to bpf_throw. > > > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> > > > --- > > > arch/x86/net/bpf_jit_comp.c | 11 +++++++++++ > > > include/linux/bpf.h | 2 ++ > > > 2 files changed, 13 insertions(+) > > > > > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > > > index 8d97c6a60f9a..052230cc7f50 100644 > > > --- a/arch/x86/net/bpf_jit_comp.c > > > +++ b/arch/x86/net/bpf_jit_comp.c > > > @@ -1579,6 +1579,17 @@ st: if (is_imm8(insn->off)) > > > } > > > if (emit_call(&prog, func, image + addrs[i - 1] + offs)) > > > return -EINVAL; > > > + /* Similar to BPF_EXIT_INSN, call for bpf_throw may be > > > + * the final instruction in the program. Insert an int3 > > > + * following the call instruction so that we can still > > > + * detect pc to be part of the bpf_prog in > > > + * bpf_ksym_find, otherwise when this is the last > > > + * instruction (as allowed by verifier, similar to exit > > > + * and jump instructions), pc will be == ksym.end, > > > + * leading to bpf_throw failing to unwind the stack. > > > + */ > > > + if (func == (u8 *)&bpf_throw) > > > + EMIT1(0xCC); /* int3 */ > > > > Probably worth explaining that this happens because bpf_throw is marked > > __attribute__((noreturn)) and compiler can emit it last without BPF_EXIT insn. > > Meaing the program might not have BPF_EXIT at all. > > Yes, sorry about omitting that. I will add it to the commit message in v2. > > > > > I wonder though whether this self-inflicted pain is worth it. > > May be it shouldn't be marked as noreturn. > > What do we gain by marking? > > It felt like the obvious thing to do to me. The cost on the kernel > side is negligible (atleast in my opinion), we just have to allow it > as final instruction in the program. If it's heavily used it allows > the compiler to better optimize the code (marking anything after it > unreachable, no need to save registers etc., although this may not be > a persuasive point for you). "no need to save registers"... "optimize"... that's the thing that worries me. I think it's better to drop noreturn attribute. bpf has implicit prolog/epilogue that only apply to bpf_exit insn. bpf_call insn that doesn't return is exploiting undefined logic in the compiler, since we never fully clarified our hidden prologue/epilogue rules. Saying it differently, bpf_tail_call is also noreturn, but if we mark it as such all kinds of things will break. We still need to add alloca(). It doesn't play well with the current BPF ISA. I think it's better to treat 'noreturn' as broken in the compiler, since its behavior may change. > Regardless of this noreturn attribute, I was thinking whether we > should always emit an extra instruction so that any IP (say one past > last instruction) we get for a BPF prog can always be seen as > belonging to it. It probably is only a problem surfaced by this > bpf_throw call at the end, but I was wondering whether doing it > unconditionally makes sense. I think it's a corner case of this 'noreturn' from bpf_call logic. The bpf prog is padded with 0xcc before and after already. What you're suggesting is to add one of 0xcc to the body of the prog. That doesn't sound right.
