On Mon, Oct 30, 2023 at 7:36 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Mon, 2023-10-30 at 21:21 +0800, Shung-Hsi Yu wrote:
> > Add a test written with inline assembly to check that the verifier does
> > not incorrecly use the src_reg field of a BPF_ALU | BPF_TO_BE | BPF_END
> > instruction.
> >
> > Signed-off-by: Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx>
> > ---
> >
> > This is the first time I'm writing a selftest so there's a lot of
> > question I can't answer myself. Looking for suggestions regarding:
> >
> > 1. Whether BPF_NEG and other BPF_END cases should be tested as well
>
> It is probably good to test BPF_NEG, unfortunately verifier does not
> track range information for BPF_NEG, so I ended up with the following
> contraption:
Makes sense to me.
> SEC("?raw_tp")
> __success __log_level(2)
> __msg("mark_precise: frame0: regs=r2 stack= before 3: (bf) r1 = r10")
> __msg("mark_precise: frame0: regs=r2 stack= before 2: (55) if r2 != 0xfffffff8 goto pc+2")
> __msg("mark_precise: frame0: regs=r2 stack= before 1: (87) r2 = -r2")
> __msg("mark_precise: frame0: regs=r2 stack= before 0: (b7) r2 = 8")
> __naked int bpf_neg(void)
> {
> asm volatile (
> "r2 = 8;"
> "r2 = -r2;"
> "if r2 != -8 goto 1f;"
> "r1 = r10;"
> "r1 += r2;"
> "1:"
> "r0 = 0;"
> "exit;"
> ::: __clobber_all);
> }
>
> Also, maybe it's good to test bswap version of BPF_END (CPU v4
> instruction) for completeness, e.g. as follows:
>
> #if (defined(__TARGET_ARCH_arm64) || defined(__TARGET_ARCH_x86) || \
> (defined(__TARGET_ARCH_riscv) && __riscv_xlen == 64) || \
> defined(__TARGET_ARCH_arm) || defined(__TARGET_ARCH_s390)) && \
> __clang_major__ >= 18
>
> ...
> "r2 = bswap16 r2;"
+1. Let's have a test for this one as well.
> ...
>
> #endif
>
>
> > 2. While the suggested way of writing BPF assembly is with inline
> > assembly[0], as done here, maybe it is better to have this test case
> > added in verifier/precise.c and written using macro instead?
> > The rational is that ideally we want the selftest to be backport to
> > the v5.3+ stable kernels alongside the fix, but __msg macro used here
> > is only available since v6.2.
>
> As far as I understand we want to have new tests written in assembly,
> but let's wait for Alexei or Andrii to comment.
Backports is not a reason to use macros.
Please continue with inline asm.
