Re: [RFC PATCH bpf-next 3/6] selftests/test: test gen_prologue and gen_epilogue

[Martin KaFai Lau <martin.lau@linux.dev>]

On 8/15/24 6:50 PM, Eduard Zingerman wrote:
> On Thu, 2024-08-15 at 17:23 -0700, Eduard Zingerman wrote:
> 
> [...]
> 
>>> Re: __retval(), the struct_ops progs is triggered by a SEC("syscall") prog.
>>> Before calling this syscall prog, the st_ops map needs to be attached first. I
>>> think the attach part is missing also? or there is a way?
>>
>> I think libbpf handles the attachment automatically, I'll double check and reply.
>>
> 
> In theory, the following addition to the example I've sent already should work:
> 
>      struct st_ops_args;
>      int bpf_kfunc_st_ops_test_prologue(struct st_ops_args *args) __ksym;
>   
>      SEC("syscall")
>      __retval(0)
>      int syscall_prologue(void *ctx)
>      {
>      	struct st_ops_args args = { -42 };
>      	bpf_kfunc_st_ops_test_prologue(&args);
>      	return args.a;
>      }
> 
> However, the initial value of -42 is not changed, e.g. here is the log:
> 
>      $ ./test_progs -vvv -t struct_ops_epilogue/syscall_prologue
>      ...
>      libbpf: loaded kernel BTF from '/sys/kernel/btf/vmlinux'
>      libbpf: extern (func ksym) 'bpf_kfunc_st_ops_test_prologue': resolved to bpf_testmod [104486]
>      libbpf: struct_ops init_kern st_ops: type_id:44 kern_type_id:104321 kern_vtype_id:104378
>      libbpf: struct_ops init_kern st_ops: func ptr test_prologue is set to prog test_prologue from data(+0) to kern_data(+0)
>      libbpf: struct_ops init_kern st_ops: func ptr test_epilogue is set to prog test_epilogue from data(+8) to kern_data(+8)
>      libbpf: map 'st_ops': created successfully, fd=5
>      run_subtest:PASS:unexpected_load_failure 0 nsec
>      VERIFIER LOG:
>      =============
>      ...
>      =============
>      do_prog_test_run:PASS:bpf_prog_test_run 0 nsec
>      run_subtest:FAIL:837 Unexpected retval: -42 != 0
>      #321/3   struct_ops_epilogue/syscall_prologue:FAIL
>      #321     struct_ops_epilogue:FAIL
> 
> So, something goes awry in bpf_kfunc_st_ops_test_prologue():
> 
>      __bpf_kfunc int bpf_kfunc_st_ops_test_prologue(struct st_ops_args *args)
>      {
>      	int ret = -1;
>      
>      	mutex_lock(&st_ops_mutex);
>      	if (st_ops && st_ops->test_prologue)

Thanks for checking!

I think the bpf_map__attach_struct_ops() is not done such that st_ops is NULL.

It probably needs another tag in the SEC("syscall") program to tell which st_ops 
map should be attached first before executing the "syscall" program.

I like the idea of using the __xlated macro to check the patched prologue, ctx 
pointer saving, and epilogue. I will add this test in the respin. I will keep 
the current way in this patch to exercise syscall and the ops/func in st_ops for 
now. We can iterate on it later and use it as an example on what supports are 
needed on the test_loader side for st_ops map testing. On the repetitive-enough 
to worth test_loader refactoring side, I suspect some of the existing st_ops 
load-success/load-failure tests may be worth to look at also. Thoughts?

>      		ret = st_ops->test_prologue(args);
>      	mutex_unlock(&st_ops_mutex);
>      
>      	return ret;
>      }
> 
> Either st_ops is null or st_ops->test_prologue is null.
> However, the log above shows:
> 
>      libbpf: struct_ops init_kern st_ops: type_id:44 kern_type_id:104321 kern_vtype_id:104378
>      libbpf: struct_ops init_kern st_ops: func ptr test_prologue is set to prog test_prologue from data(+0) to kern_data(+0)
>      libbpf: struct_ops init_kern st_ops: func ptr test_epilogue is set to prog test_epilogue from data(+8) to kern_data(+8)
> 
> Here libbpf does autoload for st_ops map and populates it, so st_ops->test_prologue should not be null.
> Will have some time tomorrow to debug this (or you can give it a shot if you'd like).
>