netdev - Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <852b205b-1d32-1d85-5dcf-0edfa8efcd6a@solarflare.com>
Date:   Fri, 19 Oct 2018 18:04:11 +0100
From:   Edward Cree <ecree@...arflare.com>
To:     Martin Lau <kafai@...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 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
 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.

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