On Tue, Mar 15, 2022 at 02:30:43PM IST, Peter Zijlstra wrote:
> On Tue, Mar 15, 2022 at 02:14:02AM +0530, Kumar Kartikeya Dwivedi wrote:
>
> > [ Note: I have no experience with trampoline code or IBT so what follows might
> > be incorrect. ]
> >
> > In case of fexit and fmod_ret, we call original function (but skip
> > X86_PATCH_SIZE bytes), with ENDBR we must also skip those 4 bytes, but in some
> > cases like bpf_fentry_test1, for which this test has fmod_ret prog, compiler
> > (gcc 11) emits endbr64, but not for do_init_module, for which we do fexit.
> >
> > This means for do_init_module module, orig_call += X86_PATCH_SIZE +
> > ENDBR_INSN_SIZE would skip more bytes than needed to emit call to original
> > function, which explains why I was seeing crash in the middle of
> > 'mov edx, 0x10' instruction.
> >
> > The diff below fixes the problem for me, and allows the test to pass.
> >
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > index b98e1c95bcc4..760c9a3c075f 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -2031,11 +2031,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> >
> > ip_off = stack_size;
> >
> > - if (flags & BPF_TRAMP_F_SKIP_FRAME)
> > + if (flags & BPF_TRAMP_F_SKIP_FRAME) {
> > /* skip patched call instruction and point orig_call to actual
> > * body of the kernel function.
> > */
> > - orig_call += X86_PATCH_SIZE + ENDBR_INSN_SIZE;
> > + if (is_endbr(*(u32 *)orig_call))
> > + orig_call += ENDBR_INSN_SIZE;
> > + orig_call += X86_PATCH_SIZE;
> > + }
> >
> > prog = image;
>
> Hmm, so I was under the impression that this was targeting the NOP from
> emit_prologue(), and that has an unconditional ENDBR. If this is instead
> targeting the 'start of random kernel function' then yes, what you
> propose will work.
>
> (obviously, once we go do more complicated CFI schemes, all this needs
> revisiting yet again).
>
> I don't seem able to run this mod_race test, it keeps saying:
>
> tgl-build# ./test_progs -v -t mod_race
> bpf_testmod.ko is already unloaded.
> Loading bpf_testmod.ko...
> Successfully loaded bpf_testmod.ko.
> Summary: 0/0 PASSED, 0 SKIPPED, 0 FAILED
> Successfully unloaded bpf_testmod.ko.
>
`./test_progs -v -t bpf_mod_race` should work.
> Which I'm taking to mean I'm doing it wrong... so I can't immediately
> verify, but your proposal looks sane so I'll fold it in.
>
> Thanks!
--
Kartikeya
