linux-kernel - Re: [PATCH bpf-next v2 3/3] bpf: Use BPF_KFUNC macro at all kfunc definitions

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: <Y9AFT4pTydKh+PD3@maniforge.lan>
Date:   Tue, 24 Jan 2023 10:20:31 -0600
From:   David Vernet <void@...ifault.com>
To:     Jonathan Corbet <corbet@....net>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        bpf@...r.kernel.org, ast@...nel.org, andrii@...nel.org,
        martin.lau@...ux.dev, song@...nel.org, yhs@...a.com,
        john.fastabend@...il.com, kpsingh@...nel.org, sdf@...gle.com,
        haoluo@...gle.com, jolsa@...nel.org, linux-kernel@...r.kernel.org,
        kernel-team@...a.com, memxor@...il.com, hch@...radead.org
Subject: Re: [PATCH bpf-next v2 3/3] bpf: Use BPF_KFUNC macro at all kfunc
 definitions

On Tue, Jan 24, 2023 at 07:50:31AM -0700, Jonathan Corbet wrote:
> David Vernet <void@...ifault.com> writes:
> 
> > I was perhaps a bit naive to think we could just throw a __bpf_kfunc
> > macro onto the function signatures and call it a day :-) I think it's
> > probably best to table this for now, and either I or someone else can
> > come back to it when we have bandwidth to solve the problem more
> > appropriately.
> 
> Now I feel bad ... I was just tossing out a thought, not wanting to
> bikeshed this work into oblivion.  If what you have solves a real

No apologies necessary. I don't think this qualifies as bikeshedding.
IMO folks are raising legitimate UX concerns, which is important and
worth getting right.

> problem and is the best that can be done now, perhaps it should just go
> in and a "more appropriate" solution can be adopted later, should
> somebody manage to come up with it?

That would be my preference, but I also understand folks' sentiment of
wanting to keep out what they feel like is odd syntax, as Christoph said
in [0], and Daniel alluded to earlier in this thread.

[0]: https://lore.kernel.org/all/Y8+FeH7rz8jDTubt@infradead.org/

I tested on an LTO build and wrapper kfuncs (with external linkage) were
not being stripped despite not being called from anywhere else in the
kernel, so for now I _think_ it's safe to call this patch set more of a
cleanup / future-proofing than solving an immediate and pressing problem
(as long as anyone adding kfuncs carefully follows the directions in
[1]). In other words, I think we have some time to do this the right way
without paying too much of a cost later. If we set up the UX correctly,
just adding an EXPORT_SYMBOL_KFUNC call (or something to that effect,
including just using BTF_ID_FLAGS) should be minimal effort even if
there are a lot more kfuncs by then.

[1]: https://docs.kernel.org/bpf/kfuncs.html

If it turns out that we start to observe problems in LTO builds without
specifying __used and/or noinline, or if folks are repeatedly making
mistakes when adding kfuncs (by e.g. not giving wrapper kfuncs external
linkage) then I think it would be a stronger case to get this in now and
fix it up later.

Thanks,
David