Message-ID: <20181019203545.6nnnok5loyyrlel4@kafai-mbp.dhcp.thefacebook.com>
Date:   Fri, 19 Oct 2018 20:35:51 +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 12:36:49PM -0700, Martin Lau wrote:
> 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.
Discussed a bit offline with folks about the two id spaces
situation and it is not good for debugging purpose.

If we must get rid of FUNC_PROTO, it is better to use the
name_off==0 check instead of adding a new function section.
We will go for this path in the next respin.

> 
> > 
> > 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