netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf v2] selftests/bpf: Fix "missing ENDBR" BUG for destructor kfunc
@ 2022-11-22  7:32 Chen Hu
  2022-11-22 13:48 ` Jiri Olsa
  0 siblings, 1 reply; 8+ messages in thread
From: Chen Hu @ 2022-11-22  7:32 UTC (permalink / raw)
  Cc: hu1.chen, jpoimboe, memxor, bpf, Pengfei Xu, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo, Jiri Olsa, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, linux-kernel, netdev

With CONFIG_X86_KERNEL_IBT enabled, the test_verifier triggers the
following BUG:

  traps: Missing ENDBR: bpf_kfunc_call_test_release+0x0/0x30
  ------------[ cut here ]------------
  kernel BUG at arch/x86/kernel/traps.c:254!
  invalid opcode: 0000 [#1] PREEMPT SMP
  <TASK>
   asm_exc_control_protection+0x26/0x50
  RIP: 0010:bpf_kfunc_call_test_release+0x0/0x30
  Code: 00 48 c7 c7 18 f2 e1 b4 e8 0d ca 8c ff 48 c7 c0 00 f2 e1 b4 c3
	0f 1f 44 00 00 66 0f 1f 00 0f 1f 44 00 00 0f 0b 31 c0 c3 66 90
       <66> 0f 1f 00 0f 1f 44 00 00 48 85 ff 74 13 4c 8d 47 18 b8 ff ff ff
   bpf_map_free_kptrs+0x2e/0x70
   array_map_free+0x57/0x140
   process_one_work+0x194/0x3a0
   worker_thread+0x54/0x3a0
   ? rescuer_thread+0x390/0x390
   kthread+0xe9/0x110
   ? kthread_complete_and_exit+0x20/0x20

This is because there are no compile-time references to the destructor
kfuncs, bpf_kfunc_call_test_release() for example. So objtool marked
them sealable and ENDBR in the functions were sealed (converted to NOP)
by apply_ibt_endbr().

This fix creates dummy compile-time references to destructor kfuncs so
ENDBR stay there.

Fixes: 05a945deefaa ("selftests/bpf: Add verifier tests for kptr")
Signed-off-by: Chen Hu <hu1.chen@intel.com>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
---
v2:
- Use generic macro name and place the macro after function body as
- suggested by Jiri Olsa

v1: https://lore.kernel.org/all/20221121085113.611504-1-hu1.chen@intel.com/

 include/linux/btf_ids.h | 7 +++++++
 net/bpf/test_run.c      | 4 ++++
 2 files changed, 11 insertions(+)

diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
index 2aea877d644f..db02691b506d 100644
--- a/include/linux/btf_ids.h
+++ b/include/linux/btf_ids.h
@@ -266,4 +266,11 @@ MAX_BTF_TRACING_TYPE,
 
 extern u32 btf_tracing_ids[];
 
+#if defined(CONFIG_X86_KERNEL_IBT) && !defined(__DISABLE_EXPORTS)
+#define FUNC_IBT_NOSEAL(name)					\
+	asm(IBT_NOSEAL(#name));
+#else
+#define FUNC_IBT_NOSEAL(name)
+#endif /* CONFIG_X86_KERNEL_IBT */
+
 #endif
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 13d578ce2a09..07263b7cc12d 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -597,10 +597,14 @@ noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
 	refcount_dec(&p->cnt);
 }
 
+FUNC_IBT_NOSEAL(bpf_kfunc_call_test_release)
+
 noinline void bpf_kfunc_call_memb_release(struct prog_test_member *p)
 {
 }
 
+FUNC_IBT_NOSEAL(bpf_kfunc_call_memb_release)
+
 noinline void bpf_kfunc_call_memb1_release(struct prog_test_member1 *p)
 {
 	WARN_ON_ONCE(1);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf v2] selftests/bpf: Fix "missing ENDBR" BUG for destructor kfunc
  2022-11-22  7:32 [PATCH bpf v2] selftests/bpf: Fix "missing ENDBR" BUG for destructor kfunc Chen Hu
@ 2022-11-22 13:48 ` Jiri Olsa
  2022-11-22 14:14   ` Peter Zijlstra
  2022-11-25 13:28   ` Chen, Hu1
  0 siblings, 2 replies; 8+ messages in thread
