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@xxxxxxxxx>
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@xxxxxxxxxx>
Cc: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>
Cc: david.faust@xxxxxxxxxx
Cc: cupertino.miranda@xxxxxxxxxx
---
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")))
