netdev - Re: [PATCH bpf-next 1/2] bpf: Allow ld_imm64 instruction to point to kfunc.

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]

Date:   Thu, 16 Mar 2023 18:39:09 -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 04:06:02PM -0700, Andrii Nakryiko wrote:
> 
> /* pass function pointer through asm otherwise compiler assumes that
> any function != 0 */
> 
> comment was referring to compiler assuming that function != 0 for
> __weak symbol? I definitely didn't read it this way. And "compiler
> assumes that function != 0" seems a bit too strong of a statement,
> because at least Clang doesn't.

Correct. Instead of 'any function' it should be 'any non-weak function'.

> 
> But for macro, it's not kfunc-specific (and macro itself has no way to
> check that you are actually passing kfunc ksym), so even if it was a
> macro, it would be better to call it something more generic like
> bpf_ksym_exists() (though that will work for .kconfig, yet will be
> inappropriately named).

Rigth. bpf_ksym_exists() is what I proposed couple emails ago in my reply to Ed.

> The asm bit, though, seems to be a premature thing that can hide real
> compiler issues, so I'm still hesitant to add it. It should work today
> with modern compilers, so I'd test and validate this.

We're using asm in lots of place to avoid fighting with compiler.
This is just one of them, but I found a different way to prevent
silent optimizations. I'll go with:

#define bpf_ksym_exists(sym) \
       ({ _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); !!sym; })

It will address the silent dead code elimination issue and
will detect missing weak immediately at build time.

Just going with:

if (bpf_rcu_read_lock) // comment about weak
   bpf_rcu_read_lock();
else
  ..

and forgetting to use __weak in bpf_rcu_read_lock() declaration will make it
"work" on current kernels. The compiler will optimize 'if' and 'else' out and
libbpf will happily find that kfunc in the kernel.
While the program will fail to load on kernels without this kfunc much later with:
libbpf: extern (func ksym) 'bpf_rcu_read_lock': not found in kernel or module BTFs.

Which is the opposite of what that block of bpf code wanted to achieve.

> > It works, but how many people know that unknown weak resolves to zero ?
> > I didn't know until yesterday.
> 
> I was explicit about this from the very beginning of BPF CO-RE work.
> ksyms were added later, but semantics was consistent between .kconfig
> and .ksym. Documentation can't be ever good enough and discoverable
> enough (like [0]), of course, but let's do our best to make it as
...
>   [0] https://nakryiko.com/posts/bpf-core-reference-guide/#kconfig-extern-variables

I read it long ago, but reading is one thing and remembering small details is another.