Re: [PATCH bpf-next] bpf: fix bpf_ksym_exists in GCC

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

On 5/2/24 10:44 AM, Jose E. Marchesi wrote:
>> On 4/28/24 4:25 AM, Jose E. Marchesi wrote:
>>> The macro bpf_ksym_exists is defined in bpf_helpers.h as:
>>>
>>>     #define bpf_ksym_exists(sym) ({								\
>>>     	_Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak");	\
>>>     	!!sym;											\
>>>     })
>>>
>>> The purpose of the macro is to determine whether a given symbol has
>>> been defined, given the address of the object associated with the
>>> symbol.  It also has a compile-time check to make sure the object
>>> whose address is passed to the macro has been declared as weak, which
>>> makes the check on `sym' meaningful.
>>>
>>> As it happens, the check for weak doesn't work in GCC in all cases,
>>> because __builtin_constant_p not always folds at parse time when
>>> optimizing.  This is because optimizations that happen later in the
>>> compilation process, like inlining, may make a previously non-constant
>>> expression a constant.  This results in errors like the following when
>>> building the selftests with GCC:
>>>
>>>     bpf_helpers.h:190:24: error: expression in static assertion is not constant
>>>     190 |         _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak");       \
>>>         |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Fortunately recent versions of GCC support a __builtin_has_attribute
>>> that can be used to directly check for the __weak__ attribute.  This
>>> patch changes bpf_helpers.h to use that builtin when building with a
>>> recent enough GCC, and to omit the check if GCC is too old to support
>>> the builtin.
>>>
>>> The macro used for GCC becomes:
>>>
>>>     #define bpf_ksym_exists(sym) ({									\
>>> 	_Static_assert(__builtin_has_attribute (*sym, __weak__), #sym " should be marked as __weak");	\
>>> 	!!sym;												\
>>>     })
>>>
>>> Note that since bpf_ksym_exists is designed to get the address of the
>>> object associated with symbol SYM, we pass *sym to
>>> __builtin_has_attribute instead of sym.  When an expression is passed
>>> to __builtin_has_attribute then it is the type of the passed
>>> expression that is checked for the specified attribute.  The
>>> expression itself is not evaluated.  This accommodates well with the
>>> existing usages of the macro:
>>>
>>> - For function objects:
>>>
>>>     struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym __weak;
>>>     [...]
>>>     bpf_ksym_exists(bpf_task_acquire)
>>>
>>> - For variable objects:
>>>
>>>     extern const struct rq runqueues __ksym __weak; /* typed */
>>>     [...]
>>>     bpf_ksym_exists(&runqueues)
>>>
>>> Note also that BPF support was added in GCC 10 and support for
>>> __builtin_has_attribute in GCC 9.
>> It would be great if you can share details with asm code and
>> BTF so we can understand better. I am not 100% sure about
>> whether __builtin_has_attribute builtin can help to do
>> run-time ksym resolution with libbpf.
> Hi Yonghong.
>
> I am a bit confused.  Is the _Static_assert supposed to contribute
> anything to the generated code?

No it is not. It is used to check whether __weak attribute is added to
the symbol or not.

>
> This is what GCC generates for pass_handler:
>
> -----
> pass_handler:
> .LFB1:
> 	r2 = 0
> 	r1 = runqueues ll
> 	call	153
> 	if r0 == 0 goto .L2
> 	r1 = runqueues ll
> 	if r1 == 0 goto .L2
> 	r2 = out__existing_typed ll
> 	r0 = *(u32 *) (r0+2920)
> 	*(u32 *) (r2+0) = r0
> .L2:
> 	r6 = out__non_existent_typed ll
> 	r1 = bpf_link_fops2 ll
> 	r3 = out__existing_typeless ll
> 	r4 = bpf_prog_active ll
> 	r5 = out__non_existent_typeless ll
> 	r9 = bpf_link_fops1 ll
> 	*(u64 *) (r3+0) = r4
> 	*(u64 *) (r5+0) = r9
> 	*(u64 *) (r6+0) = r1
> 	if r1 == 0 goto .L3
> 	r2 = 0
> 	call	153
> 	*(u64 *) (r6+0) = r0
> .L3:
> 	r1 = bpf_task_acquire ll
> 	if r1 == 0 goto .L20
> .L4:
> 	r1 = bpf_testmod_test_mod_kfunc ll
> 	if r1 == 0 goto .L21
> .L5:
> 	r1 = invalid_kfunc ll
> 	if r1 == 0 goto .L6
> 	call	invalid_kfunc
> .L6:
> 	r0 = 0
> 	exit
> .L21:
> 	call	bpf_testmod_test_mod_kfunc
> 	goto .L5
> .L20:
> 	call	bpf_task_acquire
> 	goto .L4
> .LFE1:
> 	.size	pass_handler, .-pass_handler
> -----
>
> And the .ksyms datasec:
>
> -----
> [7693] DATASEC '.ksyms' size=0 vlen=7
> 	type_id=7690 offset=0 size=0 (FUNC 'invalid_kfunc')
> 	type_id=7691 offset=0 size=0 (FUNC 'bpf_testmod_test_mod_kfunc')
> 	type_id=7692 offset=0 size=0 (FUNC 'bpf_task_acquire')
> 	type_id=7530 offset=0 size=4 (VAR 'bpf_link_fops2')
> 	type_id=7550 offset=0 size=1 (VAR 'bpf_link_fops1')
> 	type_id=7475 offset=0 size=1 (VAR 'bpf_prog_active')
> 	type_id=7535 offset=0 size=3456 (VAR 'runqueues')
> -----
>
> Is the entry for runqueues en the datasec enough for libbpf to patch the
> ksym value in the corresponding `r1 = runqueues ll' instructions

