On 8/19/20 3:40 PM, Hao Luo wrote:

Pseudo_btf_id is a type of ld_imm insn that associates a btf_id to a ksym so that further dereferences on the ksym can use the BTF info to validate accesses. Internally, when seeing a pseudo_btf_id ld insn, the verifier reads the btf_id stored in the insn[0]'s imm field and marks the dst_reg as PTR_TO_BTF_ID. The btf_id points to a VAR_KIND, which is encoded in btf_vminux by pahole. If the VAR is not of a struct type, the dst reg will be marked as PTR_TO_MEM instead of PTR_TO_BTF_ID and the mem_size is resolved to the size of the VAR's type.

From the VAR btf_id, the verifier can also read the address of the ksym's corresponding kernel var from kallsyms and use that to fill dst_reg.

Therefore, the proper functionality of pseudo_btf_id depends on (1) kallsyms and (2) the encoding of kernel global VARs in pahole, which should be available since pahole v1.18.

I tried your patch with latest pahole but it did not generate expected BTF_TYPE_VARs. My pahole head is: f3d9054ba8ff btf_encoder: Teach pahole to store percpu variables in vmlinux BTF.

First I made the following changes to facilitate debugging: diff --git a/btf_encoder.c b/btf_encoder.c index 982f59d..f94c3a6 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -334,6 +334,9 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force) /* percpu variables are allocated in global space */ if (variable__scope(var) != VSCOPE_GLOBAL) continue; + /* type 0 is void, probably an internal error */ + if (var->ip.tag.type == 0) + continue; has_global_var = true; head = &hash_addr[hashaddr__fn(var->ip.addr)]; hlist_add_head(&var->tool_hnode, head); @@ -399,8 +402,8 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force) }

if (verbose) - printf("symbol '%s' of address 0x%lx encoded\n", - sym_name, addr); + printf("symbol '%s' of address 0x%lx encoded, type %u\n", + sym_name, addr, type);

/* add a BTF_KIND_VAR in btfe->types */ linkage = var->external ? BTF_VAR_GLOBAL_ALLOCATED : BTF_VAR_STATIC; diff --git a/libbtf.c b/libbtf.c index 7a01ded..3a0d8d7 100644 --- a/libbtf.c +++ b/libbtf.c @@ -304,6 +304,8 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = { [BTF_KIND_RESTRICT] = "RESTRICT", [BTF_KIND_FUNC] = "FUNC", [BTF_KIND_FUNC_PROTO] = "FUNC_PROTO", + [BTF_KIND_VAR] = "VAR", + [BTF_KIND_DATASEC] = "DATASEC", };

static const char *btf_elf__name_in_gobuf(const struct btf_elf *btfe, uint32_t offset) @@ -671,7 +673,7 @@ int32_t btf_elf__add_var_type(struct btf_elf *btfe, uint32_t type, uint32_t name return -1; }

- btf_elf__log_type(btfe, &t.type, false, false, "type=%u name=%s", + btf_elf__log_type(btfe, &t.type, false, false, "type=%u name=%s\n", t.type.type, btf_elf__name_in_gobuf(btfe, t.type.name_off));

return btfe->type_index;

It would be good if you can add some of the above changes to pahole for easier `pahole -JV` dump.

With the above change, I only got static per cpu variables. For example, static DEFINE_PER_CPU(unsigned int , mirred_rec_level); in net/sched/act_mirred.c.

[10] INT 'unsigned int' size=4 bits_offset=0 nr_bits=32 encoding=(none) [74536] VAR 'mirred_rec_level' type_id=10, linkage=static

The dwarf debug_info entry for `mirred_rec_level`: 0x0001d8d6: DW_TAG_variable DW_AT_name ("mirred_rec_level") DW_AT_decl_file ("/data/users/yhs/work/net-next/net/sched/act_mirred.c") DW_AT_decl_line (31) DW_AT_decl_column (0x08) DW_AT_type (0x00000063 "unsigned int") DW_AT_location (DW_OP_addr 0x0) It is not a declaration and it contains type.

All global per cpu variables do not have BTF_KIND_VAR generated. I did a brief investigation and found this mostly like to be a pahole issue. For example, for global per cpu variable bpf_prog_active, include/linux/bpf.h DECLARE_PER_CPU(int , bpf_prog_active); kernel/bpf/syscall.c DEFINE_PER_CPU(int , bpf_prog_active); it is declared in the header include/linux/bpf.h and defined in kernel/bpf/syscall.c.

In many cu's, you will see: 0x0003592a: DW_TAG_variable DW_AT_name ("bpf_prog_active") DW_AT_decl_file ("/data/users/yhs/work/net-next/include/linux/bpf.h") DW_AT_decl_line (1074) DW_AT_decl_column (0x01) DW_AT_type (0x0001fa7e "int") DW_AT_external (true) DW_AT_declaration (true)

In kernel/bpf/syscall.c, I see the following dwarf entry for real definition: 0x00013534: DW_TAG_variable DW_AT_name ("bpf_prog_active") DW_AT_decl_file ("/data/users/yhs/work/net-next/include/linux/bpf.h") DW_AT_decl_line (1074) DW_AT_decl_column (0x01) DW_AT_type (0x000000d6 "int") DW_AT_external (true) DW_AT_declaration (true)

0x00021a25: DW_TAG_variable DW_AT_specification (0x00013534 "bpf_prog_active") DW_AT_decl_file ("/data/users/yhs/work/net-next/kernel/bpf/syscall.c") DW_AT_decl_line (43) DW_AT_location (DW_OP_addr 0x0)

Note that for the second entry DW_AT_specification points to the declaration. I am not 100% sure whether pahole handle this properly or not. It generates a type id 0 (void) for bpf_prog_active variable.

Could you investigate this a little more?

I am using gcc 8.2.1. Using kernel default dwarf (dwarf 2) exposed the above issue. Tries to use dwarf 4 and the problem still exists.

Signed-off-by: Hao Luo haoluo@google.com

include/linux/btf.h | 15 +++++++++ include/uapi/linux/bpf.h | 38 ++++++++++++++++------ kernel/bpf/btf.c | 15 --------- kernel/bpf/verifier.c | 68 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 24 deletions(-)

[...]

On 8/19/20 3:40 PM, Hao Luo wrote:

Pseudo_btf_id is a type of ld_imm insn that associates a btf_id to a ksym so that further dereferences on the ksym can use the BTF info to validate accesses. Internally, when seeing a pseudo_btf_id ld insn, the verifier reads the btf_id stored in the insn[0]'s imm field and marks the dst_reg as PTR_TO_BTF_ID. The btf_id points to a VAR_KIND, which is encoded in btf_vminux by pahole. If the VAR is not of a struct type, the dst reg will be marked as PTR_TO_MEM instead of PTR_TO_BTF_ID and the mem_size is resolved to the size of the VAR's type.

From the VAR btf_id, the verifier can also read the address of the ksym's corresponding kernel var from kallsyms and use that to fill dst_reg.

Therefore, the proper functionality of pseudo_btf_id depends on (1) kallsyms and (2) the encoding of kernel global VARs in pahole, which should be available since pahole v1.18.

Signed-off-by: Hao Luo haoluo@google.com

include/linux/btf.h | 15 +++++++++ include/uapi/linux/bpf.h | 38 ++++++++++++++++------ kernel/bpf/btf.c | 15 --------- kernel/bpf/verifier.c | 68 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 24 deletions(-)

diff --git a/include/linux/btf.h b/include/linux/btf.h index 8b81fbb4497c..cee4089e83c0 100644 --- a/include/linux/btf.h +++ b/include/linux/btf.h @@ -107,6 +107,21 @@ static inline bool btf_type_is_func_proto(const struct btf_type *t) return BTF_INFO_KIND(t->info) == BTF_KIND_FUNC_PROTO; } +static inline bool btf_type_is_var(const struct btf_type *t) +{

• return BTF_INFO_KIND(t->info) == BTF_KIND_VAR;

+}

+/* union is only a special case of struct:

• • all its offsetof(member) == 0
• */

+static inline bool btf_type_is_struct(const struct btf_type *t) +{

• u8 kind = BTF_INFO_KIND(t->info);
• return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;

+}

