Re: [PATCH bpf-next v2 1/2] bpf: Support private stack for bpf progs

[Eduard Zingerman <eddyz87@gmail.com>]

Hi Yonghong,

In general I think that changes in this patch are logical and make sense.
I have a suggestion regarding testing JIT related changes.

We currently lack a convenient way to verify jit behaviour modulo
runtime tests. I think we should have a capability to write tests like below:

    SEC("tp")
    __jited_x86("f:	endbr64")
    __jited_x86("13:	movabs $0x.*,%r9")
    __jited_x86("1d:	add    %gs:0x.*,%r9")
    __jited_x86("26:	mov    $0x1,%edi")
    __jited_x86("2b:	mov    %rdi,-0x8(%r9)")
    __jited_x86("2f:	mov    -0x8(%r9),%rdi")
    __jited_x86("33:	xor    %eax,%eax")
    __jited_x86("35:	lock xchg %rax,-0x8(%r9)")
    __jited_x86("3a:	lock xadd %rax,-0x8(%r9)")
    __naked void stack_access_insns(void)
    {
    	asm volatile (
    	"r1 = 1;"
    	"*(u64 *)(r10 - 8) = r1;"
    	"r1 = *(u64 *)(r10 - 8);"
    	"r0 = 0;"
    	"r0 = xchg_64(r10 - 8, r0);"
    	"r0 = atomic_fetch_add((u64 *)(r10 - 8), r0);"
    	"exit;"
    	::: __clobber_all);
    }

In the following branch I explored a way to add such capability:
https://github.com/eddyz87/bpf/tree/yhs-private-stack-plus-jit-testing

Beside testing exact translation, such tests also provide good
starting point for people trying to figure out how some jit features work.

The below two commits are the gist of the feature:
8f9361be2fb3 ("selftests/bpf: __jited_x86 test tag to check x86 assembly after jit")
0156b148b5b4 ("selftests/bpf: utility function to get program disassembly after jit")

For "0156b148b5b4" I opted to do a popen() call and execute bpftool process,
an alternative would be to:
a. either link tools/bpf/bpftool/jit_disasm.c as a part of the
   test_progs executable;
b. call libbfd (binutils dis-assembler) directly from the bpftool.

Currently bpftool can use two dis-assemblers: libbfd and llvm library,
depending on the build environment. For CI builds libbfd is used.
I don't know if llvm and libbfd always produce same output for
identical binary code. Imo, if people are Ok with adding libbfd
dependency to test_progs, option (b) is the best. If folks on the
mailing list agree with this, I can work on updating the patches.

-------------

Aside from testing I agree with Andrii regarding rbp usage,
it seems like it should be possible to do the following in prologue:

    movabs $0x...,%rsp
    add %gs:0x...,%rsp
    push %rbp

and there would be no need to modify translation for instructions
accessing r10, plus debugger stack unrolling logic should still work?.
Or am I mistaken?

Thanks,
Eduard