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 <[email protected]>
> ---
> 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(-)
>
[...]
Yonghong,
Thank you for taking a look. Explicitly cc'ing Arnaldo to see if he
has any immediate insights. In the meantime, I'll dedicate time to
investigate this issue you found.
Thanks,
Hao
On Thu, Aug 20, 2020 at 8:23 AM Yonghong Song <[email protected]> wrote:
>
>
>
> 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 <[email protected]>
> > ---
> > 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(-)
> >
> [...]
Yonghong,
An update on this thread. I successfully reproduced this issue on a
8.2.0 gcc compiler, It looks like gcc 4.9 did not have this issue. I
was also using clang which did not show this bug.
It seems having a DW_AT_specification that refers to another
DW_TAG_variable isn't handled in pahole. I have a (maybe hacky) pahole
patch as fix and let me clean it up and post for review soon.
Hao
On 8/24/20 5:05 PM, Hao Luo wrote:
> Yonghong,
>
> An update on this thread. I successfully reproduced this issue on a
> 8.2.0 gcc compiler, It looks like gcc 4.9 did not have this issue. I
> was also using clang which did not show this bug.
>
> It seems having a DW_AT_specification that refers to another
> DW_TAG_variable isn't handled in pahole. I have a (maybe hacky) pahole
> patch as fix and let me clean it up and post for review soon.
Sounds good. Thanks for fixing it in pahole! People may use gcc 4.9 up
to gcc 10, or llvm compiler (let us say llvm10 which is the version
which has CO-RE support and is recommended for compiling bpf
programs.) Once you have fix for gcc 8.2, it might be worthwhile to
check a few other gcc/llvm version (at least gcc8/9/10 and llvm10)
to ensure it still works.
Currently, by default kernel is using DWARF2, pahole has a good
support for dwarf2. I am not sure whether it supports dwarf4 well
or not. But I think additional dwarf4 support, if needed, can
be done a little bit later as dwarf4 is not widely used yet for kernel,
I think.
>
> Hao
>