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] [day] [month] [year] [list]

Date:   Fri, 17 Mar 2023 09:28:28 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...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 6:39 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> 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'.

ok, your new macro implementation seems to prevent usage of non-weak
ksyms, which is great

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

I don't see it, maybe some email was dropped. But sounds good.

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

Yep, I like this protection against using non-weak ksym with this
check. It's very helpful, thanks!

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

Yep. Good testing is still important, of course.

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

I like the new bpf_ksym_exists() implementation, now I think it adds
value, instead of hiding an issue.

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

That's understandable, no worries. I'm just saying that this is
officially supported semantics, so if compilers somehow break this,
I'd like to rely on BPF selftests to detect this early, thanks to BPF
CI's use of nightly Clang builds (and eventually hopefully we'll also
use GCC-BPF nightlies).