It should be okay. libbpf will patch `r1 = runqueues ll` with
`r1 = <btf_obj_fd/btf_id>` and the kernel will translate it to
`r1 = <kernel addr of runqueues>`.

Based on your above output, the patch looks good to me.

Acked-by: Yonghong Song <yonghong.song@linux.dev>

>
>> The following is what clang does:
>>
>> For example, for progs/test_ksyms_weak.c, we have
>>   43         if (rq && bpf_ksym_exists(&runqueues))
>>   44                 out__existing_typed = rq->cpu;
>> ...
>>   56         if (!bpf_ksym_exists(bpf_task_acquire))
>>   57                 /* dead code won't be seen by the verifier */
>>   58                 bpf_task_acquire(0);
>>
>> The asm code:
>>
>>          .loc    0 42 20 prologue_end            # progs/test_ksyms_weak.c:42:20
>> .Ltmp0:
>>          r6 = runqueues ll
>>          r1 = runqueues ll
>>          w2 = 0
>>          call 153
>> .Ltmp1:
>> .Ltmp2:
>>          #DEBUG_VALUE: pass_handler:rq <- $r0
>>          .loc    0 43 9                          # progs/test_ksyms_weak.c:43:9
>> .Ltmp3:
>>          if r0 == 0 goto LBB0_3
>> .Ltmp4:
>> .Ltmp5:
>> # %bb.1:                                # %entry
>>          #DEBUG_VALUE: pass_handler:rq <- $r0
>>          if r6 == 0 goto LBB0_3
>> ...
>> LBB0_5:                                 # %if.end4
>>          .loc    0 56 6 is_stmt 1                # progs/test_ksyms_weak.c:56:6
>> .Ltmp25:
>>          r1 = bpf_task_acquire ll
>>          if r1 != 0 goto LBB0_7
>> # %bb.6:                                # %if.then9
>>
>> Here, 'runqueues' and 'bpf_task_acquire' will be changed by libbpf
>> based on the *current* kernel state. The BTF datasec encodes such ksym
>> information like below which will be used by libbpf:
>>
>>          .long   13079                           # BTF_KIND_DATASEC(id = 395)
>>          .long   251658247                       # 0xf000007
>>          .long   0
>>          .long   377
>>          .long   bpf_task_acquire
>>          .long   0
>>          .long   379
>>          .long   bpf_testmod_test_mod_kfunc
>>          .long   0
>>          .long   381
>>          .long   invalid_kfunc
>>          .long   0
>>          .long   387
>>          .long   runqueues
>>          .long   3264
>>          .long   388
>>          .long   bpf_prog_active
>>          .long   1
>>          .long   389
>>          .long   bpf_link_fops1
>>          .long   1
>>          .long   391
>>          .long   bpf_link_fops2
>>          .long   4
>>
>> What gcc generates for the above example? It would be great
>> if this can be put in the commit message.
>>
>>> Locally tested in bpf-next master branch.
>>> No regressions.
>>>
>>> Signed-of-by: Jose E. Marchesi <jose.marchesi@oracle.com>
>>> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>>> Cc: david.faust@oracle.com
>>> Cc: cupertino.miranda@oracle.com
>>> ---
>>>    tools/lib/bpf/bpf_helpers.h | 9 +++++++++
>>>    1 file changed, 9 insertions(+)
>>>
>>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
>>> index 62e1c0cc4a59..a720636a87d9 100644
>>> --- a/tools/lib/bpf/bpf_helpers.h
>>> +++ b/tools/lib/bpf/bpf_helpers.h
>>> @@ -186,10 +186,19 @@ enum libbpf_tristate {
>>>    #define __kptr __attribute__((btf_type_tag("kptr")))
>>>    #define __percpu_kptr __attribute__((btf_type_tag("percpu_kptr")))
>>>    +#if defined (__clang__)
>>>    #define bpf_ksym_exists(sym) ({									\
>>>    	_Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak");	\
>>>    	!!sym;											\
>>>    })
>>> +#elif __GNUC__ > 8
>> | +#define bpf_ksym_exists(sym) ({									\
>>
>>> +	_Static_assert(__builtin_has_attribute (*sym, __weak__), #sym " should be marked as __weak");	\
>>> +	!!sym;												\
>>> +})
>>> +#else
>>> +#define bpf_ksym_exists(sym) !!sym
>>> +#endif
>>>      #define __arg_ctx __attribute__((btf_decl_tag("arg:ctx")))
>>>    #define __arg_nonnull __attribute((btf_decl_tag("arg:nonnull")))