Re: [PATCH bpf-next v1 2/2] selftests/bpf: Add compiler-layout terminal gotox test

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 26/08/30 07:31AM, Siddharth Chintamaneni wrote:
> Add coverage for the instruction layout emitted by LLVM for a computed
> goto. The jump-table targets precede the dispatch block and gotox is the
> final instruction in the subprogram.

I would shorten this, and remove mention of compiler altogether (this
is not a compiler bug after all). Just explain that we add a test
where gotox is the terminal instruction of a program.

> An equivalent unoptimized C source is:
> 
>     int bpf_prog_trigger_syscall_prog(void *ctx)
>     {
>             __label__ l1, l2;
>             void *tgt;
>             int ret = 0;
> 
>             if (ctx)
>                     tgt = &&l1;
>             else
>                     tgt = &&l2;
>             goto *tgt;
>     l1:
>             ret += 1;
>     l2:
>             ret += 2;
>             return 0;
>     }

This is absolutely not obvious why this must necessarily 
translate to the test you're added. Also, again, this tests
the verification, not code generation. Just omit this C chunk.

> 
> Tested:
> 
>     #646/5 verifier_gotox/jump_table_compiler_layout:OK
>     #646 verifier_gotox:OK
>     Summary: 1/28 PASSED, 0 SKIPPED, 0/0 FAILED

This output is not adding any information, please omit it.

> 
> Signed-off-by: Siddharth Chintamaneni <sidchintamaneni@gmail.com>
> ---
>  .../selftests/bpf/progs/verifier_gotox.c      | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/progs/verifier_gotox.c b/tools/testing/selftests/bpf/progs/verifier_gotox.c
> index 5b18c9a27717..0ea445287175 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_gotox.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_gotox.c
> @@ -47,6 +47,61 @@ DEFINE_SIMPLE_JUMP_TABLE_PROG(reserved_field_src_reg,      BPF_REG_1, 0, 0, __fa
>  DEFINE_SIMPLE_JUMP_TABLE_PROG(reserved_field_non_zero_off, BPF_REG_0, 1, 0, __failure __msg("BPF_JA|BPF_X uses reserved fields"))
>  DEFINE_SIMPLE_JUMP_TABLE_PROG(reserved_field_non_zero_imm, BPF_REG_0, 0, 1, __failure __msg("BPF_JA|BPF_X uses reserved fields"))
>  
> +SEC("socket")
> +__success __retval(0)
> +__naked void jump_table_compiler_layout(void)
> +{
> +	asm volatile ("						        \
> +	.pushsection .jumptables,\"\",@progbits;			\
> +jt_l1_%=:								\
> +	.quad l1_%= - socket;						\
> +	.size jt_l1_%=, 8;						\
> +	.global jt_l1_%=;						\
> +jt_l2_%=:								\
> +	.quad l2_%= - socket;						\
> +	.size jt_l2_%=, 8;						\
> +	.global jt_l2_%=;						\
> +	.popsection;							\
> +									\
> +	*(u64 *)(r10 - 8) = r1;						\
> +	*(u32 *)(r10 - 20) = 0;						\
> +	r1 = *(u64 *)(r10 - 8);						\
> +	if r1 == 0 goto select_l2_%=;					\
> +	goto select_l1_%=;						\
> +select_l1_%=:								\
> +	r1 = jt_l1_%= ll;						\
> +	r1 = *(u64 *)(r1 + 0);						\
> +	*(u64 *)(r10 - 16) = r1;					\
> +	goto selected_%=;						\
> +select_l2_%=:								\
> +	r1 = jt_l2_%= ll;						\
> +	r1 = *(u64 *)(r1 + 0);						\
> +	*(u64 *)(r10 - 16) = r1;					\
> +	goto selected_%=;						\
> +selected_%=:								\
> +	r1 = *(u64 *)(r10 - 16);					\
> +	*(u64 *)(r10 - 32) = r1;					\
> +	goto dispatch_%=;						\
> +l1_%=:									\
> +	w1 = *(u32 *)(r10 - 20);					\
> +	w1 += 1;							\
> +	*(u32 *)(r10 - 20) = w1;					\
> +	goto l2_%=;							\
> +l2_%=:									\
> +	w1 = *(u32 *)(r10 - 20);					\
> +	w1 += 2;							\
> +	*(u32 *)(r10 - 20) = w1;					\
> +	w0 = 0;								\
> +	exit;								\
> +dispatch_%=:								\
> +	r1 = *(u64 *)(r10 - 32);					\
> +	.8byte %[gotox_r1];						\
> +"	:
> +	: __imm_insn(gotox_r1, BPF_RAW_INSN(BPF_JMP | BPF_JA | BPF_X,
> +					    BPF_REG_1, 0, 0, 0))
> +	: __clobber_all);
> +}

This assembly can be simplified, no reason to pass something
generated from C. Just create a JT with one target, and jump
to it from the end. Something around

      0: rX = &jump_table  /* jump_table[1] = { &&ret, } */
      1: rX = *rX
      2: goto end
    ret:
      3: return 0
    end:
      4: gotox *rX

Also, if you want a full selftest coverage, another selftest should
check the same thing for a subprogram, not the main prog.

> +
>  /*
>   * Gotox is forbidden when there is no jump table loaded
>   * which points to the sub-function where the gotox is used
> -- 
> 2.43.0
>