Re: [PATCH 3/3] btf_encoder: Change functions check due to broken dwarf

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Nov 11, 2020 at 12:20 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> On Wed, Nov 11, 2020 at 11:59:20AM -0800, Andrii Nakryiko wrote:
>
> SNIP
>
> > > +       if (!fl->init_bpf_begin &&
> > > +           !strcmp("__init_bpf_preserve_type_begin", elf_sym__name(sym, btfe->symtab)))
> > > +               fl->init_bpf_begin = sym->st_value;
> > > +
> > > +       if (!fl->init_bpf_end &&
> > > +           !strcmp("__init_bpf_preserve_type_end", elf_sym__name(sym, btfe->symtab)))
> > > +               fl->init_bpf_end = sym->st_value;
> > > +}
> > > +
> > > +static int has_all_symbols(struct funcs_layout *fl)
> > > +{
> > > +       return fl->mcount_start && fl->mcount_stop &&
> > > +              fl->init_begin && fl->init_end &&
> > > +              fl->init_bpf_begin && fl->init_bpf_end;
> >
> > See below for what seems to be the root cause for the immediate problem.
> >
> > But me, Alexei and Daniel had a discussion offline, and we concluded
> > that this special bpf_preserve_init section is probably not the right
> > approach overall. We should roll back the bpf patch and instead adjust
> > pahole's approach. I think we should just drop the __init check and
> > include all the __init functions into BTF. There could be cases where
> > we'd need to attach BPF programs to __init functions (e.g., bpf_lsm
> > security cases), so having BTFs for those FUNCs are necessary as well.
> > Ftrace currently disallows that, but it's only because no user-space
> > application has a way to attach probes early enough. This might change
> > in the future, so there is no need to invent special mechanisms now
> > for bpf_iter function preservation. Let's just include all __init
> > functions in BTF. Can you please do that change and check how much
> > more functions we get in BTF? Thanks!
>
> sure, not problem to keep all init functions, will give you the count
>
> SNIP
>
> > >
> > > +static bool has_arg_names(struct cu *cu, struct ftype *ftype)
> > > +{
> > > +       struct parameter *param;
> > > +       const char *name;
> > > +
> > > +       ftype__for_each_parameter(ftype, param) {
> > > +               name = dwarves__active_loader->strings__ptr(cu, param->name);
> > > +               if (name == NULL)
> > > +                       return false;
> > > +       }
> > > +       return true;
> > > +}
> > > +
> >
> > I suspect (but haven't verified) that the problem is in this function.
> > If it happens that DWARF for a function has no arguments, then we'll
> > conclude it has all arg names. Don't know what's the best solution
> > here, but please double-check this.
> >
> > Specifically, two selftests are failing now. One of them:
> >
> > libbpf: load bpf program failed: Permission denied
> > libbpf: -- BEGIN DUMP LOG ---
> > libbpf:
> > arg#0 type is not a struct
> > Unrecognized arg#0 type PTR
> > ; int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
> > 0: (79) r6 = *(u64 *)(r1 +0)
> > func 'security_inode_getattr' doesn't have 1-th argument
> > invalid bpf_context access off=0 size=8
> > processed 1 insns (limit 1000000) max_states_per_insn 0 total_states 0
> > peak_states 0 mark_read 0
> > libbpf: -- END LOG --
> > libbpf: failed to load program 'prog_stat'
> > libbpf: failed to load object 'test_d_path'
> > libbpf: failed to load BPF skeleton 'test_d_path': -4007
> > test_d_path:FAIL:setup d_path skeleton failed
> > #27 d_path:FAIL
> >
> > This is because in generated BTF security_inode_getattr has a
> > prototype void security_inode_getattr(void); And once we emit this
> > prototype, due to logic in should_generate_function() we won't attempt
> > to do it again, even for the prototype with the right arguments.
>
> hum it works for me :-\
>
>         #27 d_path:OK
>
> with:
>
>         [25962] FUNC_PROTO '(anon)' ret_type_id=17 vlen=1
>                 'path' type_id=729
>         [31327] FUNC 'security_inode_getattr' type_id=25962 linkage=static
>
>
> perhaps your gcc generates DWARF that breaks the way you described
> above, but I'd expect to see function with argument without name,
> not function without arguments at all
>
> what gcc version are you on?

10.2.0, built from sources

>
> when you dump debug information, do you see security_inode_getattr
> record with no arguments?

Yeah, I think so:

21158467- <1><2b7e168>: Abbrev Number: 93 (DW_TAG_subprogram)
21158468-    <2b7e169>   DW_AT_external    : 1
21158469-    <2b7e169>   DW_AT_declaration : 1

  ..  BTW, we should probably still ignore DW_AT_declaration: 1, if it's set.

21158470:    <2b7e169>   DW_AT_linkage_name: (indirect string, offset:
0x120a0a): security_inode_getattr
21158471:    <2b7e16d>   DW_AT_name        : (indirect string, offset:
0x120a0a): security_inode_getattr
21158472-    <2b7e171>   DW_AT_decl_file   : 141
21158473-    <2b7e172>   DW_AT_decl_line   : 346
21158474-    <2b7e174>   DW_AT_decl_column : 5

...

36920783- <1><4c3bc3c>: Abbrev Number: 26 (DW_TAG_subprogram)
36920784-    <4c3bc3d>   DW_AT_external    : 1
36920785:    <4c3bc3d>   DW_AT_name        : (indirect string, offset:
0x120a0a): security_inode_getattr
36920786-    <4c3bc41>   DW_AT_decl_file   : 1
36920787-    <4c3bc42>   DW_AT_decl_line   : 1275
36920788-    <4c3bc44>   DW_AT_decl_column : 5
36920789-    <4c3bc45>   DW_AT_prototyped  : 1
36920790-    <4c3bc45>   DW_AT_type        : <0x4c17ffc>
36920791-    <4c3bc49>   DW_AT_low_pc      : 0xffffffff817d9d70
36920792-    <4c3bc51>   DW_AT_high_pc     : 0x67
36920793-    <4c3bc59>   DW_AT_frame_base  : 1 byte block: 9c
(DW_OP_call_frame_cfa)
36920794-    <4c3bc5b>   DW_AT_GNU_all_call_sites: 1
36920795-    <4c3bc5b>   DW_AT_sibling     : <0x4c3be10>
36920796- <2><4c3bc5f>: Abbrev Number: 17 (DW_TAG_formal_parameter)
36920797-    <4c3bc60>   DW_AT_name        : (indirect string, offset:
0x137dc3): path
36920798-    <4c3bc64>   DW_AT_decl_file   : 1
36920799-    <4c3bc65>   DW_AT_decl_line   : 1275
36920800-    <4c3bc67>   DW_AT_decl_column : 47
36920801-    <4c3bc68>   DW_AT_type        : <0x4c22144>
36920802-    <4c3bc6c>   DW_AT_location    : 0x1b2122c (location list)
36920803-    <4c3bc70>   DW_AT_GNU_locviews: 0x1b21226


>
> thanks,
> jirka
>



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux