Re: [PATCH bpf v2] selftests/bpf: Fix "missing ENDBR" BUG for destructor kfunc

[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@xxxxxxxxx>
>> Tested-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxx/
>>
>>  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
>>