On Mon, Oct 30, 2023 at 10:17:10AM -0700, Alexei Starovoitov wrote:
> 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.
Got it, will add tests for negation and bswap with inline assembly.
Thanks you both for feedbacks and suggestions!
