Message-ID: <CAADnVQLud8-+pexQo8rscVM2d8K2dsYU1rJbFGK2ZZygdAgkQA@mail.gmail.com>
Date:   Thu, 16 Mar 2023 15:25:56 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...nel.org>,
        David Vernet <void@...ifault.com>,
        Dave Marchevsky <davemarchevsky@...a.com>,
        Tejun Heo <tj@...nel.org>,
        Kumar Kartikeya Dwivedi <memxor@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 1/2] bpf: Allow ld_imm64 instruction to point to kfunc.

On Thu, Mar 16, 2023 at 1:34 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Wed, Mar 15, 2023 at 3:36 PM Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
> >
> > From: Alexei Starovoitov <ast@...nel.org>
> >
> > Allow ld_imm64 insn with BPF_PSEUDO_BTF_ID to hold the address of kfunc.
> > PTR_MEM is already recognized for NULL-ness by is_branch_taken(),
> > so unresolved kfuncs will be seen as zero.
> > This allows BPF programs to detect at load time whether kfunc is present
> > in the kernel with bpf_kfunc_exists() macro.
> >
> > Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> > ---
> >  kernel/bpf/verifier.c       | 7 +++++--
> >  tools/lib/bpf/bpf_helpers.h | 3 +++
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 60793f793ca6..4e49d34d8cd6 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -15955,8 +15955,8 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
> >                 goto err_put;
> >         }
> >
> > -       if (!btf_type_is_var(t)) {
> > -               verbose(env, "pseudo btf_id %d in ldimm64 isn't KIND_VAR.\n", id);
> > +       if (!btf_type_is_var(t) && !btf_type_is_func(t)) {
> > +               verbose(env, "pseudo btf_id %d in ldimm64 isn't KIND_VAR or KIND_FUNC\n", id);
> >                 err = -EINVAL;
> >                 goto err_put;
> >         }
> > @@ -15990,6 +15990,9 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
> >                 aux->btf_var.reg_type = PTR_TO_BTF_ID | MEM_PERCPU;
> >                 aux->btf_var.btf = btf;
> >                 aux->btf_var.btf_id = type;
> > +       } else if (!btf_type_is_func(t)) {
> > +               aux->btf_var.reg_type = PTR_TO_MEM | MEM_RDONLY;
> > +               aux->btf_var.mem_size = 0;
> >         } else if (!btf_type_is_struct(t)) {
> >                 const struct btf_type *ret;
> >                 const char *tname;
> > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > index 7d12d3e620cc..43abe4c29409 100644
> > --- a/tools/lib/bpf/bpf_helpers.h
> > +++ b/tools/lib/bpf/bpf_helpers.h
> > @@ -177,6 +177,9 @@ enum libbpf_tristate {
> >  #define __kptr_untrusted __attribute__((btf_type_tag("kptr_untrusted")))
> >  #define __kptr __attribute__((btf_type_tag("kptr")))
> >
> > +/* pass function pointer through asm otherwise compiler assumes that any function != 0 */
> > +#define bpf_kfunc_exists(fn) ({ void *__p = fn; asm ("" : "+r"(__p)); __p; })
> > +
>
> I think we shouldn't add this helper macro. It just obfuscates a
> misuse of libbpf features and will be more confusing in practice.

I don't think it obfuscates anything.
If __weak is missing in extern declaration of kfunc the libbpf will
error anyway, so there is no danger to miss it.

> If I understand the comment, that asm is there to avoid compiler
> optimization of *knowing* that kfunc exists (it's extern is resolved
> to something other than 0), even if kfunc's ksym is not declared with
> __weak.

That's the current behavior of the combination of llvm and libbpf.
Resolution of weak is a linker job. libbpf loading process is
equivalent to linking. It's "linking" bpf elf .o into kernel and
resolving weak symbols.
We can document and guarantee that libbpf will evaluate
unresolved into zero, but we might have a hard time doing the same with
compilers. It's currently the case for LLVM and I hope GCC will follow.
Here is nice writeup about weak:
https://maskray.me/blog/2021-04-25-weak-symbol

Two things to notice in that writeup:
1. "Unresolved weak symbols have a zero value."
This is part of ELF spec for linkers.
In our case it applies to libbpf.
But it doesn't apply to compilers.

2. "GCC<5 (at least x86_64 and arm) may emit PC-relative relocations
for hidden undefined weak symbols. GCC<5 i386 may optimize if (&foo)
foo(); to unconditional foo();"

In other words if compiler implements weak as PC-relative
the optimizer part of the compiler may consider it as always not-null
and optimize the check out.

We can try to prevent that in LLVM and GCC compilers.
Another approach is to have a macro that passes weak addr through asm
which prevents such optimization.
So we still rely on libbpf resolving to zero while "linking" and
macro prevents compilers from messing up the check.
I feel it's safer to do it with macro.

I guess I'm fine leaving it out for now and just do
if (bpf_rcu_read_lock)
   bpf_rcu_read_lock();

though such code looks ugly and begs for a comment or macro like:

if (bpf_kfunc_exists(bpf_rcu_read_lock))
   bpf_rcu_read_lock();

or

if (bpf_rcu_read_lock) // unknown weak kfunc resolve to zero by libbpf
   // and compiler won't optimize it out
   bpf_rcu_read_lock();

but adding such comment everywhere feels odd.
macro is a cleaner way to document the code.

> __weak has a consistent semantics to indicate something that's
> optional. This is documented (e.g., [0] for kconfig variables) We do
> have tests making sure this works for weak __kconfig and variable
> __ksyms. Let's add similar ones for kfunc ksyms.

Right. We should definitely document that libbpf resolves
unknown __ksym __weak into zero.

> +       if (bpf_iter_num_new2) { // works
> +               struct bpf_iter_num it;
> +               bpf_iter_num_new2(&it, 0, 100);
> +               bpf_iter_num_destroy(&it);
> +       }

It works, but how many people know that unknown weak resolves to zero ?
I didn't know until yesterday.

Powered by blists - more mailing lists