Re: [PATCH bpf-next] bpf: Prevent inlining of bpf_fentry_test7()

[Yonghong Song <yonghong.song@linux.dev>]

On 8/28/23 12:26 AM, Alan Maguire wrote:
> On 26/08/2023 21:08, Yonghong Song wrote:
>> With latest clang18, I hit test_progs failures for the following test:
>>    #13/2    bpf_cookie/multi_kprobe_link_api:FAIL
>>    #13/3    bpf_cookie/multi_kprobe_attach_api:FAIL
>>    #13      bpf_cookie:FAIL
>>    #75      fentry_fexit:FAIL
>>    #76/1    fentry_test/fentry:FAIL
>>    #76      fentry_test:FAIL
>>    #80/1    fexit_test/fexit:FAIL
>>    #80      fexit_test:FAIL
>>    #110/1   kprobe_multi_test/skel_api:FAIL
>>    #110/2   kprobe_multi_test/link_api_addrs:FAIL
>>    #110/3   kprobe_multi_test/link_api_syms:FAIL
>>    #110/4   kprobe_multi_test/attach_api_pattern:FAIL
>>    #110/5   kprobe_multi_test/attach_api_addrs:FAIL
>>    #110/6   kprobe_multi_test/attach_api_syms:FAIL
>>    #110     kprobe_multi_test:FAIL
>>
>> For example, for #13/2, the error messages are
>>    ...
>>    kprobe_multi_test_run:FAIL:kprobe_test7_result unexpected kprobe_test7_result: actual 0 != expected 1
>>    ...
>>    kprobe_multi_test_run:FAIL:kretprobe_test7_result unexpected kretprobe_test7_result: actual 0 != expected 1
>>
>> clang17 does not have this issue.
>>
>> Further investigation shows that kernel func bpf_fentry_test7(), used
>> in the above tests, is inlined by the compiler although it is
>> marked as noinline.
>>
>>    int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg)
>>    {
>>          return (long)arg;
>>    }
>>
>> It is known that for simple functions like the above (e.g. just returning
>> a constant or an input argument), the clang compiler may still do inlining
>> for a noinline function. Adding 'asm volatile ("")' in the beginning of the
>> bpf_fentry_test7() can prevent inlining.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> 
> Fixes: d923021c2ce12 ("bpf: Add tests for PTR_TO_BTF_ID vs. null
> comparison")
> 
> ...might help this land in stable trees too. Thanks!

I didn't put a Fixes tag since the issue is caused by upgrading
clang compiler. The commit d923021c2ce12 does not have any issues
at its patch-applying time.

Since this is not a core kernel functionality bug and it is due
to upgrading clang compiler, I think backport is optional. But
if maintainers think I should add Fixes tag, I can do it
in the next revision.

> 
>> ---
>>   net/bpf/test_run.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
>> index 57a7a64b84ed..0841f8d82419 100644
>> --- a/net/bpf/test_run.c
>> +++ b/net/bpf/test_run.c
>> @@ -543,6 +543,7 @@ struct bpf_fentry_test_t {
>>   
>>   int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg)
>>   {
>> +	asm volatile ("");
>>   	return (long)arg;
>>   }
>>   
> Is there a risk bpf_fentry_test8/9 might get inlined too?

So far, not. I cannot predict in the future. But if this
happens with more complicated expressions got inlined for
a non-inline function, I can discuss with upstream.