From: Martin KaFai Lau <kafai@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Kernel Team <kernel-team@fb.com>,
Networking <netdev@vger.kernel.org>
Subject: Re: [PATCH bpf-next 02/15] bpf: btf: Support parsing extern func
Date: Fri, 19 Mar 2021 15:45:32 -0700 [thread overview]
Message-ID: <20210319224532.7wlmhrgtrvkwqmzg@kafai-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <CAEf4Bza6Fiv+AFJc9-L=S6Duvm7wyyjvqoDGEED3TDTmwiZvVQ@mail.gmail.com>
On Fri, Mar 19, 2021 at 03:29:57PM -0700, Andrii Nakryiko wrote:
> On Fri, Mar 19, 2021 at 3:19 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Fri, Mar 19, 2021 at 02:27:13PM -0700, Andrii Nakryiko wrote:
> > > On Thu, Mar 18, 2021 at 10:29 PM Martin KaFai Lau <kafai@fb.com> wrote:
> > > >
> > > > On Thu, Mar 18, 2021 at 09:13:56PM -0700, Andrii Nakryiko wrote:
> > > > > On Thu, Mar 18, 2021 at 4:39 PM Martin KaFai Lau <kafai@fb.com> wrote:
> > > > > >
> > > > > > On Thu, Mar 18, 2021 at 03:53:38PM -0700, Andrii Nakryiko wrote:
> > > > > > > On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@fb.com> wrote:
> > > > > > > >
> > > > > > > > This patch makes BTF verifier to accept extern func. It is used for
> > > > > > > > allowing bpf program to call a limited set of kernel functions
> > > > > > > > in a later patch.
> > > > > > > >
> > > > > > > > When writing bpf prog, the extern kernel function needs
> > > > > > > > to be declared under a ELF section (".ksyms") which is
> > > > > > > > the same as the current extern kernel variables and that should
> > > > > > > > keep its usage consistent without requiring to remember another
> > > > > > > > section name.
> > > > > > > >
> > > > > > > > For example, in a bpf_prog.c:
> > > > > > > >
> > > > > > > > extern int foo(struct sock *) __attribute__((section(".ksyms")))
> > > > > > > >
> > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1
> > > > > > > > '(anon)' type_id=18
> > > > > > > > [25] FUNC 'foo' type_id=24 linkage=extern
> > > > > > > > [ ... ]
> > > > > > > > [33] DATASEC '.ksyms' size=0 vlen=1
> > > > > > > > type_id=25 offset=0 size=0
> > > > > > > >
> > > > > > > > LLVM will put the "func" type into the BTF datasec ".ksyms".
> > > > > > > > The current "btf_datasec_check_meta()" assumes everything under
> > > > > > > > it is a "var" and ensures it has non-zero size ("!vsi->size" test).
> > > > > > > > The non-zero size check is not true for "func". This patch postpones the
> > > > > > > > "!vsi-size" test from "btf_datasec_check_meta()" to
> > > > > > > > "btf_datasec_resolve()" which has all types collected to decide
> > > > > > > > if a vsi is a "var" or a "func" and then enforce the "vsi->size"
> > > > > > > > differently.
> > > > > > > >
> > > > > > > > If the datasec only has "func", its "t->size" could be zero.
> > > > > > > > Thus, the current "!t->size" test is no longer valid. The
> > > > > > > > invalid "t->size" will still be caught by the later
> > > > > > > > "last_vsi_end_off > t->size" check. This patch also takes this
> > > > > > > > chance to consolidate other "t->size" tests ("vsi->offset >= t->size"
> > > > > > > > "vsi->size > t->size", and "t->size < sum") into the existing
> > > > > > > > "last_vsi_end_off > t->size" test.
> > > > > > > >
> > > > > > > > The LLVM will also put those extern kernel function as an extern
> > > > > > > > linkage func in the BTF:
> > > > > > > >
> > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1
> > > > > > > > '(anon)' type_id=18
> > > > > > > > [25] FUNC 'foo' type_id=24 linkage=extern
> > > > > > > >
> > > > > > > > This patch allows BTF_FUNC_EXTERN in btf_func_check_meta().
> > > > > > > > Also extern kernel function declaration does not
> > > > > > > > necessary have arg name. Another change in btf_func_check() is
> > > > > > > > to allow extern function having no arg name.
> > > > > > > >
> > > > > > > > The btf selftest is adjusted accordingly. New tests are also added.
> > > > > > > >
> > > > > > > > The required LLVM patch: https://reviews.llvm.org/D93563
> > > > > > > >
> > > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > > > > > > > ---
> > > > > > >
> > > > > > > High-level question about EXTERN functions in DATASEC. Does kernel
> > > > > > > need to see them under DATASEC? What if libbpf just removed all EXTERN
> > > > > > > funcs from under DATASEC and leave them as "free-floating" EXTERN
> > > > > > > FUNCs in BTF.
> > > > > > >
> > > > > > > We need to tag EXTERNs with DATASECs mainly for libbpf to know whether
> > > > > > > it's .kconfig or .ksym or other type of externs. Does kernel need to
> > > > > > > care?
> > > > > > Although the kernel does not need to know, since the a legit llvm generates it,
> > > > > > I go with a proper support in the kernel (e.g. bpftool btf dump can better
> > > > > > reflect what was there).
> > > > >
> > > > > LLVM also generates extern VAR with BTF_VAR_EXTERN, yet libbpf is
> > > > > replacing it with fake INTs.
> > > > Yep. I noticed the loop in collect_extern() in libbpf.
> > > > It replaces the var->type with INT.
> > > >
> > > > > We could do just that here as well.
> > > > What to replace in the FUNC case?
> > >
> > > if we do that, I'd just replace them with same INTs. Or we can just
> > > remove the entire DATASEC. Now it is easier to do with BTF write APIs.
> > > Back then it was a major pain. I'd probably get rid of DATASEC
> > > altogether instead of that INT replacement, if I had BTF write APIs.
> > Do you mean vsi->type = INT?
>
> yes, that's what existing logic does for EXTERN var
There may be no var.
>
> >
> > >
> > > >
> > > > Regardless, supporting it properly in the kernel is a better way to go
> > > > instead of asking the userspace to move around it. It is not very
> > > > complicated to support it in the kernel also.
> > > >
> > > > What is the concern of having the kernel to support it?
> > >
> > > Just more complicated BTF validation logic, which means that there are
> > > higher chances of permitting invalid BTF. And then the question is
> > > what can the kernel do with those EXTERNs in BTF? Probably nothing.
> > > And that .ksyms section is special, and purely libbpf convention.
> > > Ideally kernel should not allow EXTERN funcs in any other DATASEC. Are
> > > you willing to hard-code ".ksyms" name in kernel for libbpf's sake?
> > > Probably not. The general rule, so far, was that kernel shouldn't see
> > > any unresolved EXTERN at all. Now it's neither here nor there. EXTERN
> > > funcs are ok, EXTERN vars are not.
> > Exactly, it is libbpf convention. The kernel does not need to enforce it.
> > The kernel only needs to be able to support the debug info generated by
> > llvm and being able to display/dump it later.
> >
> > There are many other things in the BTF that the kernel does not need to
>
> Curious, what are those many other things?
VAR '_license'.
deeper things could be STRUCT 'tcp_congestion_ops' and the types under it.
>
> > know. It is there for debug purpose which the BTF is used for. Yes,
> > the func call can be discovered by instruction dump. It is also nice to
> > see everything in one ksyms datasec also during btf dump.
> >
> > If there is a need to strip everything that the kernel does not need
> > from the BTF, it can all be stripped in another "--strip-debug" like
> > option.
>
> Where does this "--strip-debug" option go? Clang, pahole, or bpftool?
> Or am I misunderstanding what you are proposing?
Could be a libbpf option during load? or it can be done during gen skel?
>
> >
> > To support EXTERN var, the kernel part should be fine. I am only not
> > sure why it has to change the vs->size and vs->offset in libbpf?
>
> vs->size and vs->offset are adjusted to match int type. Otherwise
> kernel BTF validation will complain about DATASEC size mismatch.
make sense. so if there is no need to replace it with INT,
they can be left as is?
>
> >
> >
> > >
> > > >
> > > > > If anyone would want to know all the kernel functions that some BPF
> > > > > program is using, they could do it from the instruction dump, with
> > > > > proper addresses and kernel function names nicely displayed there.
> > > > > That's way more useful, IMO.
next prev parent reply other threads:[~2021-03-19 22:46 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-16 1:13 [PATCH bpf-next 00/15] Support calling kernel function Martin KaFai Lau
2021-03-16 1:13 ` [PATCH bpf-next 01/15] bpf: Simplify freeing logic in linfo and jited_linfo Martin KaFai Lau
2021-03-16 1:13 ` [PATCH bpf-next 02/15] bpf: btf: Support parsing extern func Martin KaFai Lau
2021-03-18 22:53 ` Andrii Nakryiko
2021-03-18 23:39 ` Martin KaFai Lau
2021-03-19 4:13 ` Andrii Nakryiko
2021-03-19 5:29 ` Martin KaFai Lau
2021-03-19 21:27 ` Andrii Nakryiko
2021-03-19 22:19 ` Martin KaFai Lau
2021-03-19 22:29 ` Andrii Nakryiko
2021-03-19 22:45 ` Martin KaFai Lau [this message]
2021-03-19 23:02 ` Andrii Nakryiko
2021-03-20 0:13 ` Martin KaFai Lau
2021-03-20 17:18 ` Andrii Nakryiko
2021-03-23 4:55 ` Martin KaFai Lau
2021-03-16 1:13 ` [PATCH bpf-next 03/15] bpf: Refactor btf_check_func_arg_match Martin KaFai Lau
2021-03-18 23:32 ` Andrii Nakryiko
2021-03-19 19:32 ` Martin KaFai Lau
2021-03-19 21:51 ` Andrii Nakryiko
2021-03-20 0:10 ` Alexei Starovoitov
2021-03-20 17:13 ` Andrii Nakryiko
2021-03-16 1:14 ` [PATCH bpf-next 04/15] bpf: Support bpf program calling kernel function Martin KaFai Lau
2021-03-19 1:03 ` Andrii Nakryiko
2021-03-19 1:51 ` Alexei Starovoitov
2021-03-19 19:47 ` Martin KaFai Lau
2021-03-16 1:14 ` [PATCH bpf-next 05/15] bpf: Support kernel function call in x86-32 Martin KaFai Lau
2021-03-16 1:14 ` [PATCH bpf-next 06/15] tcp: Rename bictcp function prefix to cubictcp Martin KaFai Lau
2021-03-16 1:14 ` [PATCH bpf-next 07/15] bpf: tcp: White list some tcp cong functions to be called by bpf-tcp-cc Martin KaFai Lau
2021-03-19 1:19 ` Andrii Nakryiko
2021-03-16 1:14 ` [PATCH bpf-next 08/15] libbpf: Refactor bpf_object__resolve_ksyms_btf_id Martin KaFai Lau
2021-03-19 2:53 ` Andrii Nakryiko
2021-03-16 1:14 ` [PATCH bpf-next 09/15] libbpf: Refactor codes for finding btf id of a kernel symbol Martin KaFai Lau
2021-03-19 3:14 ` Andrii Nakryiko
2021-03-16 1:14 ` [PATCH bpf-next 10/15] libbpf: Rename RELO_EXTERN to RELO_EXTERN_VAR Martin KaFai Lau
2021-03-19 3:15 ` Andrii Nakryiko
2021-03-16 1:14 ` [PATCH bpf-next 11/15] libbpf: Record extern sym relocation first Martin KaFai Lau
2021-03-19 3:16 ` Andrii Nakryiko
2021-03-16 1:14 ` [PATCH bpf-next 12/15] libbpf: Support extern kernel function Martin KaFai Lau
2021-03-19 4:11 ` Andrii Nakryiko
2021-03-19 5:06 ` Martin KaFai Lau
2021-03-19 21:38 ` Andrii Nakryiko
2021-03-16 1:14 ` [PATCH bpf-next 13/15] bpf: selftests: Rename bictcp to bpf_cubic Martin KaFai Lau
2021-03-19 4:14 ` Andrii Nakryiko
2021-03-16 1:15 ` [PATCH bpf-next 14/15] bpf: selftest: bpf_cubic and bpf_dctcp calling kernel functions Martin KaFai Lau
2021-03-19 4:15 ` Andrii Nakryiko
2021-03-16 1:15 ` [PATCH bpf-next 15/15] bpf: selftest: Add kfunc_call test Martin KaFai Lau
2021-03-16 3:39 ` kernel test robot
2021-03-19 4:21 ` Andrii Nakryiko
2021-03-19 5:40 ` Martin KaFai Lau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210319224532.7wlmhrgtrvkwqmzg@kafai-mbp.dhcp.thefacebook.com \
--to=kafai@fb.com \
--cc=andrii.nakryiko@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).