On Sun, Jun 15, 2025 at 7:00 AM Alexis Lothoré <alexis.loth...@bootlin.com> wrote: > > 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.loth...@bootlin.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-0a32fe723...@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-0a32fe723...@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.
yes > 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. so be it. If syzbot was taught to fuzz dwarf I'm sure it would have exposed hundreds of bugs in the format itself and compilers, but since such convoluted constructs are not present in the kernel source code it's not a concern.