• static inline u16 btf_type_vlen(const struct btf_type *t) { return BTF_INFO_VLEN(t->info);

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 0480f893facd..468376f2910b 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -346,18 +346,38 @@ enum bpf_link_type { #define BPF_F_TEST_STATE_FREQ (1U << 3) /* When BPF ldimm64's insn[0].src_reg != 0 then this can have

• • two extensions:
• • insn[0].src_reg: BPF_PSEUDO_MAP_FD BPF_PSEUDO_MAP_VALUE
• • insn[0].imm: map fd map fd
• • insn[1].imm: 0 offset into value
• • insn[0].off: 0 0
• • insn[1].off: 0 0
• • ldimm64 rewrite: address of map address of map[0]+offset
• • verifier type: CONST_PTR_TO_MAP PTR_TO_MAP_VALUE

• • the following extensions:
• • insn[0].src_reg: BPF_PSEUDO_MAP_FD
• • insn[0].imm: map fd
• • insn[1].imm: 0
• • insn[0].off: 0
• • insn[1].off: 0
• • ldimm64 rewrite: address of map
• • verifier type: CONST_PTR_TO_MAP
  */ #define BPF_PSEUDO_MAP_FD 1

+/*

• • insn[0].src_reg: BPF_PSEUDO_MAP_VALUE
• • insn[0].imm: map fd
• • insn[1].imm: offset into value
• • insn[0].off: 0
• • insn[1].off: 0
• • ldimm64 rewrite: address of map[0]+offset
• • verifier type: PTR_TO_MAP_VALUE
• */ #define BPF_PSEUDO_MAP_VALUE 2

+/*

• • insn[0].src_reg: BPF_PSEUDO_BTF_ID
• • insn[0].imm: kernel btd id of VAR
• • insn[1].imm: 0
• • insn[0].off: 0
• • insn[1].off: 0
• • ldimm64 rewrite: address of the kernel variable
• • verifier type: PTR_TO_BTF_ID or PTR_TO_MEM, depending on whether the var

• •               is struct/union.
    

• */

+#define BPF_PSEUDO_BTF_ID 3 /* when bpf_call->src_reg == BPF_PSEUDO_CALL, bpf_call->imm == pc-relative

• offset to another bpf function

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 91afdd4c82e3..b6d8f653afe2 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -353,16 +353,6 @@ static bool btf_type_nosize_or_null(const struct btf_type *t) return !t || btf_type_nosize(t); } -/* union is only a special case of struct:

• • all its offsetof(member) == 0
• */

-static bool btf_type_is_struct(const struct btf_type *t) -{

• u8 kind = BTF_INFO_KIND(t->info);
• return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;

-}

• static bool __btf_type_is_struct(const struct btf_type *t) { return BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT;

@@ -373,11 +363,6 @@ static bool btf_type_is_array(const struct btf_type *t) return BTF_INFO_KIND(t->info) == BTF_KIND_ARRAY; } -static bool btf_type_is_var(const struct btf_type *t) -{

• return BTF_INFO_KIND(t->info) == BTF_KIND_VAR;

-}

• static bool btf_type_is_datasec(const struct btf_type *t) { return BTF_INFO_KIND(t->info) == BTF_KIND_DATASEC;

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index ef938f17b944..47badde71f83 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -7205,6 +7205,68 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env, return 0; } +/* verify ld_imm64 insn of type PSEUDO_BTF_ID is valid */ +static inline int check_pseudo_btf_id(struct bpf_verifier_env *env,

• 		      struct bpf_insn *insn)
  

+{

• struct bpf_reg_state *regs = cur_regs(env);
• u32 type, id = insn->imm;
• u64 addr;
• const char *sym_name;
• const struct btf_type *t = btf_type_by_id(btf_vmlinux, id);

Since this is new code, please try to conform to reverse christmas tree coding style. For the last one, the assignment no need to be in declaration, you can put "t = ..." right before the first use of "t".

same for other places.

• if (!t) {

• verbose(env, "%s: invalid btf_id %d\n", __func__, id);
  

• return -ENOENT;
  

• }
• if (insn[1].imm != 0) {

• verbose(env, "%s: BPF_PSEUDO_BTF_ID uses reserved fields\n",
  

• 	__func__);
  

• return -EINVAL;
  

• }
• if (!btf_type_is_var(t)) {

• verbose(env, "%s: btf_id %d isn't KIND_VAR\n", __func__, id);
  

• return -EINVAL;
  

• }
• sym_name = btf_name_by_offset(btf_vmlinux, t->name_off);
• addr = kallsyms_lookup_name(sym_name);
• if (!addr) {

• verbose(env, "%s: failed to find the address of symbol '%s'.\n",
  

• 	__func__, sym_name);
  

• return -ENOENT;
  

• }
• insn[0].imm = (u32)addr;
• insn[1].imm = addr >> 32;
• mark_reg_known_zero(env, regs, insn->dst_reg);
• type = t->type;
• t = btf_type_skip_modifiers(btf_vmlinux, type, NULL);
• if (!btf_type_is_struct(t)) {

• u32 tsize;
  

• const struct btf_type *ret;
  

• const char *tname; > +
  

• /* resolve the type size of ksym. */
  

• ret = btf_resolve_size(btf_vmlinux, t, &tsize, NULL, NULL);
  

• if (IS_ERR(ret)) {
  

• 	tname = btf_name_by_offset(btf_vmlinux, t->name_off);
  

• 	verbose(env, "unable to resolve the size of type '%s': %ld\n",
  

• 		tname, PTR_ERR(ret));
  

• 	return -EINVAL;
  

• }
  

• regs[insn->dst_reg].type = PTR_TO_MEM;
  

• regs[insn->dst_reg].mem_size = tsize;
  

• } else {

• regs[insn->dst_reg].type = PTR_TO_BTF_ID;
  

• regs[insn->dst_reg].btf_id = type;
  

• }
• return 0;

+}

• /* verify BPF_LD_IMM64 instruction */

[...]

On 8/19/20 3:40 PM, Hao Luo wrote:

Propagate BPF_PSEUDO_BTF_ID from include/linux/uapi/bpf.h to tools/include/linux/uapi/bpf.h.

This can be folded into the previous patch.

Signed-off-by: Hao Luo haoluo@google.com

tools/include/uapi/linux/bpf.h | 38 ++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 0480f893facd..468376f2910b 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -346,18 +346,38 @@ enum bpf_link_type { #define BPF_F_TEST_STATE_FREQ (1U << 3) /* When BPF ldimm64's insn[0].src_reg != 0 then this can have

• • two extensions:
• • insn[0].src_reg: BPF_PSEUDO_MAP_FD BPF_PSEUDO_MAP_VALUE
• • insn[0].imm: map fd map fd
• • insn[1].imm: 0 offset into value
• • insn[0].off: 0 0
• • insn[1].off: 0 0
• • ldimm64 rewrite: address of map address of map[0]+offset
• • verifier type: CONST_PTR_TO_MAP PTR_TO_MAP_VALUE

• • the following extensions:
• • insn[0].src_reg: BPF_PSEUDO_MAP_FD
• • insn[0].imm: map fd
• • insn[1].imm: 0
• • insn[0].off: 0
• • insn[1].off: 0
• • ldimm64 rewrite: address of map
• • verifier type: CONST_PTR_TO_MAP
  */ #define BPF_PSEUDO_MAP_FD 1

+/*

• • insn[0].src_reg: BPF_PSEUDO_MAP_VALUE
• • insn[0].imm: map fd
• • insn[1].imm: offset into value
• • insn[0].off: 0
• • insn[1].off: 0
• • ldimm64 rewrite: address of map[0]+offset
• • verifier type: PTR_TO_MAP_VALUE
• */ #define BPF_PSEUDO_MAP_VALUE 2

+/*

• • insn[0].src_reg: BPF_PSEUDO_BTF_ID
• • insn[0].imm: kernel btd id of VAR
• • insn[1].imm: 0
• • insn[0].off: 0
• • insn[1].off: 0
• • ldimm64 rewrite: address of the kernel variable
• • verifier type: PTR_TO_BTF_ID or PTR_TO_MEM, depending on whether the var

• •               is struct/union.
    

• */

+#define BPF_PSEUDO_BTF_ID 3 /* when bpf_call->src_reg == BPF_PSEUDO_CALL, bpf_call->imm == pc-relative

• offset to another bpf function

On 8/19/20 3:40 PM, Hao Luo wrote:

For a ksym to be safely dereferenced and accessed, its type defined in bpf program should basically match its type defined in kernel. Implement a help function for a quick matching, which is used by libbpf when resolving the kernel btf_id of a ksym.

Signed-off-by: Hao Luo haoluo@google.com

tools/lib/bpf/btf.c | 171 ++++++++++++++++++++++++++++++++++++++++++++ tools/lib/bpf/btf.h | 2 + 2 files changed, 173 insertions(+)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c index a3d259e614b0..2ff31f244d7a 100644 --- a/tools/lib/bpf/btf.c +++ b/tools/lib/bpf/btf.c @@ -1005,6 +1005,177 @@ int btf__get_map_kv_tids(const struct btf *btf, const char *map_name, return 0; } +/*

• • Basic type check for ksym support. Only checks type kind and resolved size.
• */

+static inline +bool btf_ksym_equal_type(const struct btf *ba, __u32 type_a,

• 	 const struct btf *bb, __u32 type_b)
  

"ba" and "bb" is not descriptive. Maybe "btf_a" or "btf_b"? or even "btf1" or "btf2" since the number does not carry extra meaning compared to letters.

The same for below, may be t1, t2?

+{

• const struct btf_type *ta, *tb;
• ta = btf__type_by_id(ba, type_a);
• tb = btf__type_by_id(bb, type_b);
• /* compare type kind */
• if (btf_kind(ta) != btf_kind(tb))

• return false;
  

• /* compare resolved type size */
• return btf__resolve_size(ba, type_a) == btf__resolve_size(bb, type_b);

+}

+/*

• • Match a ksym's type defined in bpf programs against its type encoded in
• • kernel btf.
• */

+bool btf_ksym_type_match(const struct btf *ba, __u32 id_a,

• 	 const struct btf *bb, __u32 id_b)
  

+{

• const struct btf_type *ta = btf__type_by_id(ba, id_a);
• const struct btf_type *tb = btf__type_by_id(bb, id_b);
• int i;
• /* compare type kind */
• if (btf_kind(ta) != btf_kind(tb)) {

• pr_warn("%s:mismatched type kind (%d v.s. %d).\n",
  

• 	__func__, btf_kind(ta), btf_kind(tb));
  

• return false;
  

• }
• switch (btf_kind(ta)) {
• case BTF_KIND_INT: { /* compare size and encoding */

• __u32 ea, eb;
  

• if (ta->size != tb->size) {
  

• 	pr_warn("%s:INT size mismatch, (%u v.s. %u)\n",
  

• 		__func__, ta->size, tb->size);
  

• 	return false;
  

• }
  

• ea = *(__u32 *)(ta + 1);
  

• eb = *(__u32 *)(tb + 1);
  

• if (ea != eb) {
  

• 	pr_warn("%s:INT encoding mismatch (%u v.s. %u)\n",
  

• 		__func__, ea, eb);
  

• 	return false;
  

• }
  

• break;
  

• }
• case BTF_KIND_ARRAY: { /* compare type and number of elements */

• const struct btf_array *ea, *eb;
  

• ea = btf_array(ta);
  

• eb = btf_array(tb);
  

• if (!btf_ksym_equal_type(ba, ea->type, bb, eb->type)) {
  

• 	pr_warn("%s:ARRAY elem type mismatch.\n", __func__);
  

• 	return false;
  

• }
  

• if (ea->nelems != eb->nelems) {
  

• 	pr_warn("%s:ARRAY nelems mismatch (%d v.s. %d)\n",
  

• 		__func__, ea->nelems, eb->nelems);
  

• 	return false;
  

• }
  

• break;
  

• }
• case BTF_KIND_STRUCT:
• case BTF_KIND_UNION: { /* compare size, vlen and member offset, name */

• const struct btf_member *ma, *mb;
  

• if (ta->size != tb->size) {
  

• 	pr_warn("%s:STRUCT size mismatch, (%u v.s. %u)\n",
  

• 		__func__, ta->size, tb->size);
  

• 	return false;
  

• }
  

• if (btf_vlen(ta) != btf_vlen(tb)) {
  

• 	pr_warn("%s:STRUCT vlen mismatch, (%u v.s. %u)\n",
  

• 		__func__, btf_vlen(ta), btf_vlen(tb));
  

• 	return false;
  

• }
  

• ma = btf_members(ta);
  

• mb = btf_members(tb);
  

• for (i = 0; i < btf_vlen(ta); i++, ma++, mb++) {
  

• 	const char *na, *nb;
  

• 	if (ma->offset != mb->offset) {
  

• 		pr_warn("%s:STRUCT field offset mismatch, (%u v.s. %u)\n",
  

• 			__func__, ma->offset, mb->offset);
  

• 		return false;
  

• 	}
  

• 	na = btf__name_by_offset(ba, ma->name_off);
  

• 	nb = btf__name_by_offset(bb, mb->name_off);
  

• 	if (strcmp(na, nb)) {
  

• 		pr_warn("%s:STRUCT field name mismatch, (%s v.s. %s)\n",
  

• 			__func__, na, nb);
  

• 		return false;
  

• 	}
  

• }
  

I am wondering whether this is too strict and how this can co-work with CO-RE. Forcing users to write almost identical structure definition to the underlying kernel will not be user friendly and may not work cross kernel versions even if the field user cares have not changed.

Maybe we can relax the constraint here. You can look at existing libbpf CO-RE code.

• break;
  

• }
• case BTF_KIND_ENUM: { /* compare vlen and member value, name */

• const struct btf_enum *ma, *mb;
  

• if (btf_vlen(ta) != btf_vlen(tb)) {
  

• 	pr_warn("%s:ENUM vlen mismatch, (%u v.s. %u)\n",
  

• 		__func__, btf_vlen(ta), btf_vlen(tb));
  

• 	return false;
  

• }
  

• ma = btf_enum(ta);
  

• mb = btf_enum(tb);
  

• for (i = 0; i < btf_vlen(ta); i++, ma++, mb++) {
  

• 	if (ma->val != mb->val) {
  

• 		pr_warn("%s:ENUM val mismatch, (%u v.s. %u)\n",
  

• 			__func__, ma->val, mb->val);
  

• 		return false;
  

• 	}
  

• }
  

• break;
  

• }
• case BTF_KIND_PTR: { /* naive compare of ref type for PTR */

• if (!btf_ksym_equal_type(ba, ta->type, bb, tb->type)) {
  

• 	pr_warn("%s:PTR ref type mismatch.\n", __func__);
  

• 	return false;
  

• }
  

• break;
  

• }
• case BTF_KIND_FUNC_PROTO: { /* naive compare of vlen and param types */

• const struct btf_param *pa, *pb;
  

• if (btf_vlen(ta) != btf_vlen(tb)) {
  

• 	pr_warn("%s:FUNC_PROTO vlen mismatch, (%u v.s. %u)\n",
  

• 		__func__, btf_vlen(ta), btf_vlen(tb));
  

• 	return false;
  

• }
  

• pa = btf_params(ta);
  

• pb = btf_params(tb);
  

• for (i = 0; i < btf_vlen(ta); i++, pa++, pb++) {
  

• 	if (!btf_ksym_equal_type(ba, pa->type, bb, pb->type)) {
  

• 		pr_warn("%s:FUNC_PROTO params type mismatch.\n",
  

• 			__func__);
  

• 		return false;
  

• 	}
  

• }
  

• break;
  

• }
• case BTF_KIND_FUNC:
• case BTF_KIND_CONST:
• case BTF_KIND_VOLATILE:
• case BTF_KIND_RESTRICT:
• case BTF_KIND_TYPEDEF:
• case BTF_KIND_VAR:
• case BTF_KIND_DATASEC:

• pr_warn("unexpected type for matching ksym types.\n");
  

• return false;
  

• default:

• pr_warn("unsupported btf types.\n");
  

• return false;
  

• }
• return true;

+}

• struct btf_ext_sec_setup_param { __u32 off; __u32 len;

diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h index 91f0ad0e0325..5ef220e52485 100644 --- a/tools/lib/bpf/btf.h +++ b/tools/lib/bpf/btf.h @@ -52,6 +52,8 @@ LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name, __u32 expected_key_size, __u32 expected_value_size, __u32 *key_type_id, __u32 *value_type_id); +LIBBPF_API bool btf_ksym_type_match(const struct btf *ba, __u32 id_a,

• 		    const struct btf *bb, __u32 id_b);
  

LIBBPF_API struct btf_ext *btf_ext__new(__u8 *data, __u32 size); LIBBPF_API void btf_ext__free(struct btf_ext *btf_ext);

The new API function should be added to libbpf.map.

On Fri, Aug 21, 2020 at 2:50 PM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:

On Thu, Aug 20, 2020 at 10:22 AM Yonghong Song yhs@fb.com wrote:

...

On 8/19/20 3:40 PM, Hao Luo wrote:

...

For a ksym to be safely dereferenced and accessed, its type defined in bpf program should basically match its type defined in kernel. Implement a help function for a quick matching, which is used by libbpf when resolving the kernel btf_id of a ksym.

Signed-off-by: Hao Luo haoluo@google.com

[...]

...

...

+/*

• • Match a ksym's type defined in bpf programs against its type encoded in
• • kernel btf.
• */

+bool btf_ksym_type_match(const struct btf *ba, __u32 id_a,

•                  const struct btf *bb, __u32 id_b)
  

+{

[...]

...

...

•                 }
  

•         }
  

I am wondering whether this is too strict and how this can co-work with CO-RE. Forcing users to write almost identical structure definition to the underlying kernel will not be user friendly and may not work cross kernel versions even if the field user cares have not changed.

Maybe we can relax the constraint here. You can look at existing libbpf CO-RE code.

Right. Hao, can you just re-use bpf_core_types_are_compat() instead? See if semantics makes sense, but I think it should. BPF CO-RE has been permissive in terms of struct size and few other type aspects, because it handles relocations so well. This approach allows to not have to exactly match all possible variations of some struct definition, which is a big problem with ever-changing kernel data structures.

I have to say I hate myself writing another type comparison instead of reusing the existing one. The issue is that when bpf_core_types_compat compares names, it uses t1->name_off == t2->name_off. It is also used in bpf_equal_common(). In my case, because these types are from two different BTFs, their name_off are not expected to be the same, right? I didn't find a good solution to refactor before posting this patch. I think I can adapt bpf_core_type_compat() and pay more attention to CO-RE.

...

...

•         break;
  

• }
  