From: Jiri Olsa @ 2022-11-22 13:48 UTC (permalink / raw)
  To: Chen Hu
  Cc: jpoimboe, memxor, bpf, Pengfei Xu, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, linux-kernel, netdev, Peter Zijlstra

On Mon, Nov 21, 2022 at 11:32:43PM -0800, Chen Hu wrote:
> With CONFIG_X86_KERNEL_IBT enabled, the test_verifier triggers the
> following BUG:
> 
>   traps: Missing ENDBR: bpf_kfunc_call_test_release+0x0/0x30
>   ------------[ cut here ]------------
>   kernel BUG at arch/x86/kernel/traps.c:254!
>   invalid opcode: 0000 [#1] PREEMPT SMP
>   <TASK>
>    asm_exc_control_protection+0x26/0x50
>   RIP: 0010:bpf_kfunc_call_test_release+0x0/0x30
>   Code: 00 48 c7 c7 18 f2 e1 b4 e8 0d ca 8c ff 48 c7 c0 00 f2 e1 b4 c3
> 	0f 1f 44 00 00 66 0f 1f 00 0f 1f 44 00 00 0f 0b 31 c0 c3 66 90
>        <66> 0f 1f 00 0f 1f 44 00 00 48 85 ff 74 13 4c 8d 47 18 b8 ff ff ff
>    bpf_map_free_kptrs+0x2e/0x70
>    array_map_free+0x57/0x140
>    process_one_work+0x194/0x3a0
>    worker_thread+0x54/0x3a0
>    ? rescuer_thread+0x390/0x390
>    kthread+0xe9/0x110
>    ? kthread_complete_and_exit+0x20/0x20
> 
> This is because there are no compile-time references to the destructor
> kfuncs, bpf_kfunc_call_test_release() for example. So objtool marked
> them sealable and ENDBR in the functions were sealed (converted to NOP)
> by apply_ibt_endbr().
> 
> This fix creates dummy compile-time references to destructor kfuncs so
> ENDBR stay there.
> 
> Fixes: 05a945deefaa ("selftests/bpf: Add verifier tests for kptr")
> Signed-off-by: Chen Hu <hu1.chen@intel.com>
> Tested-by: Pengfei Xu <pengfei.xu@intel.com>
> ---
> v2:
> - Use generic macro name and place the macro after function body as
> - suggested by Jiri Olsa
> 
> v1: https://lore.kernel.org/all/20221121085113.611504-1-hu1.chen@intel.com/
> 
>  include/linux/btf_ids.h | 7 +++++++
>  net/bpf/test_run.c      | 4 ++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> index 2aea877d644f..db02691b506d 100644
> --- a/include/linux/btf_ids.h
> +++ b/include/linux/btf_ids.h
> @@ -266,4 +266,11 @@ MAX_BTF_TRACING_TYPE,
>  
>  extern u32 btf_tracing_ids[];
>  
> +#if defined(CONFIG_X86_KERNEL_IBT) && !defined(__DISABLE_EXPORTS)
> +#define FUNC_IBT_NOSEAL(name)					\
> +	asm(IBT_NOSEAL(#name));
> +#else
> +#define FUNC_IBT_NOSEAL(name)
> +#endif /* CONFIG_X86_KERNEL_IBT */

hum, IBT_NOSEAL is x86 specific, so this will probably fail build
on other archs.. I think we could ifdef it with CONFIG_X86, but
it should go to some IBT related header? surely not to btf_ids.h

cc-ing Peter and Josh

thanks,
jirka


> +
>  #endif
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 13d578ce2a09..07263b7cc12d 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -597,10 +597,14 @@ noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
>  	refcount_dec(&p->cnt);
>  }
>  
> +FUNC_IBT_NOSEAL(bpf_kfunc_call_test_release)
> +
>  noinline void bpf_kfunc_call_memb_release(struct prog_test_member *p)
>  {
>  }
>  
> +FUNC_IBT_NOSEAL(bpf_kfunc_call_memb_release)
> +
>  noinline void bpf_kfunc_call_memb1_release(struct prog_test_member1 *p)
>  {
>  	WARN_ON_ONCE(1);
> -- 
> 2.34.1
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf v2] selftests/bpf: Fix "missing ENDBR" BUG for destructor kfunc
  2022-11-22 13:48 ` Jiri Olsa
@ 2022-11-22 14:14   ` Peter Zijlstra
  2022-11-25 13:44     ` Chen, Hu1
  2022-11-25 13:28   ` Chen, Hu1
  1 sibling, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2022-11-22 14:14 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Chen Hu, jpoimboe, memxor, bpf, Pengfei Xu, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, linux-kernel, netdev

On Tue, Nov 22, 2022 at 02:48:07PM +0100, Jiri Olsa wrote:
> On Mon, Nov 21, 2022 at 11:32:43PM -0800, Chen Hu wrote:
> > With CONFIG_X86_KERNEL_IBT enabled, the test_verifier triggers the
> > following BUG:
> > 
> >   traps: Missing ENDBR: bpf_kfunc_call_test_release+0x0/0x30
> >   ------------[ cut here ]------------
> >   kernel BUG at arch/x86/kernel/traps.c:254!
> >   invalid opcode: 0000 [#1] PREEMPT SMP
> >   <TASK>
> >    asm_exc_control_protection+0x26/0x50
> >   RIP: 0010:bpf_kfunc_call_test_release+0x0/0x30
> >   Code: 00 48 c7 c7 18 f2 e1 b4 e8 0d ca 8c ff 48 c7 c0 00 f2 e1 b4 c3
> > 	0f 1f 44 00 00 66 0f 1f 00 0f 1f 44 00 00 0f 0b 31 c0 c3 66 90
> >        <66> 0f 1f 00 0f 1f 44 00 00 48 85 ff 74 13 4c 8d 47 18 b8 ff ff ff
> >    bpf_map_free_kptrs+0x2e/0x70
> >    array_map_free+0x57/0x140
> >    process_one_work+0x194/0x3a0
> >    worker_thread+0x54/0x3a0
> >    ? rescuer_thread+0x390/0x390
> >    kthread+0xe9/0x110
> >    ? kthread_complete_and_exit+0x20/0x20
> > 
> > This is because there are no compile-time references to the destructor
> > kfuncs, bpf_kfunc_call_test_release() for example. So objtool marked
> > them sealable and ENDBR in the functions were sealed (converted to NOP)
> > by apply_ibt_endbr().

If there is no compile time reference to it, what stops an LTO linker
from throwing it out in the first place?


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf v2] selftests/bpf: Fix "missing ENDBR" BUG for destructor kfunc
  2022-11-22 13:48 ` Jiri Olsa
  2022-11-22 14:14   ` Peter Zijlstra
@ 2022-11-25 13:28   ` Chen, Hu1
  2022-11-27 21:58     ` Jiri Olsa
  1 sibling, 1 reply; 8+ messages in thread
From: Chen, Hu1 @ 2022-11-25 13:28 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: jpoimboe, memxor, bpf, Pengfei Xu, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, linux-kernel, netdev, Peter Zijlstra, Chen, Hu1

On 11/22/2022 9:48 PM, Jiri Olsa wrote:
> On Mon, Nov 21, 2022 at 11:32:43PM -0800, Chen Hu wrote:
>> With CONFIG_X86_KERNEL_IBT enabled, the test_verifier triggers the
>> following BUG:
>>
>>   traps: Missing ENDBR: bpf_kfunc_call_test_release+0x0/0x30
>>   ------------[ cut here ]------------
>>   kernel BUG at arch/x86/kernel/traps.c:254!
>>   invalid opcode: 0000 [#1] PREEMPT SMP
>>   <TASK>
>>    asm_exc_control_protection+0x26/0x50
>>   RIP: 0010:bpf_kfunc_call_test_release+0x0/0x30
>>   Code: 00 48 c7 c7 18 f2 e1 b4 e8 0d ca 8c ff 48 c7 c0 00 f2 e1 b4 c3
>> 	0f 1f 44 00 00 66 0f 1f 00 0f 1f 44 00 00 0f 0b 31 c0 c3 66 90
>>        <66> 0f 1f 00 0f 1f 44 00 00 48 85 ff 74 13 4c 8d 47 18 b8 ff ff ff
>>    bpf_map_free_kptrs+0x2e/0x70
>>    array_map_free+0x57/0x140
>>    process_one_work+0x194/0x3a0
>>    worker_thread+0x54/0x3a0
>>    ? rescuer_thread+0x390/0x390
>>    kthread+0xe9/0x110
>>    ? kthread_complete_and_exit+0x20/0x20
>>
>> This is because there are no compile-time references to the destructor
>> kfuncs, bpf_kfunc_call_test_release() for example. So objtool marked
>> them sealable and ENDBR in the functions were sealed (converted to NOP)
>> by apply_ibt_endbr().
>>
>> This fix creates dummy compile-time references to destructor kfuncs so
>> ENDBR stay there.
>>
>> Fixes: 05a945deefaa ("selftests/bpf: Add verifier tests for kptr")
>> Signed-off-by: Chen Hu <hu1.chen@intel.com>
>> Tested-by: Pengfei Xu <pengfei.xu@intel.com>
>> ---
>> v2:
>> - Use generic macro name and place the macro after function body as
>> - suggested by Jiri Olsa
>>
>> v1: https://lore.kernel.org/all/20221121085113.611504-1-hu1.chen@intel.com/
>>
>>  include/linux/btf_ids.h | 7 +++++++
>>  net/bpf/test_run.c      | 4 ++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
>> index 2aea877d644f..db02691b506d 100644
>> --- a/include/linux/btf_ids.h
>> +++ b/include/linux/btf_ids.h
>> @@ -266,4 +266,11 @@ MAX_BTF_TRACING_TYPE,
>>  
>>  extern u32 btf_tracing_ids[];
>>  
>> +#if defined(CONFIG_X86_KERNEL_IBT) && !defined(__DISABLE_EXPORTS)
>> +#define FUNC_IBT_NOSEAL(name)					\
>> +	asm(IBT_NOSEAL(#name));
>> +#else
>> +#define FUNC_IBT_NOSEAL(name)
>> +#endif /* CONFIG_X86_KERNEL_IBT */
> 
> hum, IBT_NOSEAL is x86 specific, so this will probably fail build
> on other archs.. I think we could ifdef it with CONFIG_X86, but
> it should go to some IBT related header? surely not to btf_ids.h
> 
> cc-ing Peter and Josh
> 
> thanks,
> jirka
>

The lkp reports build success because X86_KERNEL_IBT alredy depends on
X86_64.

Currently, arch/x86/include/asm/ibt.h which defines macro IBT_NOSEAL is
x86 specific. How about we just put asm at test_run.c directly (ugly?):

#if defined(CONFIG_X86_KERNEL_IBT) && !defined(__DISABLE_EXPORTS)
asm(IBT_NOSEAL("bpf_kfunc_call_test_release"));
asm(IBT_NOSEAL("bpf_kfunc_call_memb_release"));
#endif

thanks
Chen Hu

> 
>> +
>>  #endif
>> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
>> index 13d578ce2a09..07263b7cc12d 100644
>> --- a/net/bpf/test_run.c
>> +++ b/net/bpf/test_run.c
>> @@ -597,10 +597,14 @@ noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
>>  	refcount_dec(&p->cnt);
>>  }
>>  
>> +FUNC_IBT_NOSEAL(bpf_kfunc_call_test_release)
>> +
>>  noinline void bpf_kfunc_call_memb_release(struct prog_test_member *p)
>>  {
>>  }
>>  
>> +FUNC_IBT_NOSEAL(bpf_kfunc_call_memb_release)
>> +
>>  noinline void bpf_kfunc_call_memb1_release(struct prog_test_member1 *p)
>>  {
>>  	WARN_ON_ONCE(1);
>> -- 
>> 2.34.1
>>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf v2] selftests/bpf: Fix "missing ENDBR" BUG for destructor kfunc
  2022-11-22 14:14   ` Peter Zijlstra
@ 2022-11-25 13:44     ` Chen, Hu1
  2022-11-27 22:04       ` Jiri Olsa
  0 siblings, 1 reply; 8+ messages in thread
From: Chen, Hu1 @ 2022-11-25 13:44 UTC (permalink / raw)
  To: Peter Zijlstra, Jiri Olsa
  Cc: jpoimboe, memxor, bpf, Pengfei Xu, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, linux-kernel, netdev, Chen, Hu1

On 11/22/2022 10:14 PM, Peter Zijlstra wrote:
> On Tue, Nov 22, 2022 at 02:48:07PM +0100, Jiri Olsa wrote:
>> On Mon, Nov 21, 2022 at 11:32:43PM -0800, Chen Hu wrote:
>>> With CONFIG_X86_KERNEL_IBT enabled, the test_verifier triggers the
>>> following BUG:
>>>
>>>   traps: Missing ENDBR: bpf_kfunc_call_test_release+0x0/0x30
>>>   ------------[ cut here ]------------
>>>   kernel BUG at arch/x86/kernel/traps.c:254!
>>>   invalid opcode: 0000 [#1] PREEMPT SMP
>>>   <TASK>
>>>    asm_exc_control_protection+0x26/0x50
>>>   RIP: 0010:bpf_kfunc_call_test_release+0x0/0x30
>>>   Code: 00 48 c7 c7 18 f2 e1 b4 e8 0d ca 8c ff 48 c7 c0 00 f2 e1 b4 c3
>>> 	0f 1f 44 00 00 66 0f 1f 00 0f 1f 44 00 00 0f 0b 31 c0 c3 66 90
>>>        <66> 0f 1f 00 0f 1f 44 00 00 48 85 ff 74 13 4c 8d 47 18 b8 ff ff ff
>>>    bpf_map_free_kptrs+0x2e/0x70
>>>    array_map_free+0x57/0x140
>>>    process_one_work+0x194/0x3a0
>>>    worker_thread+0x54/0x3a0
>>>    ? rescuer_thread+0x390/0x390
>>>    kthread+0xe9/0x110
>>>    ? kthread_complete_and_exit+0x20/0x20
>>>
>>> This is because there are no compile-time references to the destructor
>>> kfuncs, bpf_kfunc_call_test_release() for example. So objtool marked
>>> them sealable and ENDBR in the functions were sealed (converted to NOP)
>>> by apply_ibt_endbr().
> 
> If there is no compile time reference to it, what stops an LTO linker
> from throwing it out in the first place?
>

Ah, my stupid.

The only references to this function from kernel space are:
    $ grep -r bpf_kfunc_call_test_release
    net/bpf/test_run.c:noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
    net/bpf/test_run.c:BTF_ID_FLAGS(func, bpf_kfunc_call_test_release, KF_RELEASE)
    net/bpf/test_run.c:BTF_ID(func, bpf_kfunc_call_test_release)

Macro BTF_ID_... puts the function names to .BTF_ids section. It looks
like:
__BTF_ID__func__bpf_kfunc_call_test_release__692

When running, it uses kallsyms_lookup_name() to find the function
address via names in .BTF_ids section.


Hi jirka,
Please kindly correct me if my understanding of BTF_ids is wrong.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf v2] selftests/bpf: Fix "missing ENDBR" BUG for destructor kfunc
  2022-11-25 13:28   ` Chen, Hu1
@ 2022-11-27 21:58     ` Jiri Olsa
  0 siblings, 0 replies; 8+ messages in thread
From: Jiri Olsa @ 2022-11-27 21:58 UTC (permalink / raw)
  To: Chen, Hu1
  Cc: Jiri Olsa, jpoimboe, memxor, bpf, Pengfei Xu, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, linux-kernel, netdev, Peter Zijlstra

On Fri, Nov 25, 2022 at 09:28:28PM +0800, Chen, Hu1 wrote:
> On 11/22/2022 9:48 PM, Jiri Olsa wrote:
> > On Mon, Nov 21, 2022 at 11:32:43PM -0800, Chen Hu wrote:
> >> With CONFIG_X86_KERNEL_IBT enabled, the test_verifier triggers the
> >> following BUG:
> >>
> >>   traps: Missing ENDBR: bpf_kfunc_call_test_release+0x0/0x30
> >>   ------------[ cut here ]------------
> >>   kernel BUG at arch/x86/kernel/traps.c:254!
> >>   invalid opcode: 0000 [#1] PREEMPT SMP
> >>   <TASK>
> >>    asm_exc_control_protection+0x26/0x50
> >>   RIP: 0010:bpf_kfunc_call_test_release+0x0/0x30
> >>   Code: 00 48 c7 c7 18 f2 e1 b4 e8 0d ca 8c ff 48 c7 c0 00 f2 e1 b4 c3
> >> 	0f 1f 44 00 00 66 0f 1f 00 0f 1f 44 00 00 0f 0b 31 c0 c3 66 90
> >>        <66> 0f 1f 00 0f 1f 44 00 00 48 85 ff 74 13 4c 8d 47 18 b8 ff ff ff
> >>    bpf_map_free_kptrs+0x2e/0x70
> >>    array_map_free+0x57/0x140
> >>    process_one_work+0x194/0x3a0
> >>    worker_thread+0x54/0x3a0
> >>    ? rescuer_thread+0x390/0x390
> >>    kthread+0xe9/0x110
> >>    ? kthread_complete_and_exit+0x20/0x20
> >>
> >> This is because there are no compile-time references to the destructor
> >> kfuncs, bpf_kfunc_call_test_release() for example. So objtool marked
> >> them sealable and ENDBR in the functions were sealed (converted to NOP)
> >> by apply_ibt_endbr().
> >>
> >> This fix creates dummy compile-time references to destructor kfuncs so
> >> ENDBR stay there.
> >>
> >> Fixes: 05a945deefaa ("selftests/bpf: Add verifier tests for kptr")
> >> Signed-off-by: Chen Hu <hu1.chen@intel.com>
> >> Tested-by: Pengfei Xu <pengfei.xu@intel.com>
> >> ---
> >> v2:
> >> - Use generic macro name and place the macro after function body as
> >> - suggested by Jiri Olsa
> >>
> >> v1: https://lore.kernel.org/all/20221121085113.611504-1-hu1.chen@intel.com/
> >>
> >>  include/linux/btf_ids.h | 7 +++++++
> >>  net/bpf/test_run.c      | 4 ++++
> >>  2 files changed, 11 insertions(+)
> >>
> >> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> >> index 2aea877d644f..db02691b506d 100644
> >> --- a/include/linux/btf_ids.h
> >> +++ b/include/linux/btf_ids.h
> >> @@ -266,4 +266,11 @@ MAX_BTF_TRACING_TYPE,
> >>  
> >>  extern u32 btf_tracing_ids[];
> >>  
> >> +#if defined(CONFIG_X86_KERNEL_IBT) && !defined(__DISABLE_EXPORTS)
> >> +#define FUNC_IBT_NOSEAL(name)					\
> >> +	asm(IBT_NOSEAL(#name));
> >> +#else
> >> +#define FUNC_IBT_NOSEAL(name)
> >> +#endif /* CONFIG_X86_KERNEL_IBT */
> > 
> > hum, IBT_NOSEAL is x86 specific, so this will probably fail build
> > on other archs.. I think we could ifdef it with CONFIG_X86, but
> > it should go to some IBT related header? surely not to btf_ids.h
> > 
> > cc-ing Peter and Josh
> > 
> > thanks,
> > jirka
> >
> 
> The lkp reports build success because X86_KERNEL_IBT alredy depends on
> X86_64.

ah right, so please just move it to some other header

jirka

> 
> Currently, arch/x86/include/asm/ibt.h which defines macro IBT_NOSEAL is
> x86 specific. How about we just put asm at test_run.c directly (ugly?):
> 
> #if defined(CONFIG_X86_KERNEL_IBT) && !defined(__DISABLE_EXPORTS)
> asm(IBT_NOSEAL("bpf_kfunc_call_test_release"));
> asm(IBT_NOSEAL("bpf_kfunc_call_memb_release"));
> #endif
> 
> thanks
> Chen Hu
> 
> > 
> >> +
> >>  #endif
> >> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> >> index 13d578ce2a09..07263b7cc12d 100644
> >> --- a/net/bpf/test_run.c
> >> +++ b/net/bpf/test_run.c
> >> @@ -597,10 +597,14 @@ noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
> >>  	refcount_dec(&p->cnt);
> >>  }
> >>  
> >> +FUNC_IBT_NOSEAL(bpf_kfunc_call_test_release)
> >> +
> >>  noinline void bpf_kfunc_call_memb_release(struct prog_test_member *p)
> >>  {
> >>  }
> >>  
> >> +FUNC_IBT_NOSEAL(bpf_kfunc_call_memb_release)
> >> +
> >>  noinline void bpf_kfunc_call_memb1_release(struct prog_test_member1 *p)
> >>  {
> >>  	WARN_ON_ONCE(1);
> >> -- 
> >> 2.34.1
> >>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf v2] selftests/bpf: Fix "missing ENDBR" BUG for destructor kfunc
  2022-11-25 13:44     ` Chen, Hu1
@ 2022-11-27 22:04       ` Jiri Olsa
  2022-11-27 22:13         ` Alexei Starovoitov
  0 siblings, 1 reply; 8+ messages in thread
From: Jiri Olsa @ 2022-11-27 22:04 UTC (permalink / raw)
  To: Chen, Hu1
  Cc: Peter Zijlstra, Jiri Olsa, jpoimboe, memxor, bpf, Pengfei Xu,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Stanislav Fomichev, Hao Luo, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-kernel, netdev

On Fri, Nov 25, 2022 at 09:44:29PM +0800, Chen, Hu1 wrote:
> On 11/22/2022 10:14 PM, Peter Zijlstra wrote:
> > On Tue, Nov 22, 2022 at 02:48:07PM +0100, Jiri Olsa wrote:
> >> On Mon, Nov 21, 2022 at 11:32:43PM -0800, Chen Hu wrote:
> >>> With CONFIG_X86_KERNEL_IBT enabled, the test_verifier triggers the
> >>> following BUG:
> >>>
> >>>   traps: Missing ENDBR: bpf_kfunc_call_test_release+0x0/0x30
> >>>   ------------[ cut here ]------------
> >>>   kernel BUG at arch/x86/kernel/traps.c:254!
> >>>   invalid opcode: 0000 [#1] PREEMPT SMP
> >>>   <TASK>
> >>>    asm_exc_control_protection+0x26/0x50
> >>>   RIP: 0010:bpf_kfunc_call_test_release+0x0/0x30
> >>>   Code: 00 48 c7 c7 18 f2 e1 b4 e8 0d ca 8c ff 48 c7 c0 00 f2 e1 b4 c3
> >>> 	0f 1f 44 00 00 66 0f 1f 00 0f 1f 44 00 00 0f 0b 31 c0 c3 66 90
> >>>        <66> 0f 1f 00 0f 1f 44 00 00 48 85 ff 74 13 4c 8d 47 18 b8 ff ff ff
> >>>    bpf_map_free_kptrs+0x2e/0x70
> >>>    array_map_free+0x57/0x140
> >>>    process_one_work+0x194/0x3a0
> >>>    worker_thread+0x54/0x3a0
> >>>    ? rescuer_thread+0x390/0x390
> >>>    kthread+0xe9/0x110
> >>>    ? kthread_complete_and_exit+0x20/0x20
> >>>
> >>> This is because there are no compile-time references to the destructor
> >>> kfuncs, bpf_kfunc_call_test_release() for example. So objtool marked
> >>> them sealable and ENDBR in the functions were sealed (converted to NOP)
> >>> by apply_ibt_endbr().
> > 
> > If there is no compile time reference to it, what stops an LTO linker
> > from throwing it out in the first place?
> >
> 
> Ah, my stupid.
> 
> The only references to this function from kernel space are:
>     $ grep -r bpf_kfunc_call_test_release
>     net/bpf/test_run.c:noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
>     net/bpf/test_run.c:BTF_ID_FLAGS(func, bpf_kfunc_call_test_release, KF_RELEASE)
>     net/bpf/test_run.c:BTF_ID(func, bpf_kfunc_call_test_release)
> 
> Macro BTF_ID_... puts the function names to .BTF_ids section. It looks
> like:
> __BTF_ID__func__bpf_kfunc_call_test_release__692

bpf_kfunc_call_test_release test function called bpf program as kfunc
(check tools/testing/selftests/bpf/progs/*.c)

it's placed in BTF ID lists so verifier can validate its ID when called
from bpf program.. it has no other caller from kernel side

jirka

> 
> When running, it uses kallsyms_lookup_name() to find the function
> address via names in .BTF_ids section.
> 
> 
> Hi jirka,
> Please kindly correct me if my understanding of BTF_ids is wrong.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf v2] selftests/bpf: Fix "missing ENDBR" BUG for destructor kfunc
  2022-11-27 22:04       ` Jiri Olsa
@ 2022-11-27 22:13         ` Alexei Starovoitov
  0 siblings, 0 replies; 8+ messages in thread
From: Alexei Starovoitov @ 2022-11-27 22:13 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Chen, Hu1, Peter Zijlstra, Josh Poimboeuf,
	Kumar Kartikeya Dwivedi, bpf, Pengfei Xu, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, LKML, Network Development

On Sun, Nov 27, 2022 at 2:05 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Fri, Nov 25, 2022 at 09:44:29PM +0800, Chen, Hu1 wrote:
> > On 11/22/2022 10:14 PM, Peter Zijlstra wrote:
> > > On Tue, Nov 22, 2022 at 02:48:07PM +0100, Jiri Olsa wrote:
> > >> On Mon, Nov 21, 2022 at 11:32:43PM -0800, Chen Hu wrote:
> > >>> With CONFIG_X86_KERNEL_IBT enabled, the test_verifier triggers the
> > >>> following BUG:
> > >>>
> > >>>   traps: Missing ENDBR: bpf_kfunc_call_test_release+0x0/0x30
> > >>>   ------------[ cut here ]------------
> > >>>   kernel BUG at arch/x86/kernel/traps.c:254!
> > >>>   invalid opcode: 0000 [#1] PREEMPT SMP
> > >>>   <TASK>
> > >>>    asm_exc_control_protection+0x26/0x50
> > >>>   RIP: 0010:bpf_kfunc_call_test_release+0x0/0x30
> > >>>   Code: 00 48 c7 c7 18 f2 e1 b4 e8 0d ca 8c ff 48 c7 c0 00 f2 e1 b4 c3
> > >>>   0f 1f 44 00 00 66 0f 1f 00 0f 1f 44 00 00 0f 0b 31 c0 c3 66 90
> > >>>        <66> 0f 1f 00 0f 1f 44 00 00 48 85 ff 74 13 4c 8d 47 18 b8 ff ff ff
> > >>>    bpf_map_free_kptrs+0x2e/0x70
> > >>>    array_map_free+0x57/0x140
> > >>>    process_one_work+0x194/0x3a0
> > >>>    worker_thread+0x54/0x3a0
> > >>>    ? rescuer_thread+0x390/0x390
> > >>>    kthread+0xe9/0x110
> > >>>    ? kthread_complete_and_exit+0x20/0x20
> > >>>
> > >>> This is because there are no compile-time references to the destructor
> > >>> kfuncs, bpf_kfunc_call_test_release() for example. So objtool marked
> > >>> them sealable and ENDBR in the functions were sealed (converted to NOP)
> > >>> by apply_ibt_endbr().
> > >
> > > If there is no compile time reference to it, what stops an LTO linker
> > > from throwing it out in the first place?
> > >
> >
> > Ah, my stupid.
> >
> > The only references to this function from kernel space are:
> >     $ grep -r bpf_kfunc_call_test_release
> >     net/bpf/test_run.c:noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
> >     net/bpf/test_run.c:BTF_ID_FLAGS(func, bpf_kfunc_call_test_release, KF_RELEASE)
> >     net/bpf/test_run.c:BTF_ID(func, bpf_kfunc_call_test_release)
> >
> > Macro BTF_ID_... puts the function names to .BTF_ids section. It looks
> > like:
> > __BTF_ID__func__bpf_kfunc_call_test_release__692
>
> bpf_kfunc_call_test_release test function called bpf program as kfunc
> (check tools/testing/selftests/bpf/progs/*.c)
>
> it's placed in BTF ID lists so verifier can validate its ID when called
> from bpf program.. it has no other caller from kernel side

They were added when we had no ability to call kfuncs from modules.
Now we should probably move all of them to bpf_testmod.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-11-27 22:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-22  7:32 [PATCH bpf v2] selftests/bpf: Fix "missing ENDBR" BUG for destructor kfunc Chen Hu
2022-11-22 13:48 ` Jiri Olsa
2022-11-22 14:14   ` Peter Zijlstra
2022-11-25 13:44     ` Chen, Hu1
2022-11-27 22:04       ` Jiri Olsa
2022-11-27 22:13         ` Alexei Starovoitov
2022-11-25 13:28   ` Chen, Hu1
2022-11-27 21:58     ` Jiri Olsa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).