On Mon, 15 Apr 2024 at 17:32, Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote: > > On Mon, Apr 15, 2024 at 11:59 PM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote: > > > > On Mon, 15 Apr 2024 at 16:55, Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote: > > > > > > On Mon, Apr 15, 2024 at 4:58 PM Ard Biesheuvel <ardb+git@xxxxxxxxxx> wrote: > > > > > > > > From: Ard Biesheuvel <ardb@xxxxxxxxxx> > > > > > > > > If the BTF code is enabled in the build configuration, the start/stop > > > > BTF markers are guaranteed to exist in the final link but not during the > > > > first linker pass. > > > > > > > > Avoid GOT based relocations to these markers in the final executable by > > > > providing preliminary definitions that will be used by the first linker > > > > pass, and superseded by the actual definitions in the subsequent ones. > > > > > > > > Make the preliminary definitions dependent on CONFIG_DEBUG_INFO_BTF so > > > > that inadvertent references to this section will trigger a link failure > > > > if they occur in code that does not honour CONFIG_DEBUG_INFO_BTF. > > > > > > > > Note that Clang will notice that taking the address of__start_BTF cannot > > > > yield NULL any longer, so testing for that condition is no longer > > > > needed. > > > > > > > > Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > > > > Acked-by: Arnd Bergmann <arnd@xxxxxxxx> > > > > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx> > > > > --- > > > > include/asm-generic/vmlinux.lds.h | 9 +++++++++ > > > > kernel/bpf/btf.c | 7 +++++-- > > > > kernel/bpf/sysfs_btf.c | 6 +++--- > > > > 3 files changed, 17 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h > > > > index e8449be62058..4cb3d88449e5 100644 > > > > --- a/include/asm-generic/vmlinux.lds.h > > > > +++ b/include/asm-generic/vmlinux.lds.h > > > > @@ -456,6 +456,7 @@ > > > > * independent code. > > > > */ > > > > #define PRELIMINARY_SYMBOL_DEFINITIONS \ > > > > + PRELIMINARY_BTF_DEFINITIONS \ > > > > PROVIDE(kallsyms_addresses = .); \ > > > > PROVIDE(kallsyms_offsets = .); \ > > > > PROVIDE(kallsyms_names = .); \ > > > > @@ -466,6 +467,14 @@ > > > > PROVIDE(kallsyms_markers = .); \ > > > > PROVIDE(kallsyms_seqs_of_names = .); > > > > > > > > +#ifdef CONFIG_DEBUG_INFO_BTF > > > > +#define PRELIMINARY_BTF_DEFINITIONS \ > > > > + PROVIDE(__start_BTF = .); \ > > > > + PROVIDE(__stop_BTF = .); > > > > +#else > > > > +#define PRELIMINARY_BTF_DEFINITIONS > > > > +#endif > > > > + > > > > > > > > > > > > Is this necessary? > > > > > > > I think so. > > > > This actually resulted in Jiri's build failure with v2, and the > > realization that there was dead code in btf_parse_vmlinux() that > > happily tried to load the contents of the BTF section if > > CONFIG_DEBUG_INFO_BTF was not enabled to begin with. > > > > So this is another pitfall with weak references: the symbol may > > unexpectedly be missing altogether rather than only during the first > > linker pass. > > > > > > > > > > > The following code (BOUNDED_SECTION_BY) > > > produces __start_BTF and __stop_BTF symbols > > > under the same condition. > > > > > > > Indeed. So if CONFIG_DEBUG_INFO_BTF=n, code can still link to > > __start_BTF and __stop_BTF even though BTF is not enabled. > > > > I am talking about the case for CONFIG_DEBUG_INFO_BTF=y. > > PROVIDE() is meaningless because __start_BTF and __stop_BTF > are produced by the existing code. > > (The code was a bit clearer before commit > 9b351be25360c5cedfb98b88d6dfd89327849e52) > > > > So, v4 of this patch will look like 2/3, right? > > > Just drop __weak attribute. > > You still need > > if (!IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) > return ERR_PTR(-ENOENT); > > But, you do not need to modify vmlinux.lds.h > OK, I see what you mean now. The PROVIDE() is indeed unnecessary - I'll drop that bit in v4.