[...]

...

...

• struct btf_ext_sec_setup_param { __u32 off; __u32 len;

diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h index 91f0ad0e0325..5ef220e52485 100644 --- a/tools/lib/bpf/btf.h +++ b/tools/lib/bpf/btf.h @@ -52,6 +52,8 @@ LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name, __u32 expected_key_size, __u32 expected_value_size, __u32 *key_type_id, __u32 *value_type_id); +LIBBPF_API bool btf_ksym_type_match(const struct btf *ba, __u32 id_a,

•                             const struct btf *bb, __u32 id_b);
  
  

  LIBBPF_API struct btf_ext *btf_ext__new(__u8 *data, __u32 size); LIBBPF_API void btf_ext__free(struct btf_ext *btf_ext);

The new API function should be added to libbpf.map.

My question is why does this even have to be a public API?

I can fix. Please pardon my ignorance, what is the difference between public and internal APIs? I wasn't sure, so used it improperly.

Thanks, Hao

Ah, I see bpf_core_types_are_compat() after sync'ing my local repo. It seems the perfect fit for my use case. I only found the btf_equal_xxx() defined in btf.c when posting these patches. I can test and use bpf_core_types_are_compat() in v2. Thanks for pointing it out and explaining the public APIs.

Hao

On Fri, Aug 21, 2020 at 7:43 PM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:

