| Message ID | 20240415162041.2491523-5-ardb+git@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | kbuild: Avoid weak external linkage where possible | expand |
Hello: This series was applied to bpf/bpf-next.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Mon, 15 Apr 2024 18:20:42 +0200 you wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > Weak external linkage is intended for cases where a symbol reference > can remain unsatisfied in the final link. Taking the address of such a > symbol should yield NULL if the reference was not satisfied. > > Given that ordinary RIP or PC relative references cannot produce NULL, > some kind of indirection is always needed in such cases, and in position > independent code, this results in a GOT entry. In ordinary code, it is > arch specific but amounts to the same thing. > > [...] Here is the summary with links: - [v4,1/3] kallsyms: Avoid weak references for kallsyms symbols (no matching commit) - [v4,2/3] vmlinux: Avoid weak reference to notes section (no matching commit) - [v4,3/3] btf: Avoid weak external references https://git.kernel.org/bpf/bpf-next/c/fc5eb4a84e4c You are awesome, thank you!
On Tue, 16 Apr 2024 at 16:40, <patchwork-bot+netdevbpf@kernel.org> wrote: > > Hello: > > This series was applied to bpf/bpf-next.git (master) > by Daniel Borkmann <daniel@iogearbox.net>: > > On Mon, 15 Apr 2024 18:20:42 +0200 you wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > > > Weak external linkage is intended for cases where a symbol reference > > can remain unsatisfied in the final link. Taking the address of such a > > symbol should yield NULL if the reference was not satisfied. > > > > Given that ordinary RIP or PC relative references cannot produce NULL, > > some kind of indirection is always needed in such cases, and in position > > independent code, this results in a GOT entry. In ordinary code, it is > > arch specific but amounts to the same thing. > > > > [...] > > Here is the summary with links: > - [v4,1/3] kallsyms: Avoid weak references for kallsyms symbols > (no matching commit) > - [v4,2/3] vmlinux: Avoid weak reference to notes section > (no matching commit) > - [v4,3/3] btf: Avoid weak external references > https://git.kernel.org/bpf/bpf-next/c/fc5eb4a84e4c > Thanks. Masahiro, could you pick up patches #1 and #2 please?
On Fri, Apr 19, 2024 at 4:57 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Tue, 16 Apr 2024 at 16:40, <patchwork-bot+netdevbpf@kernel.org> wrote: > > > > Hello: > > > > This series was applied to bpf/bpf-next.git (master) > > by Daniel Borkmann <daniel@iogearbox.net>: > > > > On Mon, 15 Apr 2024 18:20:42 +0200 you wrote: > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > Weak external linkage is intended for cases where a symbol reference > > > can remain unsatisfied in the final link. Taking the address of such a > > > symbol should yield NULL if the reference was not satisfied. > > > > > > Given that ordinary RIP or PC relative references cannot produce NULL, > > > some kind of indirection is always needed in such cases, and in position > > > independent code, this results in a GOT entry. In ordinary code, it is > > > arch specific but amounts to the same thing. > > > > > > [...] > > > > Here is the summary with links: > > - [v4,1/3] kallsyms: Avoid weak references for kallsyms symbols > > (no matching commit) > > - [v4,2/3] vmlinux: Avoid weak reference to notes section > > (no matching commit) > > - [v4,3/3] btf: Avoid weak external references > > https://git.kernel.org/bpf/bpf-next/c/fc5eb4a84e4c > > > > > Thanks. > > Masahiro, could you pick up patches #1 and #2 please? > I do not like PROVIDE() because it potentially shifts a build error (i.e. link error) into a run-time error, which is usually more difficult to debug than build error. If someone references the kallsyms_* symbols when CONFIG_KALLSYMS=n, it is likely a mistake. In general, it should be reported as a link error. With PROVIDE() added, we will never detect it at a build time. Do you want me to pick up 1/3? -- Best Regards Masahiro Yamada
On Sat, 20 Apr 2024 at 14:32, Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Fri, Apr 19, 2024 at 4:57 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Tue, 16 Apr 2024 at 16:40, <patchwork-bot+netdevbpf@kernel.org> wrote: > > > > > > Hello: > > > > > > This series was applied to bpf/bpf-next.git (master) > > > by Daniel Borkmann <daniel@iogearbox.net>: > > > > > > On Mon, 15 Apr 2024 18:20:42 +0200 you wrote: > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > > > Weak external linkage is intended for cases where a symbol reference > > > > can remain unsatisfied in the final link. Taking the address of such a > > > > symbol should yield NULL if the reference was not satisfied. > > > > > > > > Given that ordinary RIP or PC relative references cannot produce NULL, > > > > some kind of indirection is always needed in such cases, and in position > > > > independent code, this results in a GOT entry. In ordinary code, it is > > > > arch specific but amounts to the same thing. > > > > > > > > [...] > > > > > > Here is the summary with links: > > > - [v4,1/3] kallsyms: Avoid weak references for kallsyms symbols > > > (no matching commit) > > > - [v4,2/3] vmlinux: Avoid weak reference to notes section > > > (no matching commit) > > > - [v4,3/3] btf: Avoid weak external references > > > https://git.kernel.org/bpf/bpf-next/c/fc5eb4a84e4c > > > > > > > > > Thanks. > > > > Masahiro, could you pick up patches #1 and #2 please? > > > > > I do not like PROVIDE() because it potentially shifts > a build error (i.e. link error) into > a run-time error, which is usually more difficult to debug > than build error. > > If someone references the kallsyms_* symbols > when CONFIG_KALLSYMS=n, it is likely a mistake. > In general, it should be reported as a link error. > OK, so the PROVIDE() should be conditional on CONFIG_KALLSYM=y. I can fix that. > With PROVIDE() added, we will never detect it > at a build time. > > Do you want me to pick up 1/3? > ???
On Sat, Apr 20, 2024 at 9:35 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Sat, 20 Apr 2024 at 14:32, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > On Fri, Apr 19, 2024 at 4:57 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > On Tue, 16 Apr 2024 at 16:40, <patchwork-bot+netdevbpf@kernel.org> wrote: > > > > > > > > Hello: > > > > > > > > This series was applied to bpf/bpf-next.git (master) > > > > by Daniel Borkmann <daniel@iogearbox.net>: > > > > > > > > On Mon, 15 Apr 2024 18:20:42 +0200 you wrote: > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > > > > > Weak external linkage is intended for cases where a symbol reference > > > > > can remain unsatisfied in the final link. Taking the address of such a > > > > > symbol should yield NULL if the reference was not satisfied. > > > > > > > > > > Given that ordinary RIP or PC relative references cannot produce NULL, > > > > > some kind of indirection is always needed in such cases, and in position > > > > > independent code, this results in a GOT entry. In ordinary code, it is > > > > > arch specific but amounts to the same thing. > > > > > > > > > > [...] > > > > > > > > Here is the summary with links: > > > > - [v4,1/3] kallsyms: Avoid weak references for kallsyms symbols > > > > (no matching commit) > > > > - [v4,2/3] vmlinux: Avoid weak reference to notes section > > > > (no matching commit) > > > > - [v4,3/3] btf: Avoid weak external references > > > > https://git.kernel.org/bpf/bpf-next/c/fc5eb4a84e4c > > > > > > > > > > > > > Thanks. > > > > > > Masahiro, could you pick up patches #1 and #2 please? > > > > > > > > > I do not like PROVIDE() because it potentially shifts > > a build error (i.e. link error) into > > a run-time error, which is usually more difficult to debug > > than build error. > > > > If someone references the kallsyms_* symbols > > when CONFIG_KALLSYMS=n, it is likely a mistake. > > In general, it should be reported as a link error. > > > > OK, so the PROVIDE() should be conditional on CONFIG_KALLSYM=y. I can fix that. You may need to take care of the dependency between CONFIG_KALLSYMS and CONFIG_VMCORE_INFO because kernel/vmcore_info.c has references to the kallsyms_* symbols. (I am still not a big fan of PROVIDE() though) > > > With PROVIDE() added, we will never detect it > > at a build time. > > > > Do you want me to pick up 1/3? > > > > ??? I will apply 2/3. It is trivial.
On Sat, 20 Apr 2024 at 15:42, Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Sat, Apr 20, 2024 at 9:35 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Sat, 20 Apr 2024 at 14:32, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > On Fri, Apr 19, 2024 at 4:57 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > > > On Tue, 16 Apr 2024 at 16:40, <patchwork-bot+netdevbpf@kernel.org> wrote: > > > > > > > > > > Hello: > > > > > > > > > > This series was applied to bpf/bpf-next.git (master) > > > > > by Daniel Borkmann <daniel@iogearbox.net>: > > > > > > > > > > On Mon, 15 Apr 2024 18:20:42 +0200 you wrote: > > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > > > > > > > Weak external linkage is intended for cases where a symbol reference > > > > > > can remain unsatisfied in the final link. Taking the address of such a > > > > > > symbol should yield NULL if the reference was not satisfied. > > > > > > > > > > > > Given that ordinary RIP or PC relative references cannot produce NULL, > > > > > > some kind of indirection is always needed in such cases, and in position > > > > > > independent code, this results in a GOT entry. In ordinary code, it is > > > > > > arch specific but amounts to the same thing. > > > > > > > > > > > > [...] > > > > > > > > > > Here is the summary with links: > > > > > - [v4,1/3] kallsyms: Avoid weak references for kallsyms symbols > > > > > (no matching commit) > > > > > - [v4,2/3] vmlinux: Avoid weak reference to notes section > > > > > (no matching commit) > > > > > - [v4,3/3] btf: Avoid weak external references > > > > > https://git.kernel.org/bpf/bpf-next/c/fc5eb4a84e4c > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > Masahiro, could you pick up patches #1 and #2 please? > > > > > > > > > > > > > I do not like PROVIDE() because it potentially shifts > > > a build error (i.e. link error) into > > > a run-time error, which is usually more difficult to debug > > > than build error. > > > > > > If someone references the kallsyms_* symbols > > > when CONFIG_KALLSYMS=n, it is likely a mistake. > > > In general, it should be reported as a link error. > > > > > > > OK, so the PROVIDE() should be conditional on CONFIG_KALLSYM=y. I can fix that. > > > You may need to take care of the dependency > between CONFIG_KALLSYMS and CONFIG_VMCORE_INFO > because kernel/vmcore_info.c has references > to the kallsyms_* symbols. > > (I am still not a big fan of PROVIDE() though) > OK, how about we use weak definitions (as opposed to weak references) in kernel/kallsyms.c, which will get superseded by the actual ones in the second linker pass. The only difference is that we will use some space in the binary for the weak definitions that are never used in the final build.
On Sat, 20 Apr 2024 at 15:56, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Sat, 20 Apr 2024 at 15:42, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > On Sat, Apr 20, 2024 at 9:35 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > On Sat, 20 Apr 2024 at 14:32, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > > > On Fri, Apr 19, 2024 at 4:57 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > > > > > On Tue, 16 Apr 2024 at 16:40, <patchwork-bot+netdevbpf@kernel.org> wrote: > > > > > > > > > > > > Hello: > > > > > > > > > > > > This series was applied to bpf/bpf-next.git (master) > > > > > > by Daniel Borkmann <daniel@iogearbox.net>: > > > > > > > > > > > > On Mon, 15 Apr 2024 18:20:42 +0200 you wrote: > > > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > > > > > > > > > Weak external linkage is intended for cases where a symbol reference > > > > > > > can remain unsatisfied in the final link. Taking the address of such a > > > > > > > symbol should yield NULL if the reference was not satisfied. > > > > > > > > > > > > > > Given that ordinary RIP or PC relative references cannot produce NULL, > > > > > > > some kind of indirection is always needed in such cases, and in position > > > > > > > independent code, this results in a GOT entry. In ordinary code, it is > > > > > > > arch specific but amounts to the same thing. > > > > > > > > > > > > > > [...] > > > > > > > > > > > > Here is the summary with links: > > > > > > - [v4,1/3] kallsyms: Avoid weak references for kallsyms symbols > > > > > > (no matching commit) > > > > > > - [v4,2/3] vmlinux: Avoid weak reference to notes section > > > > > > (no matching commit) > > > > > > - [v4,3/3] btf: Avoid weak external references > > > > > > https://git.kernel.org/bpf/bpf-next/c/fc5eb4a84e4c > > > > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > Masahiro, could you pick up patches #1 and #2 please? > > > > > > > > > > > > > > > > > I do not like PROVIDE() because it potentially shifts > > > > a build error (i.e. link error) into > > > > a run-time error, which is usually more difficult to debug > > > > than build error. > > > > > > > > If someone references the kallsyms_* symbols > > > > when CONFIG_KALLSYMS=n, it is likely a mistake. > > > > In general, it should be reported as a link error. > > > > > > > > > > OK, so the PROVIDE() should be conditional on CONFIG_KALLSYM=y. I can fix that. > > > > > > You may need to take care of the dependency > > between CONFIG_KALLSYMS and CONFIG_VMCORE_INFO > > because kernel/vmcore_info.c has references > > to the kallsyms_* symbols. > > > > (I am still not a big fan of PROVIDE() though) > > > > > OK, how about we use weak definitions (as opposed to weak references) > in kernel/kallsyms.c, which will get superseded by the actual ones in > the second linker pass. > > The only difference is that we will use some space in the binary for > the weak definitions that are never used in the final build. Btw those references in kernel/vmcore_info.c are guarded by #ifdef CONFIG_KALLSYMS=y too.
On Sat, Apr 20, 2024 at 11:00 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Sat, 20 Apr 2024 at 15:56, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Sat, 20 Apr 2024 at 15:42, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > On Sat, Apr 20, 2024 at 9:35 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > > > On Sat, 20 Apr 2024 at 14:32, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > > > > > On Fri, Apr 19, 2024 at 4:57 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > > > > > > > On Tue, 16 Apr 2024 at 16:40, <patchwork-bot+netdevbpf@kernel.org> wrote: > > > > > > > > > > > > > > Hello: > > > > > > > > > > > > > > This series was applied to bpf/bpf-next.git (master) > > > > > > > by Daniel Borkmann <daniel@iogearbox.net>: > > > > > > > > > > > > > > On Mon, 15 Apr 2024 18:20:42 +0200 you wrote: > > > > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > > > > > > > > > > > Weak external linkage is intended for cases where a symbol reference > > > > > > > > can remain unsatisfied in the final link. Taking the address of such a > > > > > > > > symbol should yield NULL if the reference was not satisfied. > > > > > > > > > > > > > > > > Given that ordinary RIP or PC relative references cannot produce NULL, > > > > > > > > some kind of indirection is always needed in such cases, and in position > > > > > > > > independent code, this results in a GOT entry. In ordinary code, it is > > > > > > > > arch specific but amounts to the same thing. > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > Here is the summary with links: > > > > > > > - [v4,1/3] kallsyms: Avoid weak references for kallsyms symbols > > > > > > > (no matching commit) > > > > > > > - [v4,2/3] vmlinux: Avoid weak reference to notes section > > > > > > > (no matching commit) > > > > > > > - [v4,3/3] btf: Avoid weak external references > > > > > > > https://git.kernel.org/bpf/bpf-next/c/fc5eb4a84e4c > > > > > > > > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > Masahiro, could you pick up patches #1 and #2 please? > > > > > > > > > > > > > > > > > > > > > I do not like PROVIDE() because it potentially shifts > > > > > a build error (i.e. link error) into > > > > > a run-time error, which is usually more difficult to debug > > > > > than build error. > > > > > > > > > > If someone references the kallsyms_* symbols > > > > > when CONFIG_KALLSYMS=n, it is likely a mistake. > > > > > In general, it should be reported as a link error. > > > > > > > > > > > > > OK, so the PROVIDE() should be conditional on CONFIG_KALLSYM=y. I can fix that. > > > > > > > > > You may need to take care of the dependency > > > between CONFIG_KALLSYMS and CONFIG_VMCORE_INFO > > > because kernel/vmcore_info.c has references > > > to the kallsyms_* symbols. > > > > > > (I am still not a big fan of PROVIDE() though) > > > > > > > > > OK, how about we use weak definitions (as opposed to weak references) > > in kernel/kallsyms.c, which will get superseded by the actual ones in > > the second linker pass. > > > > The only difference is that we will use some space in the binary for > > the weak definitions that are never used in the final build. I am fine if that fixes the issue. "git grep __weak" shows a bunch of weak definitions. > Btw those references in kernel/vmcore_info.c are guarded by #ifdef > CONFIG_KALLSYMS=y too. Ah, OK. Then, this is not an issue.
From: Ard Biesheuvel <ardb@kernel.org> Weak external linkage is intended for cases where a symbol reference can remain unsatisfied in the final link. Taking the address of such a symbol should yield NULL if the reference was not satisfied. Given that ordinary RIP or PC relative references cannot produce NULL, some kind of indirection is always needed in such cases, and in position independent code, this results in a GOT entry. In ordinary code, it is arch specific but amounts to the same thing. While unavoidable in some cases, weak references are currently also used to declare symbols that are always defined in the final link, but not in the first linker pass. This means we end up with worse codegen for no good reason. So let's clean this up, by providing preliminary definitions that are only used as a fallback. Changes since v3: - drop unnecessary preliminary definitions for BTF start/stop - add Jiri's ack Changes since v2: - fix build issue in patch #3 reported by Jiri - add Arnd's acks Changes since v1: - update second occurrence of BTF start/end markers - drop NULL check of __start_BTF[] which is no longer meaningful - avoid the preliminary BTF symbols if CONFIG_DEBUG_INFO_BTF is not set - add Andrii's ack to patch #3 - patches #1 and #2 unchanged Cc: Masahiro Yamada <masahiroy@kernel.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Martin KaFai Lau <martin.lau@linux.dev> Cc: linux-arch@vger.kernel.org Cc: linux-kbuild@vger.kernel.org Cc: bpf@vger.kernel.org Cc: Andrii Nakryiko <andrii@kernel.org> Cc: Jiri Olsa <olsajiri@gmail.com> Ard Biesheuvel (3): kallsyms: Avoid weak references for kallsyms symbols vmlinux: Avoid weak reference to notes section btf: Avoid weak external references include/asm-generic/vmlinux.lds.h | 19 +++++++++++++ kernel/bpf/btf.c | 7 +++-- kernel/bpf/sysfs_btf.c | 6 ++-- kernel/kallsyms.c | 6 ---- kernel/kallsyms_internal.h | 30 ++++++++------------ kernel/ksysfs.c | 4 +-- lib/buildid.c | 4 +-- 7 files changed, 43 insertions(+), 33 deletions(-)