[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <35519fec-754c-0a17-4f01-9d6e39a8a7e8@fb.com>
Date: Thu, 20 Aug 2020 08:22:29 -0700
From: Yonghong Song <yhs@...com>
To: Hao Luo <haoluo@...gle.com>, <netdev@...r.kernel.org>,
<bpf@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-kselftest@...r.kernel.org>
CC: Shuah Khan <shuah@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
Andrii Nakryiko <andriin@...com>,
Daniel Borkmann <daniel@...earbox.net>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...omium.org>,
Quentin Monnet <quentin@...valent.com>,
Steven Rostedt <rostedt@...dmis.org>,
Ingo Molnar <mingo@...hat.com>, Andrey Ignatov <rdna@...com>,
Jakub Sitnicki <jakub@...udflare.com>
Subject: Re: [PATCH bpf-next v1 1/8] bpf: Introduce pseudo_btf_id
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@...gle.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(-)
>
[...]
Powered by blists - more mailing lists