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

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

[Yonghong Song]

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>
---
 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;
 }
 
-- 
2.34.1

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

[Eduard Zingerman]

On Sat, 2023-08-26 at 13:08 -0700, 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.

Can confirm, this patch fixes listed flaky tests for me.
(when using LLVM main 651e644595b7 from 6 days ago).

Tested-by: Eduard Zingerman <eddyz87@gmail.com>

> 
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  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;
>  }
>  

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

[Alan Maguire]

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!

> ---
>  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?

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

[Yonghong Song]

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.

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Sat, 26 Aug 2023 13:08:43 -0700 you 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
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf: Prevent inlining of bpf_fentry_test7()
    https://git.kernel.org/bpf/bpf/c/32337c0a2824

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html