On Fri, Aug 21, 2020 at 5:43 PM Hao Luo haoluo@google.com wrote:

...

On Fri, Aug 21, 2020 at 2:50 PM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:

...

On Thu, Aug 20, 2020 at 10:22 AM Yonghong Song yhs@fb.com wrote:

...

On 8/19/20 3:40 PM, Hao Luo wrote:

...

For a ksym to be safely dereferenced and accessed, its type defined in bpf program should basically match its type defined in kernel. Implement a help function for a quick matching, which is used by libbpf when resolving the kernel btf_id of a ksym.

Signed-off-by: Hao Luo haoluo@google.com

[...]

...

...

...

+/*

• • Match a ksym's type defined in bpf programs against its type encoded in
• • kernel btf.
• */

+bool btf_ksym_type_match(const struct btf *ba, __u32 id_a,

•                  const struct btf *bb, __u32 id_b)
  

+{

[...]

...

...

•                 }
  

•         }
  

I am wondering whether this is too strict and how this can co-work with CO-RE. Forcing users to write almost identical structure definition to the underlying kernel will not be user friendly and may not work cross kernel versions even if the field user cares have not changed.

Maybe we can relax the constraint here. You can look at existing libbpf CO-RE code.

Right. Hao, can you just re-use bpf_core_types_are_compat() instead? See if semantics makes sense, but I think it should. BPF CO-RE has been permissive in terms of struct size and few other type aspects, because it handles relocations so well. This approach allows to not have to exactly match all possible variations of some struct definition, which is a big problem with ever-changing kernel data structures.

I have to say I hate myself writing another type comparison instead of reusing the existing one. The issue is that when bpf_core_types_compat compares names, it uses t1->name_off == t2->name_off. It is also used

Huh? Are we talking about the same bpf_core_types_are_compat() (there is no bpf_core_types_compat, I think it's a typo)? bpf_core_types_are_compat() doesn't even compare any name, so I'm not sure what you are talking about. Some of btf_dedup functions do string comparisons using name_off directly, but that's a special and very careful case, it's not relevant here.

...

in bpf_equal_common(). In my case, because these types are from two different BTFs, their name_off are not expected to be the same, right? I didn't find a good solution to refactor before posting this patch. I

bpf_core_types_are_compat() didn't land until this week, so you must be confusing something. Please take another look.

...

think I can adapt bpf_core_type_compat() and pay more attention to CO-RE.

...

...

...

•         break;
  

• }
  

[...]

...

...

• struct btf_ext_sec_setup_param { __u32 off; __u32 len;

diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h index 91f0ad0e0325..5ef220e52485 100644 --- a/tools/lib/bpf/btf.h +++ b/tools/lib/bpf/btf.h @@ -52,6 +52,8 @@ LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name, __u32 expected_key_size, __u32 expected_value_size, __u32 *key_type_id, __u32 *value_type_id); +LIBBPF_API bool btf_ksym_type_match(const struct btf *ba, __u32 id_a,

•                             const struct btf *bb, __u32 id_b);
  
  

  LIBBPF_API struct btf_ext *btf_ext__new(__u8 *data, __u32 size); LIBBPF_API void btf_ext__free(struct btf_ext *btf_ext);

The new API function should be added to libbpf.map.

My question is why does this even have to be a public API?

I can fix. Please pardon my ignorance, what is the difference between public and internal APIs? I wasn't sure, so used it improperly.

public APIs are those that users of libbpf are supposed to use, internal one is just for libbpf internal use. The former can't change, the latter can be refactor as much as we need to.

...

Thanks, Hao

On Wed, Aug 19, 2020 at 3:42 PM Hao Luo haoluo@google.com wrote:

If a ksym is defined with a type, libbpf will try to find the ksym's btf information from kernel btf. If a valid btf entry for the ksym is found, libbpf can pass in the found btf id to the verifier, which validates the ksym's type and value.

Typeless ksyms (i.e. those defined as 'void') will not have such btf_id, but it has the symbol's address (read from kallsyms) and its value is treated as a raw pointer.

Signed-off-by: Hao Luo haoluo@google.com

tools/lib/bpf/libbpf.c | 130 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 114 insertions(+), 16 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 4a81c6b2d21b..94eff612c7c2 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -357,7 +357,16 @@ struct extern_desc { bool is_signed; } kcfg; struct {

•                   unsigned long long addr;
  

•                   /*
  

•                    *  1. If ksym is typeless, the field 'addr' is valid.
  

•                    *  2. If ksym is typed, the field 'vmlinux_btf_id' is
  

•                    *     valid.
  

•                    */
  

•                   bool is_typeless;
  

•                   union {
  

•                           unsigned long long addr;
  

•                           int vmlinux_btf_id;
  

•                   };
  

ksym is 16 bytes anyways, union doesn't help to save space. I propose to encode all this with just two fields: vmlinux_btf_id and addr. If btf_id == 0, then extern is typeless.

            } ksym;
    };

}; @@ -382,6 +391,7 @@ struct bpf_object {

    bool loaded;
    bool has_pseudo_calls;

•   bool has_typed_ksyms;
  
    /*
     * Information when doing elf related work. Only valid if fd
  

@@ -2521,6 +2531,10 @@ static int bpf_object__load_vmlinux_btf(struct bpf_object *obj) if (obj->btf_ext && obj->btf_ext->core_relo_info.len) need_vmlinux_btf = true;

•   /* Support for typed ksyms needs kernel BTF */
  

•   if (obj->has_typed_ksyms)
  

•           need_vmlinux_btf = true;
  

On the second read, I don't think you really need `has_typed_ksyms` at all. Just iterate over ksym externs and see if you have a typed one. It's the only place that cares.

•   bpf_object__for_each_program(prog, obj) {
            if (!prog->load)
                    continue;
  

@@ -2975,10 +2989,10 @@ static int bpf_object__collect_externs(struct bpf_object *obj) ext->type = EXT_KSYM;

                    vt = skip_mods_and_typedefs(obj->btf, t->type, NULL);

•                   if (!btf_is_void(vt)) {
  

•                           pr_warn("extern (ksym) '%s' is not typeless (void)\n", ext_name);
  

•                           return -ENOTSUP;
  

•                   }
  

•                   ext->ksym.is_typeless = btf_is_void(vt);
  

•                   if (!obj->has_typed_ksyms && !ext->ksym.is_typeless)
  

•                           obj->has_typed_ksyms = true;
  

nit: keep it simple:

if (ext->ksym.is_typeless) obj->has_typed_ksyms = true;

            } else {
                    pr_warn("unrecognized extern section '%s'\n", sec_name);
                    return -ENOTSUP;

@@ -2992,9 +3006,9 @@ static int bpf_object__collect_externs(struct bpf_object *obj) /* sort externs by type, for kcfg ones also by (align, size, name) */ qsort(obj->externs, obj->nr_extern, sizeof(*ext), cmp_externs);

•   /* for .ksyms section, we need to turn all externs into allocated
  

•    * variables in BTF to pass kernel verification; we do this by
  

•    * pretending that each extern is a 8-byte variable
  

•   /* for .ksyms section, we need to turn all typeless externs into
  

•    * allocated variables in BTF to pass kernel verification; we do
  

•    * this by pretending that each typeless extern is a 8-byte variable
     */
    if (ksym_sec) {
            /* find existing 4-byte integer type in BTF to use for fake
  

@@ -3012,7 +3026,7 @@ static int bpf_object__collect_externs(struct bpf_object *obj)

            sec = ksym_sec;
            n = btf_vlen(sec);

•           for (i = 0, off = 0; i < n; i++, off += sizeof(int)) {
  

•           for (i = 0, off = 0; i < n; i++) {
                    struct btf_var_secinfo *vs = btf_var_secinfos(sec) + i;
                    struct btf_type *vt;
  
  

@@ -3025,9 +3039,14 @@ static int bpf_object__collect_externs(struct bpf_object *obj) return -ESRCH; } btf_var(vt)->linkage = BTF_VAR_GLOBAL_ALLOCATED;

•                   vt->type = int_btf_id;
  

•                   if (ext->ksym.is_typeless) {
  

•                           vt->type = int_btf_id;
  

•                           vs->size = sizeof(int);
  

•                   }
                    vs->offset = off;
  

•                   vs->size = sizeof(int);
  

•                   off += vs->size;
  

•                   pr_debug("ksym var_secinfo: var '%s', type #%d, size %d, offset %d\n",
  

•                            ext->name, vt->type, vs->size, vs->offset);
  

It's a bit of a waste that we still allocate memory for those typed ksym externs, as they don't really need space. But modifying BTF is a pain right now, so I think we'll have to do it, until we have a better BTF API. But let's make them integers for now to take a fixed and small amount of space.

            }
            sec->size = off;
    }

@@ -5300,8 +5319,13 @@ bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj) insn[0].imm = obj->maps[obj->kconfig_map_idx].fd; insn[1].imm = ext->kcfg.data_off; } else /* EXT_KSYM */ {

•                           insn[0].imm = (__u32)ext->ksym.addr;
  

•                           insn[1].imm = ext->ksym.addr >> 32;
  

•                           if (ext->ksym.is_typeless) { /* typelss ksyms */
  

typo: typeless

•                                   insn[0].imm = (__u32)ext->ksym.addr;
  

•                                   insn[1].imm = ext->ksym.addr >> 32;
  

•                           } else { /* typed ksyms */
  

•                                   insn[0].src_reg = BPF_PSEUDO_BTF_ID;
  

•                                   insn[0].imm = ext->ksym.vmlinux_btf_id;
  

•                           }
                    }
                    break;
            case RELO_CALL:
  

@@ -6019,6 +6043,10 @@ static int bpf_object__read_kallsyms_file(struct bpf_object *obj) if (!ext || ext->type != EXT_KSYM) continue;

•           /* Typed ksyms have the verifier to fill their addresses. */
  

•           if (!ext->ksym.is_typeless)
  

•                   continue;
  

It might still be a good idea to try to find the symbol in kallsyms and emit nicer message if it's not there. Think about the user's experience when some global variable is removed from the kernel (or compiled out due to missing Kconfig). If libbpf can easily detect this, we should do it and provide a good error message.

•           if (ext->is_set && ext->ksym.addr != sym_addr) {
                    pr_warn("extern (ksym) '%s' resolution is ambiguous: 0x%llx or 0x%llx\n",
                            sym_name, ext->ksym.addr, sym_addr);
  

@@ -6037,10 +6065,72 @@ static int bpf_object__read_kallsyms_file(struct bpf_object *obj) return err; }

+static int bpf_object__resolve_ksyms_btf_id(struct bpf_object *obj) +{

•   struct extern_desc *ext;
  

•   int i, id;
  

•   if (!obj->btf_vmlinux) {
  

•           pr_warn("support of typed ksyms needs kernel btf.\n");
  

•           return -ENOENT;
  

•   }
  

•   for (i = 0; i < obj->nr_extern; i++) {
  

•           const struct btf_type *v, *vx; /* VARs in object and vmlinux btf */
  

•           const struct btf_type *t, *tx; /* TYPEs in btf */
  

•           __u32 vt, vtx; /* btf_ids of TYPEs */
  

I use targ_ and local_ prefixes with CO-RE to distinguish something that's coming from BPF object's BTF vs kernel's BTF, can you please adopt the same, as the process is essentially the same. "vx" vs "vtx" vs "v" are hard to distinguish and understand.

•           ext = &obj->externs[i];
  

•           if (ext->type != EXT_KSYM)
  

•                   continue;
  

•           if (ext->ksym.is_typeless)
  

•                   continue;
  

nit: combine into a single filter condition (we need typed ksym, that's one check)

•           if (ext->is_set) {
  

•                   pr_warn("typed ksym '%s' resolved as typeless ksyms.\n",
  

•                           ext->name);
  

•                   return -EFAULT;
  

this is a bug if that happens, that should be caught by test, please remove unnecessary check

•           }
  

•           id = btf__find_by_name_kind(obj->btf_vmlinux, ext->name,
  

•                                       BTF_KIND_VAR);
  

•           if (id <= 0) {
  

•                   pr_warn("no btf entry for ksym '%s' in vmlinux.\n",
  

•                           ext->name);
  

•                   return -ESRCH;
  

•           }
  

•           vx = btf__type_by_id(obj->btf_vmlinux, id);
  

•           tx = skip_mods_and_typedefs(obj->btf_vmlinux, vx->type, &vtx);
  

•           v = btf__type_by_id(obj->btf, ext->btf_id);
  

•           t = skip_mods_and_typedefs(obj->btf, v->type, &vt);
  

•           if (!btf_ksym_type_match(obj->btf_vmlinux, vtx, obj->btf, vt)) {
  

•                   const char *tname, *txname; /* names of TYPEs */
  

•                   txname = btf__name_by_offset(obj->btf_vmlinux, tx->name_off);
  

•                   tname = btf__name_by_offset(obj->btf, t->name_off);
  

•                   pr_warn("ksym '%s' expects type '%s' (vmlinux_btf_id: #%d), "
  

•                           "but got '%s' (btf_id: #%d)\n", ext->name,
  

•                           txname, vtx, tname, vt);
  

•                   return -EINVAL;
  

•           }
  

yeah, definitely just use bpf_core_types_are_compat() here. You'll want to skip_mods_and_typedefs first, but everything else should work for your use case.

•           ext->is_set = true;
  

•           ext->ksym.vmlinux_btf_id = id;
  

•           pr_debug("extern (ksym) %s=vmlinux_btf_id(#%d)\n", ext->name, id);
  

•   }
  

•   return 0;
  

+}

[...]

On Thu, Aug 27, 2020 at 3:29 PM Hao Luo haoluo@google.com wrote:

On Fri, Aug 21, 2020 at 3:37 PM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:

...

On Wed, Aug 19, 2020 at 3:42 PM Hao Luo haoluo@google.com wrote:

...

If a ksym is defined with a type, libbpf will try to find the ksym's btf information from kernel btf. If a valid btf entry for the ksym is found, libbpf can pass in the found btf id to the verifier, which validates the ksym's type and value.

Typeless ksyms (i.e. those defined as 'void') will not have such btf_id, but it has the symbol's address (read from kallsyms) and its value is treated as a raw pointer.

Signed-off-by: Hao Luo haoluo@google.com

tools/lib/bpf/libbpf.c | 130 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 114 insertions(+), 16 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 4a81c6b2d21b..94eff612c7c2 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -357,7 +357,16 @@ struct extern_desc { bool is_signed; } kcfg; struct {

•                   unsigned long long addr;
  

•                   /*
  

•                    *  1. If ksym is typeless, the field 'addr' is valid.
  

•                    *  2. If ksym is typed, the field 'vmlinux_btf_id' is
  

•                    *     valid.
  

•                    */
  

•                   bool is_typeless;
  

•                   union {
  

•                           unsigned long long addr;
  

•                           int vmlinux_btf_id;
  

•                   };
  

ksym is 16 bytes anyways, union doesn't help to save space. I propose to encode all this with just two fields: vmlinux_btf_id and addr. If btf_id == 0, then extern is typeless.

Ack on expanding the union. But I slightly preferred keeping is_typeless. IIUC, btf_id points a VAR_KIND, we need the following pointer chasing every time

t = btf__type_by_id(obj->btf, ext->btf_id); t->type;

which I felt is worse than keeping a is_typeless flag.

Sorry, I'm not following. In all places where you would check sym->is_typeless, you'd now just do:

if (ext->ksym.vmlinux_btf_id) { /* typed, use ext->ksym.vmlinux_btf_id */ } else { /* typeless */ }

...

...

            } ksym;
    };

}; @@ -382,6 +391,7 @@ struct bpf_object {

    bool loaded;
    bool has_pseudo_calls;

•   bool has_typed_ksyms;
  
    /*
     * Information when doing elf related work. Only valid if fd
  

@@ -2521,6 +2531,10 @@ static int bpf_object__load_vmlinux_btf(struct bpf_object *obj) if (obj->btf_ext && obj->btf_ext->core_relo_info.len) need_vmlinux_btf = true;

•   /* Support for typed ksyms needs kernel BTF */
  

•   if (obj->has_typed_ksyms)
  

•           need_vmlinux_btf = true;
  

On the second read, I don't think you really need `has_typed_ksyms` at all. Just iterate over ksym externs and see if you have a typed one. It's the only place that cares.

Ack. Will iterate over ksym externs here.

...

...

•   bpf_object__for_each_program(prog, obj) {
            if (!prog->load)
                    continue;
  

@@ -2975,10 +2989,10 @@ static int bpf_object__collect_externs(struct bpf_object *obj) ext->type = EXT_KSYM;

                    vt = skip_mods_and_typedefs(obj->btf, t->type, NULL);

•                   if (!btf_is_void(vt)) {
  

•                           pr_warn("extern (ksym) '%s' is not typeless (void)\n", ext_name);
  

•                           return -ENOTSUP;
  

•                   }
  

•                   ext->ksym.is_typeless = btf_is_void(vt);
  

•                   if (!obj->has_typed_ksyms && !ext->ksym.is_typeless)
  

•                           obj->has_typed_ksyms = true;
  

nit: keep it simple:

if (ext->ksym.is_typeless) obj->has_typed_ksyms = true;

Ack.

...

...

            } else {
                    pr_warn("unrecognized extern section '%s'\n", sec_name);
                    return -ENOTSUP;

@@ -2992,9 +3006,9 @@ static int bpf_object__collect_externs(struct bpf_object *obj) /* sort externs by type, for kcfg ones also by (align, size, name) */ qsort(obj->externs, obj->nr_extern, sizeof(*ext), cmp_externs);

•   /* for .ksyms section, we need to turn all externs into allocated
  

•    * variables in BTF to pass kernel verification; we do this by
  

•    * pretending that each extern is a 8-byte variable
  

•   /* for .ksyms section, we need to turn all typeless externs into
  

•    * allocated variables in BTF to pass kernel verification; we do
  

•    * this by pretending that each typeless extern is a 8-byte variable
     */
    if (ksym_sec) {
            /* find existing 4-byte integer type in BTF to use for fake
  

@@ -3012,7 +3026,7 @@ static int bpf_object__collect_externs(struct bpf_object *obj)

            sec = ksym_sec;
            n = btf_vlen(sec);

•           for (i = 0, off = 0; i < n; i++, off += sizeof(int)) {
  

•           for (i = 0, off = 0; i < n; i++) {
                    struct btf_var_secinfo *vs = btf_var_secinfos(sec) + i;
                    struct btf_type *vt;
  
  

@@ -3025,9 +3039,14 @@ static int bpf_object__collect_externs(struct bpf_object *obj) return -ESRCH; } btf_var(vt)->linkage = BTF_VAR_GLOBAL_ALLOCATED;

•                   vt->type = int_btf_id;
  

•                   if (ext->ksym.is_typeless) {
  

•                           vt->type = int_btf_id;
  

•                           vs->size = sizeof(int);
  

•                   }
                    vs->offset = off;
  

•                   vs->size = sizeof(int);
  

•                   off += vs->size;
  

•                   pr_debug("ksym var_secinfo: var '%s', type #%d, size %d, offset %d\n",
  

•                            ext->name, vt->type, vs->size, vs->offset);
  

It's a bit of a waste that we still allocate memory for those typed ksym externs, as they don't really need space. But modifying BTF is a pain right now, so I think we'll have to do it, until we have a better BTF API. But let's make them integers for now to take a fixed and small amount of space.

Do you mean making typed ksym externs of type integer? If so, we can't do that, I think. After collect_externs, we later need to compare the declared extern's type against the type defined in kernel. Better not rewrite their types in BTf.

Then maybe we need to make btf_id to point to the actual type of the variable, not BTF_KIND_VAR? Or just additionally record type's btf_id, not sure which one makes more sense at the moment.

I am generally against modifying BTF. I initially didn't notice that all the ksym externs' types are modified to 'int' and the type comparison I mentioned above always failed. I dumped the btf in vmlinux and the btf in object file, checked the kernel variable's source code, printed out everything I could. The experience was very bad.

It might be confusing, I agree, but the alternative is just a waste of memory just to match the BTF definition of a DATASEC, which describes externs. It seems sloppy to allocate a bunch of unused memory just to match the kernel's variable size, while in reality we either use 8 bytes used (for typeless externs, storing ksym address) or none (for typed externs).

Another alternative is to not specify BTF ID for .ksyms map, but it's not great for typeless externs case, as we are losing all type info completely. Trade-offs...

[...]

...

...

•           }
  

•           id = btf__find_by_name_kind(obj->btf_vmlinux, ext->name,
  

•                                       BTF_KIND_VAR);
  

•           if (id <= 0) {
  

•                   pr_warn("no btf entry for ksym '%s' in vmlinux.\n",
  

•                           ext->name);
  

•                   return -ESRCH;
  

•           }
  

•           vx = btf__type_by_id(obj->btf_vmlinux, id);
  

•           tx = skip_mods_and_typedefs(obj->btf_vmlinux, vx->type, &vtx);
  

•           v = btf__type_by_id(obj->btf, ext->btf_id);
  

•           t = skip_mods_and_typedefs(obj->btf, v->type, &vt);
  

•           if (!btf_ksym_type_match(obj->btf_vmlinux, vtx, obj->btf, vt)) {
  

•                   const char *tname, *txname; /* names of TYPEs */
  

•                   txname = btf__name_by_offset(obj->btf_vmlinux, tx->name_off);
  

•                   tname = btf__name_by_offset(obj->btf, t->name_off);
  

•                   pr_warn("ksym '%s' expects type '%s' (vmlinux_btf_id: #%d), "
  

•                           "but got '%s' (btf_id: #%d)\n", ext->name,
  

•                           txname, vtx, tname, vt);
  

•                   return -EINVAL;
  

•           }
  

yeah, definitely just use bpf_core_types_are_compat() here. You'll want to skip_mods_and_typedefs first, but everything else should work for your use case.

Ack. bpf_core_types_are_compat() is indeed a perfect fit here.

ok, great

[...]

On Tue, Sep 1, 2020 at 1:35 PM Hao Luo haoluo@google.com wrote:

On Tue, Sep 1, 2020 at 11:11 AM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:

...

On Thu, Aug 27, 2020 at 3:29 PM Hao Luo haoluo@google.com wrote:

...

On Fri, Aug 21, 2020 at 3:37 PM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:

...

On Wed, Aug 19, 2020 at 3:42 PM Hao Luo haoluo@google.com wrote:

...

If a ksym is defined with a type, libbpf will try to find the ksym's btf information from kernel btf. If a valid btf entry for the ksym is found, libbpf can pass in the found btf id to the verifier, which validates the ksym's type and value.

Typeless ksyms (i.e. those defined as 'void') will not have such btf_id, but it has the symbol's address (read from kallsyms) and its value is treated as a raw pointer.

Signed-off-by: Hao Luo haoluo@google.com

tools/lib/bpf/libbpf.c | 130 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 114 insertions(+), 16 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 4a81c6b2d21b..94eff612c7c2 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -357,7 +357,16 @@ struct extern_desc { bool is_signed; } kcfg; struct {

•                   unsigned long long addr;
  

•                   /*
  

•                    *  1. If ksym is typeless, the field 'addr' is valid.
  

•                    *  2. If ksym is typed, the field 'vmlinux_btf_id' is
  

•                    *     valid.
  

•                    */
  

•                   bool is_typeless;
  

•                   union {
  

•                           unsigned long long addr;
  

•                           int vmlinux_btf_id;
  

•                   };
  

ksym is 16 bytes anyways, union doesn't help to save space. I propose to encode all this with just two fields: vmlinux_btf_id and addr. If btf_id == 0, then extern is typeless.

Ack on expanding the union. But I slightly preferred keeping is_typeless. IIUC, btf_id points a VAR_KIND, we need the following pointer chasing every time

t = btf__type_by_id(obj->btf, ext->btf_id); t->type;

which I felt is worse than keeping a is_typeless flag.

Sorry, I'm not following. In all places where you would check sym->is_typeless, you'd now just do:

if (ext->ksym.vmlinux_btf_id) { /* typed, use ext->ksym.vmlinux_btf_id */ } else { /* typeless */ }

My apologies, I should be more specific.

'vmlinux_btf_id' gets its value in bpf_object__resolve_ksyms_btf_id(). Before we call this function, there are three places that need to tell whether a ksym is typed, currently in v1. Specifically,

• in bpf_object__collect_externs(), typeless ksyms are rewritten as

'int', in contrast, typed ones are left untouched (though this may change in v2).

• bpf_object__load_vmlinux_btf() now is called before

bpf_object__resolve_ksyms_btf_id(). In v1's design, if there is no typed ksym, we could skip loading vmlinux_btf potentially.

• even bpf_object__resolve_ksyms_btf_id() itself is conditionally

called, depending on whether there is any typed ksym.

At the time when these places are called, vmlinux_btf_id is unavailable and we can't use it for the purpose of telling whether a ksym is typed.

However, rather than vmlinux_btf_id, there may be an alternative. We can record the ksym extern's type's btf_id and use that as 'is_typeless' flag. This also solves the problem below.

Oh, I was thinking that vmlinux_btf_id contains a local BTF ID this whole time (clearly ignoring the "vmlinux_" part).

[...]

...

...

...

...

            } else {
                    pr_warn("unrecognized extern section '%s'\n", sec_name);
                    return -ENOTSUP;

@@ -2992,9 +3006,9 @@ static int bpf_object__collect_externs(struct bpf_object *obj) /* sort externs by type, for kcfg ones also by (align, size, name) */ qsort(obj->externs, obj->nr_extern, sizeof(*ext), cmp_externs);

•   /* for .ksyms section, we need to turn all externs into allocated
  

•    * variables in BTF to pass kernel verification; we do this by
  

•    * pretending that each extern is a 8-byte variable
  

•   /* for .ksyms section, we need to turn all typeless externs into
  

•    * allocated variables in BTF to pass kernel verification; we do
  

•    * this by pretending that each typeless extern is a 8-byte variable
     */
    if (ksym_sec) {
            /* find existing 4-byte integer type in BTF to use for fake
  

@@ -3012,7 +3026,7 @@ static int bpf_object__collect_externs(struct bpf_object *obj)

            sec = ksym_sec;
            n = btf_vlen(sec);

•           for (i = 0, off = 0; i < n; i++, off += sizeof(int)) {
  

•           for (i = 0, off = 0; i < n; i++) {
                    struct btf_var_secinfo *vs = btf_var_secinfos(sec) + i;
                    struct btf_type *vt;
  
  

@@ -3025,9 +3039,14 @@ static int bpf_object__collect_externs(struct bpf_object *obj) return -ESRCH; } btf_var(vt)->linkage = BTF_VAR_GLOBAL_ALLOCATED;

•                   vt->type = int_btf_id;
  

•                   if (ext->ksym.is_typeless) {
  

•                           vt->type = int_btf_id;
  

•                           vs->size = sizeof(int);
  

•                   }
                    vs->offset = off;
  

•                   vs->size = sizeof(int);
  

•                   off += vs->size;
  

•                   pr_debug("ksym var_secinfo: var '%s', type #%d, size %d, offset %d\n",
  

•                            ext->name, vt->type, vs->size, vs->offset);
  

It's a bit of a waste that we still allocate memory for those typed ksym externs, as they don't really need space. But modifying BTF is a pain right now, so I think we'll have to do it, until we have a better BTF API. But let's make them integers for now to take a fixed and small amount of space.

Do you mean making typed ksym externs of type integer? If so, we can't do that, I think. After collect_externs, we later need to compare the declared extern's type against the type defined in kernel. Better not rewrite their types in BTf.

Then maybe we need to make btf_id to point to the actual type of the variable, not BTF_KIND_VAR? Or just additionally record type's btf_id, not sure which one makes more sense at the moment.

...

I am generally against modifying BTF. I initially didn't notice that all the ksym externs' types are modified to 'int' and the type comparison I mentioned above always failed. I dumped the btf in vmlinux and the btf in object file, checked the kernel variable's source code, printed out everything I could. The experience was very bad.

It might be confusing, I agree, but the alternative is just a waste of memory just to match the BTF definition of a DATASEC, which describes externs. It seems sloppy to allocate a bunch of unused memory just to match the kernel's variable size, while in reality we either use 8 bytes used (for typeless externs, storing ksym address) or none (for typed externs).

Another alternative is to not specify BTF ID for .ksyms map, but it's not great for typeless externs case, as we are losing all type info completely. Trade-offs...

I see. It looks like rewriting all ksym externs' type to integers is the most straightforward solution here, though I felt a bit hacky.

I can record the btf_id of the var's type before rewriting, so bpf_core_type_are_compat() can find the true type for comparison. One good thing about recording the type's btf_id is that it can be used to tell whether the ksym extern is typed or not, before vmlinux_btf_id

that's what I've been getting at, but I missed that vmlinux_btf_id is kernel BTF type ID. So let's record both local and target BTF type IDs and use local_btf_id as an indicator of typed vs typeless?

gets its value. I will think about this and try the alternatives a bit more and follow up if I come up with a better solution.

Thanks!

[...]

On Thu, Aug 20, 2020 at 10:32 AM Yonghong Song yhs@fb.com wrote:

On 8/19/20 3:40 PM, Hao Luo wrote:

...

Selftests for typed ksyms. Tests two types of ksyms: one is a struct, the other is a plain int. This tests two paths in the kernel. Struct ksyms will be converted into PTR_TO_BTF_ID by the verifier while int typed ksyms will be converted into PTR_TO_MEM.

Signed-off-by: Hao Luo haoluo@google.com

.../selftests/bpf/prog_tests/ksyms_btf.c | 77 +++++++++++++++++++ .../selftests/bpf/progs/test_ksyms_btf.c | 23 ++++++ 2 files changed, 100 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/ksyms_btf.c create mode 100644 tools/testing/selftests/bpf/progs/test_ksyms_btf.c

diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c new file mode 100644 index 000000000000..1dad61ba7e99 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2020 Google */

+#include <test_progs.h> +#include <bpf/libbpf.h> +#include <bpf/btf.h> +#include "test_ksyms_btf.skel.h"

+static int duration;

+static __u64 kallsyms_find(const char *sym) +{

• char type, name[500];
  

• __u64 addr, res = 0;
  

• FILE *f;
  

• f = fopen("/proc/kallsyms", "r");
  

• if (CHECK(!f, "kallsyms_fopen", "failed to open: %d\n", errno))
  

•         return 0;
  

could you check whether libbpf API can provide this functionality for you? As far as I know, libbpf does parse /proc/kallsyms.

No need to use libbpf's implementation. We already have kallsyms_find() in prog_tests/ksyms.c and a combination of load_kallsyms() + ksym_get_addr() in trace_helpers.c. It would be good to switch to one implementation for both prog_tests/ksyms.c and this one.

...

• while (fscanf(f, "%llx %c %499s%*[^\n]\n", &addr, &type, name) > 0) {
  

•         if (strcmp(name, sym) == 0) {
  

•                 res = addr;
  

•                 goto out;
  

•         }
  

• }
  

[...]

...

diff --git a/tools/testing/selftests/bpf/progs/test_ksyms_btf.c b/tools/testing/selftests/bpf/progs/test_ksyms_btf.c new file mode 100644 index 000000000000..e04e31117f84 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_ksyms_btf.c @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2020 Google */

+#include "vmlinux.h"

+#include <bpf/bpf_helpers.h>

+__u64 out__runqueues = -1; +__u64 out__bpf_prog_active = -1;

+extern const struct rq runqueues __ksym; /* struct type global var. */ +extern const int bpf_prog_active __ksym; /* int type global var. */

+SEC("raw_tp/sys_enter") +int handler(const void *ctx) +{

• out__runqueues = (__u64)&runqueues;
  

• out__bpf_prog_active = (__u64)&bpf_prog_active;
  

You didn't test accessing any of the members of runqueues, because BTF only has per-CPU variables, right? Adding global/static variables was adding too much data to BTF or something like that, is that right?

...

• return 0;
  

+}

+char _license[] SEC("license") = "GPL";

On Sat, Aug 22, 2020 at 12:27 AM Hao Luo haoluo@google.com wrote:

On Fri, Aug 21, 2020 at 4:03 PM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:

...

On Thu, Aug 20, 2020 at 10:32 AM Yonghong Song yhs@fb.com wrote:

...

On 8/19/20 3:40 PM, Hao Luo wrote:

...

Selftests for typed ksyms. Tests two types of ksyms: one is a struct, the other is a plain int. This tests two paths in the kernel. Struct ksyms will be converted into PTR_TO_BTF_ID by the verifier while int typed ksyms will be converted into PTR_TO_MEM.

Signed-off-by: Hao Luo haoluo@google.com

.../selftests/bpf/prog_tests/ksyms_btf.c | 77 +++++++++++++++++++ .../selftests/bpf/progs/test_ksyms_btf.c | 23 ++++++ 2 files changed, 100 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/ksyms_btf.c create mode 100644 tools/testing/selftests/bpf/progs/test_ksyms_btf.c

diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c new file mode 100644 index 000000000000..1dad61ba7e99 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2020 Google */

+#include <test_progs.h> +#include <bpf/libbpf.h> +#include <bpf/btf.h> +#include "test_ksyms_btf.skel.h"

+static int duration;

+static __u64 kallsyms_find(const char *sym) +{

• char type, name[500];
  

• __u64 addr, res = 0;
  

• FILE *f;
  

• f = fopen("/proc/kallsyms", "r");
  

• if (CHECK(!f, "kallsyms_fopen", "failed to open: %d\n", errno))
  

•         return 0;
  

could you check whether libbpf API can provide this functionality for you? As far as I know, libbpf does parse /proc/kallsyms.

No need to use libbpf's implementation. We already have kallsyms_find() in prog_tests/ksyms.c and a combination of load_kallsyms() + ksym_get_addr() in trace_helpers.c. It would be good to switch to one implementation for both prog_tests/ksyms.c and this one.

Ack. I can do some refactoring. The least thing that I can do is moving kallsyms_find() to a header for both prog_tests/ksyms.c and this test to use.

Please no extra headers. Just put kallsyms_find() in trace_helpers.c, along other kallsyms-related functions.

...

...

...

diff --git a/tools/testing/selftests/bpf/progs/test_ksyms_btf.c b/tools/testing/selftests/bpf/progs/test_ksyms_btf.c new file mode 100644 index 000000000000..e04e31117f84 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_ksyms_btf.c @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2020 Google */

+#include "vmlinux.h"

+#include <bpf/bpf_helpers.h>

+__u64 out__runqueues = -1; +__u64 out__bpf_prog_active = -1;

+extern const struct rq runqueues __ksym; /* struct type global var. */ +extern const int bpf_prog_active __ksym; /* int type global var. */

+SEC("raw_tp/sys_enter") +int handler(const void *ctx) +{

• out__runqueues = (__u64)&runqueues;
  

• out__bpf_prog_active = (__u64)&bpf_prog_active;
  

You didn't test accessing any of the members of runqueues, because BTF only has per-CPU variables, right? Adding global/static variables was adding too much data to BTF or something like that, is that right?

Right. With some experiments, I found the address of a percpu variable doesn't necessarily point to a valid structure. So it doesn't make sense to dereference runqueues and access its members. However, right now there are only percpu variables encoded in BTF, so I can't test accessing members of general global/static variables unfortunately.

Hao

On Wed, Aug 19, 2020 at 3:42 PM Hao Luo haoluo@google.com wrote:

Add bpf_per_cpu_ptr() to help bpf programs access percpu vars. bpf_per_cpu_ptr() has the same semantic as per_cpu_ptr() in the kernel except that it may return NULL. This happens when the cpu parameter is out of range. So the caller must check the returned value.

Signed-off-by: Hao Luo haoluo@google.com

The logic looks correct, few naming nits, but otherwise:

Acked-by: Andrii Nakryiko andriin@fb.com

include/linux/bpf.h | 3 ++ include/linux/btf.h | 11 +++++++ include/uapi/linux/bpf.h | 14 +++++++++ kernel/bpf/btf.c | 10 ------- kernel/bpf/verifier.c | 64 ++++++++++++++++++++++++++++++++++++++-- kernel/trace/bpf_trace.c | 18 +++++++++++ 6 files changed, 107 insertions(+), 13 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 55f694b63164..613404beab33 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -268,6 +268,7 @@ enum bpf_arg_type { ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */ ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */ ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */

•   ARG_PTR_TO_PERCPU_BTF_ID,       /* pointer to in-kernel percpu type */
  

};

/* type of values returned from helper functions */ @@ -281,6 +282,7 @@ enum bpf_return_type { RET_PTR_TO_SOCK_COMMON_OR_NULL, /* returns a pointer to a sock_common or NULL */ RET_PTR_TO_ALLOC_MEM_OR_NULL, /* returns a pointer to dynamically allocated memory or NULL */ RET_PTR_TO_BTF_ID_OR_NULL, /* returns a pointer to a btf_id or NULL */

•   RET_PTR_TO_MEM_OR_BTF_OR_NULL,  /* returns a pointer to a valid memory or a btf_id or NULL */
  

I know it's already very long, but still let's use BTF_ID consistently

};

/* eBPF function prototype used by verifier to allow BPF_CALLs from eBPF programs @@ -360,6 +362,7 @@ enum bpf_reg_type { PTR_TO_RDONLY_BUF_OR_NULL, /* reg points to a readonly buffer or NULL */ PTR_TO_RDWR_BUF, /* reg points to a read/write buffer */ PTR_TO_RDWR_BUF_OR_NULL, /* reg points to a read/write buffer or NULL */

•   PTR_TO_PERCPU_BTF_ID,    /* reg points to percpu kernel type */
  

};

[...]

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 468376f2910b..c7e49a102ed2 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -3415,6 +3415,19 @@ union bpf_attr {

•         A non-negative value equal to or less than *size* on success,
  

•         or a negative error in case of failure.
  

• • void *bpf_per_cpu_ptr(const void *ptr, u32 cpu)

• • Description
    

• •         Take the address of a percpu ksym and return a pointer pointing
    

• •         to the variable on *cpu*. A ksym is an extern variable decorated
    

• •         with '__ksym'. A ksym is percpu if there is a global percpu var
    

• •         (either static or global) defined of the same name in the kernel.
    

The function signature has "ptr", not "ksym", but the description is using "ksym". please make them consistent (might name param "percpu_ptr")

• •         bpf_per_cpu_ptr() has the same semantic as per_cpu_ptr() in the
    

• •         kernel, except that bpf_per_cpu_ptr() may return NULL. This
    

• •         happens if *cpu* is larger than nr_cpu_ids. The caller of
    

• •         bpf_per_cpu_ptr() must check the returned value.
    

• • Return
    

• •         A generic pointer pointing to the variable on *cpu*.
    

  */

[...]

•   } else if (arg_type == ARG_PTR_TO_PERCPU_BTF_ID) {
  

•           expected_type = PTR_TO_PERCPU_BTF_ID;
  

•           if (type != expected_type)
  

•                   goto err_type;
  

•           if (!reg->btf_id) {
  

•                   verbose(env, "Helper has zero btf_id in R%d\n", regno);
  

nit: "invalid btf_id"?

•                   return -EACCES;
  

•           }
  

•           meta->ret_btf_id = reg->btf_id;
    } else if (arg_type == ARG_PTR_TO_BTF_ID) {
            expected_type = PTR_TO_BTF_ID;
            if (type != expected_type)
  

@@ -4904,6 +4918,30 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL; regs[BPF_REG_0].id = ++env->id_gen; regs[BPF_REG_0].mem_size = meta.mem_size;

[...]

On 8/19/20 3:40 PM, Hao Luo wrote:

Sync tools/include/linux/uapi/bpf.h with include/linux/uapi/bpf.h

This can be folded into the previous patch.

Signed-off-by: Hao Luo haoluo@google.com

tools/include/uapi/linux/bpf.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 468376f2910b..7e3dfb2bbb86 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -3415,6 +3415,20 @@ union bpf_attr {

• A non-negative value equal to or less than *size* on success,
  

• or a negative error in case of failure.
  

• • void *bpf_per_cpu_ptr(const void *ptr, u32 cpu)
• • Description

• • Take the address of a percpu ksym and return a pointer pointing
    

• • to the variable on *cpu*. A ksym is an extern variable decorated
    

• • with '__ksym'. A ksym is percpu if there is a global percpu var
    

• • (either static or global) defined of the same name in the kernel.
    

• • bpf_per_cpu_ptr() has the same semantic as per_cpu_ptr() in the
    

• • kernel, except that bpf_per_cpu_ptr() may return NULL. This
    

• • happens if *cpu* is larger than nr_cpu_ids. The caller of
    

• • bpf_per_cpu_ptr() must check the returned value.
    

• • Return

• • A generic pointer pointing to the variable on *cpu*.
    

• */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \

@@ -3559,6 +3573,7 @@ union bpf_attr { FN(skc_to_tcp_request_sock), \ FN(skc_to_udp6_sock), \ FN(get_task_stack), \

• FN(bpf_per_cpu_ptr), \ /* */

/* integer value in 'imm' field of BPF_CALL instruction selects which helper

On Wed, Aug 19, 2020 at 3:42 PM Hao Luo haoluo@google.com wrote:

Test bpf_per_cpu_ptr(). Test two paths in the kernel. If the base pointer points to a struct, the returned reg is of type PTR_TO_BTF_ID. Direct pointer dereference can be applied on the returned variable. If the base pointer isn't a struct, the returned reg is of type PTR_TO_MEM, which also supports direct pointer dereference.

Signed-off-by: Hao Luo haoluo@google.com

Acked-by: Andrii Nakryiko andriin@fb.com

.../testing/selftests/bpf/prog_tests/ksyms_btf.c | 4 ++++ .../testing/selftests/bpf/progs/test_ksyms_btf.c | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c index 1dad61ba7e99..bdedd4a76b42 100644 --- a/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c +++ b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c @@ -71,6 +71,10 @@ void test_ksyms_btf(void) "got %llu, exp %llu\n", data->out__runqueues, runqueues_addr); CHECK(data->out__bpf_prog_active != bpf_prog_active_addr, "bpf_prog_active", "got %llu, exp %llu\n", data->out__bpf_prog_active, bpf_prog_active_addr);

•   CHECK(data->out__rq_cpu != 1, "rq_cpu",
  

•         "got %u, exp %u\n", data->out__rq_cpu, 1);
  

•   CHECK(data->out__process_counts == -1, "process_counts",
  

•         "got %lu, exp != -1", data->out__process_counts);
  
  

cleanup: test_ksyms_btf__destroy(skel); diff --git a/tools/testing/selftests/bpf/progs/test_ksyms_btf.c b/tools/testing/selftests/bpf/progs/test_ksyms_btf.c index e04e31117f84..78cf1ebb753d 100644 --- a/tools/testing/selftests/bpf/progs/test_ksyms_btf.c +++ b/tools/testing/selftests/bpf/progs/test_ksyms_btf.c @@ -7,16 +7,29 @@

__u64 out__runqueues = -1; __u64 out__bpf_prog_active = -1; +__u32 out__rq_cpu = -1; +unsigned long out__process_counts = -1;

try to not use long for variables, it is 32-bit integer in user-space but always 64-bit in BPF. This causes problems when using skeleton on 32-bit architecture.

-extern const struct rq runqueues __ksym; /* struct type global var. */ +extern const struct rq runqueues __ksym; /* struct type percpu var. */ extern const int bpf_prog_active __ksym; /* int type global var. */ +extern const unsigned long process_counts __ksym; /* int type percpu var. */

SEC("raw_tp/sys_enter") int handler(const void *ctx) {

•   struct rq *rq;
  

•   unsigned long *count;
  

•   out__runqueues = (__u64)&runqueues;
    out__bpf_prog_active = (__u64)&bpf_prog_active;
  
  

•   rq = (struct rq *)bpf_per_cpu_ptr(&runqueues, 1);
  

•   if (rq)
  

•           out__rq_cpu = rq->cpu;
  

this is awesome!

Are there any per-cpu variables that are arrays? Would be nice to test those too.

•   count = (unsigned long *)bpf_per_cpu_ptr(&process_counts, 1);
  

•   if (count)
  

•           out__process_counts = *count;
  

•   return 0;
  

}

-- 2.28.0.220.ged08abb693-goog