Message-Id: <DAN5THWRO6KS.XXZ00IOTQZH9@bootlin.com>
Date: Sun, 15 Jun 2025 16:00:30 +0200
From: Alexis Lothoré <alexis.lothore@...tlin.com>
To: "Alexei Starovoitov" <alexei.starovoitov@...il.com>
Cc: "Peter Zijlstra" <peterz@...radead.org>, "Alexei Starovoitov"
 <ast@...nel.org>, "Daniel Borkmann" <daniel@...earbox.net>, "Andrii
 Nakryiko" <andrii@...nel.org>, "Martin KaFai Lau" <martin.lau@...ux.dev>,
 "Eduard Zingerman" <eddyz87@...il.com>, "Song Liu" <song@...nel.org>,
 "Yonghong Song" <yonghong.song@...ux.dev>, "John Fastabend"
 <john.fastabend@...il.com>, "KP Singh" <kpsingh@...nel.org>, "Stanislav
 Fomichev" <sdf@...ichev.me>, "Hao Luo" <haoluo@...gle.com>, "Jiri Olsa"
 <jolsa@...nel.org>, "David S. Miller" <davem@...emloft.net>, "David Ahern"
 <dsahern@...nel.org>, "Thomas Gleixner" <tglx@...utronix.de>, "Ingo Molnar"
 <mingo@...hat.com>, "Borislav Petkov" <bp@...en8.de>, "Dave Hansen"
 <dave.hansen@...ux.intel.com>, "X86 ML" <x86@...nel.org>, "H. Peter Anvin"
 <hpa@...or.com>, "Menglong Dong" <imagedong@...cent.com>,
 Björn Töpel <bjorn@...nel.org>, "Pu Lehui"
 <pulehui@...wei.com>, "Puranjay Mohan" <puranjay@...nel.org>, "Paul
 Walmsley" <paul.walmsley@...ive.com>, "Palmer Dabbelt"
 <palmer@...belt.com>, "Albert Ou" <aou@...s.berkeley.edu>, "Alexandre
 Ghiti" <alex@...ti.fr>, "Ilya Leoshkevich" <iii@...ux.ibm.com>, "Heiko
 Carstens" <hca@...ux.ibm.com>, "Vasily Gorbik" <gor@...ux.ibm.com>,
 "Alexander Gordeev" <agordeev@...ux.ibm.com>, "Christian Borntraeger"
 <borntraeger@...ux.ibm.com>, "Sven Schnelle" <svens@...ux.ibm.com>, "Hari
 Bathini" <hbathini@...ux.ibm.com>, "Christophe Leroy"
 <christophe.leroy@...roup.eu>, "Naveen N Rao" <naveen@...nel.org>,
 "Madhavan Srinivasan" <maddy@...ux.ibm.com>, "Michael Ellerman"
 <mpe@...erman.id.au>, "Nicholas Piggin" <npiggin@...il.com>, "Mykola
 Lysenko" <mykolal@...com>, "Shuah Khan" <shuah@...nel.org>, "Maxime
 Coquelin" <mcoquelin.stm32@...il.com>, "Alexandre Torgue"
 <alexandre.torgue@...s.st.com>, <ebpf@...uxfoundation.org>, "Thomas
 Petazzoni" <thomas.petazzoni@...tlin.com>, "Bastien Curutchet"
 <bastien.curutchet@...tlin.com>, "Network Development"
 <netdev@...r.kernel.org>, "bpf" <bpf@...r.kernel.org>, "LKML"
 <linux-kernel@...r.kernel.org>, Björn Töpel
 <bjorn@...osinc.com>, "linux-riscv" <linux-riscv@...ts.infradead.org>,
 "linux-s390" <linux-s390@...r.kernel.org>, "ppc-dev"
 <linuxppc-dev@...ts.ozlabs.org>, "open list:KERNEL SELFTEST FRAMEWORK"
 <linux-kselftest@...r.kernel.org>,
 <linux-stm32@...md-mailman.stormreply.com>, "linux-arm-kernel"
 <linux-arm-kernel@...ts.infradead.org>, "dwarves" <dwarves@...r.kernel.org>
