Message-ID: <20181019193548.g4neaudunoan5gvn@kafai-mbp.dhcp.thefacebook.com>
Date:   Fri, 19 Oct 2018 19:36:49 +0000
From:   Martin Lau <kafai@...com>
To:     Edward Cree <ecree@...arflare.com>
CC:     Yonghong Song <yhs@...com>, Alexei Starovoitov <ast@...com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and
 BTF_KIND_FUNC_PROTO

On Fri, Oct 19, 2018 at 06:04:11PM +0100, Edward Cree wrote:
> On 18/10/18 22:19, Martin Lau wrote:
> > As I have mentioned earlier, it is also special to
> > the kernel because the BTF verifier and bpf_prog_load()
> > need to do different checks for FUNC and FUNC_PROTO to
> > ensure they are sane.
> >
> > First, we need to agree that the kernel needs to verify
> > them differently.
> >
> > and then we can proceed to discuss how to distinguish them.
> > We picked the current way to avoid adding a
> > new BTF function section and keep it
> > strict forward to distinguish them w/o relying
> > on other hints from 'struct btf_type'.
> >
> > Are you suggesting another way of doing it?
> But you *do* have such a new section.
> The patch comment talks about a 'FuncInfo Table' which appears to
Note that the new section, which contains the FuncInfo Table,
is in a new ELF section ".BTF.ext" instead of the ".BTF".
It is not in the ".BTF" section because it is only useful during
bpf_prog_load().

I was meaning a new function section within ".BTF".

>  map (section, insn_idx) to type_id.  (I think this gets added in
>  .BTF.ext per patch 9?)  So when you're looking at a FUNC type
>  because you looked up a type_id from that table, you know it's
>  the signature of a subprogram, and you're checking it as such.
> Whereas, if you were doing something with some other type and it
>  referenced a FUNC type (e.g., in the patch comment's example,
>  you're checking foo's first argument against the type bar) in
>  its type_id, you know you're using it as a formal type (a FUNC_
>  PROTO in your parlance) and not as a subprogram.
> The context in which you are using a type entry tells you which
>  kind it is.  And the verifier can and should be smart enough to
>  know what it's doing in this way.
> 
> And it's entirely reasonable for the same type entry to get used
>  for both those cases; in my example, you'd have a FUNC type for
>  int foo(int), referenced both by the func_info entry for foo
>  and by the PTR type for bar.  And if you had another subprogram
>  int baz(int), its func_info entry could reference the same
>  type_id, because the (reference to the) name of the function
>  should live in the func_info, not in the type.
IIUC, I think what you are suggesting here is to use (type_id, name)
to describe DW_TAG_subprogram "int foo1(int) {}", "int foo2(int) {}",
"int foo3(int) {}" where type_id here is referring to the same
DW_TAG_subroutine_type, and only define that _one_
DW_TAG_subroutine_type in the BTF "type" section.

That will require more manipulation/type-merging in the dwarf2btf
process and it could get quite complicated.

Note that CTF is also fully spelling out the return type
and arg types for each DW_TAG_subprogram in a separate
function section (still within the same ELF section).
The only difference here is they are merged into the type
section and FUNC_PROTO is added.

If the concern is having both FUNC and FUNC_PROTO is confusing,
we could go back to the CTF way which adds a new function section
in ".BTF" and it is only for DW_TAG_subprogram.
BTF_KIND_FUNC_PROTO is then no longer necessary.
Some of new BTF verifier checkings may actually go away also.
The down side is there will be two id spaces.

> 
> What you are proposing seems to be saying "if we have this
>  particular special btf_kind, then this BTF entry doesn't just
>  define a type, it declares a subprogram of that type".  Oh,
>  and with the name of the type as the subprogram name.  Which
>  just creates blurry confusion as to whether BTF entries define
>  types or declare objects; IMNSHO the correct approach is for
>  objects to be declared elsewhere and to reference BTF types by
>  their type_id.
> Which is what the func_info table in patch 9 appears to do.
> 
> (It also rather bothers me the way we are using special type
>  names to associate maps with their k/v types, rather than
>  extending the records in the maps section to include type_ids
>  referencing them.  It's the same kind of weird implicitness,
>  and if I'd spotted it when it was going in I'd've nacked it,
>  but I suppose it's ABI now and too late to change.)
> 
> -Ed

Powered by blists - more mailing lists