Subject: Re: [PATCH bpf 2/7] bpf/x86: prevent trampoline attachment when
 args location on stack is uncertain

On Sat Jun 14, 2025 at 12:35 AM CEST, Alexei Starovoitov wrote:
> On Fri, Jun 13, 2025 at 1:59 AM Alexis Lothoré
> <alexis.lothore@...tlin.com> wrote:
>>
>> On Fri Jun 13, 2025 at 10:32 AM CEST, Peter Zijlstra wrote:
>> > On Fri, Jun 13, 2025 at 10:26:37AM +0200, Alexis Lothoré wrote:

[...]

>> If I need to respin, I'll rewrite the commit message to include the details
>> above.
>
> No need to respin. The cover letter is quite detailed already.
>
> But looking at the patch and this thread I think we need to agree
> on the long term approach to BTF, since people assume that
> it's a more compact dwarf and any missing information
> should be added to it.
> Like in this case special alignment case and packed attributes
> are not expressed in BTF and I believe they should not be.
> BTF is not a debug format and not a substitute for dwarf.
> There is no goal to express everything possible in C.
> It's minimal, because BTF is _practical_ description of
> types and data present in the kernel.
> I don't think the special case of packing and alignment exists
> in the kernel today, so the current format is sufficient.
> It doesn't miss anything.
> I think we made arm64 JIT unnecessary restrictive and now considering
> to make all other JITs restrictive too for hypothetical case
> of some future kernel functions.
> I feel we're going in the wrong direction.
> Instead we should teach pahole to sanitize BTF where functions
> are using this fancy alignment and packed structs.
> pahole can see it in dwarf and can skip emitting BTF for such
> functions. Then the kernel JITs on all architectures won't even
> see such cases.
>
> The issue was initially discovered by a selftest:
> https://lore.kernel.org/bpf/20250411-many_args_arm64-v1-3-0a32fe72339e@bootlin.com/
> that attempted to support these two types:
> +struct bpf_testmod_struct_arg_4 {
> + __u64 a;
> + __u64 b;
> +};
> +
> +struct bpf_testmod_struct_arg_5 {
> + __int128 a;
> +};
>
> The former is present in the kernel. It's more or less sockptr_t,
> and people want to access it for observability in tracing.
> The latter doesn't exist in the kernel and we cannot represent
> it properly in BTF without losing alignment.
>
> So I think we should go back to that series:
> https://lore.kernel.org/bpf/20250411-many_args_arm64-v1-0-0a32fe72339e@bootlin.com/
>
> remove __int128 selftest, but also teach pahole
> to recognize types that cannot be represented in BTF and
> don't emit them either into vmlinux or in kernel module
> (like in this case it was bpf_testmod.ko)
> I think that would be a better path forward aligned
> with the long term goal of BTF.
>
> And before people ask... pahole is a trusted component of the build
> system. We trust it just as we trust gcc, clang, linker, objtool.

So if I understand correctly your point, it would be better to just move out
those constraints from the JIT compilers, and just not represent those special
cases in BTF, so that it becomes impossible to hook programs on those functions,
since they are not event present in BTF info.
And so:
- cancel this series
- revert the small ARM64 check about struct passed on stack
- update pahole to make sure that it does not encode info about this specific
  kind of functions.

I still expect some challenges with this. AFAIU pahole uses DWARF to generate
BTF, and discussions in [1] highlighted the fact that the attributes altering
the structs alignment are not reliably encoded in DWARF. Maybe pahole can
"guess" if a struct has been altered, by doing something like
btf_is_struct_packed in libbpf ? As Andrii mentioned in [2], it may not be
able to cover all cases, but that could  be a start. If that's indeed the
desired direction, I can take a further look at this.

+ CC dwarves ML

Alexis

[1] https://lore.kernel.org/bpf/9a2ba0ad-b34d-42f8-89a6-d9a44f007bdc@linux.dev/
[2] https://lore.kernel.org/bpf/CAEf4BzZHMYyGDZ4c4eNXG7Fm=ecxCCbKhKbQTbCjvWmKtdwvBw@mail.gmail.com/

Powered by blists - more mailing lists