[linux-6.6.y]kbuild: Avoid weak external linkage where possible#1347
Conversation
commit 951bcae upstream. kallsyms is a directory of all the symbols in the vmlinux binary, and so creating it is somewhat of a chicken-and-egg problem, as its non-zero size affects the layout of the binary, and therefore the values of the symbols. For this reason, the kernel is linked more than once, and the first pass does not include any kallsyms data at all. For the linker to accept this, the symbol declarations describing the kallsyms metadata are emitted as having weak linkage, so they can remain unsatisfied. During the subsequent passes, the weak references are satisfied by the kallsyms metadata that was constructed based on information gathered from the preceding passes. Weak references lead to somewhat worse codegen, because taking their address may need to produce NULL (if the reference was unsatisfied), and this is not usually supported by RIP or PC relative symbol references. Given that these references are ultimately always satisfied in the final link, let's drop the weak annotation, and instead, provide fallback definitions in the linker script that are only emitted if an unsatisfied reference exists. While at it, drop the FRV specific annotation that these symbols reside in .rodata - FRV is long gone. Tested-by: Nick Desaulniers <ndesaulniers@google.com> # Boot Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Reviewed-by: Kees Cook <keescook@chromium.org> Acked-by: Arnd Bergmann <arnd@arndb.de> Link: https://lkml.kernel.org/r/20230504174320.3930345-1-ardb%40kernel.org Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
commit 377d909 upstream. Weak references are references that are permitted to remain unsatisfied in the final link. This means they cannot be implemented using place relative relocations, resulting in GOT entries when using position independent code generation. The notes section should always exist, so the weak annotations can be omitted. Acked-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
commit fc5eb4a upstream. If the BTF code is enabled in the build configuration, the start/stop BTF markers are guaranteed to exist. Only when CONFIG_DEBUG_INFO_BTF=n, the references in btf_parse_vmlinux() will remain unsatisfied, relying on the weak linkage of the external references to avoid breaking the build. Avoid GOT based relocations to these markers in the final executable by dropping the weak attribute and instead, make btf_parse_vmlinux() return ERR_PTR(-ENOENT) directly if CONFIG_DEBUG_INFO_BTF is not enabled to begin with. The compiler will drop any subsequent references to __start_BTF and __stop_BTF in that case, allowing the link to succeed. Note that Clang will notice that taking the address of __start_BTF can no longer yield NULL, so testing for that condition becomes unnecessary. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Signed-off-by: Juxin Gao <gaojuxin@loongson.cn> Acked-by: Andrii Nakryiko <andrii@kernel.org> Acked-by: Arnd Bergmann <arnd@arndb.de> Acked-by: Jiri Olsa <jolsa@kernel.org> Link: https://lore.kernel.org/bpf/20240415162041.2491523-8-ardb+git@google.com Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
Reviewer's GuideBackports an upstream change to avoid weak external linkage by replacing __weak extern declarations with strong externs and providing preliminary symbol definitions in the linker script, plus minor behavioral adjustments where weak linkage was previously relied upon. Sequence diagram for reading vmlinux BTF via sysfs using strong __start_BTF/__stop_BTFsequenceDiagram
actor User
participant Sysfs as sysfs_btf_vmlinux
participant Kernel as kernel_btf
User->>Sysfs: open /sys/kernel/btf/vmlinux
Sysfs->>Kernel: btf_vmlinux_init()
Kernel->>Kernel: size = __stop_BTF - __start_BTF
Kernel->>Kernel: if size == 0 return 0
Kernel->>Kernel: create kobject and bin_attribute
Kernel-->>Sysfs: initialization result
User->>Sysfs: read()
Sysfs->>Kernel: btf_vmlinux_read(file, kobj, attr, buf, off, count)
Kernel->>Kernel: compute offset in [__start_BTF, __stop_BTF)
Kernel-->>Sysfs: copy BTF bytes into buf
Sysfs-->>User: return BTF data
Flow diagram for btf_parse_vmlinux with CONFIG_DEBUG_INFO_BTF gatingflowchart TD
start([Start btf_parse_vmlinux])
checkConfig{CONFIG_DEBUG_INFO_BTF enabled?}
returnNoEnt["Return ERR_PTR(-ENOENT)"]
allocEnv["kzalloc env"]
allocFail{env is NULL?}
returnNoMem["Return ERR_PTR(-ENOMEM)"]
parseBTF["Parse vmlinux BTF using __start_BTF and __stop_BTF"]
done([Return btf or error])
start --> checkConfig
checkConfig -->|No| returnNoEnt
checkConfig -->|Yes| allocEnv
allocEnv --> allocFail
allocFail -->|Yes| returnNoMem
allocFail -->|No| parseBTF
parseBTF --> done
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Hi @AaronDot. Thanks for your PR. I'm waiting for a deepin-community member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider centralizing the
__start_notes/__stop_notesdeclarations in a common header instead of repeatingexterndeclarations in both ksysfs.c and buildid.c to keep these linker-provided symbols consistent across users. - The
PRELIMINARY_SYMBOL_DEFINITIONSmacro is injected intoRO_DATA(align); if any of these symbols ever need a different placement or alignment from generic rodata, it might be cleaner to hook them into a dedicated linker script block rather than piggybacking on the rodata section.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider centralizing the `__start_notes`/`__stop_notes` declarations in a common header instead of repeating `extern` declarations in both ksysfs.c and buildid.c to keep these linker-provided symbols consistent across users.
- The `PRELIMINARY_SYMBOL_DEFINITIONS` macro is injected into `RO_DATA(align)`; if any of these symbols ever need a different placement or alignment from generic rodata, it might be cleaner to hook them into a dedicated linker script block rather than piggybacking on the rodata section.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR backports upstream changes to eliminate weak external linkage for linker-provided kernel symbols, replacing it with a more robust approach using preliminary symbol definitions. The motivation is to avoid relying on weak external linkage, which requires a Global Offset Table (GOT) when used in position-independent code, improving both code efficiency and reliability.
Key Changes
- Introduces
PRELIMINARY_SYMBOL_DEFINITIONSmacro in the linker script that usesPROVIDE()to define kallsyms symbols for the first link pass - Removes
__weakannotations from kallsyms, BTF, and .notes section symbol declarations across multiple files - Removes redundant runtime BUG_ON checks in kallsyms that are no longer needed since symbols are guaranteed to exist
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| include/asm-generic/vmlinux.lds.h | Adds PRELIMINARY_SYMBOL_DEFINITIONS macro with PROVIDE() directives for kallsyms symbols and integrates it into RO_DATA section |
| kernel/kallsyms_internal.h | Removes __weak annotations from all kallsyms extern declarations and updates comments to explain the new PROVIDE() approach |
| kernel/kallsyms.c | Removes BUG_ON checks for kallsyms_addresses and kallsyms_offsets, now guaranteed to exist by linker script |
| kernel/bpf/btf.c | Removes __weak from __start_BTF/__stop_BTF and adds CONFIG_DEBUG_INFO_BTF guard to btf_parse_vmlinux() |
| kernel/bpf/sysfs_btf.c | Removes __weak from __start_BTF/__stop_BTF and simplifies null check to only test size (symbols guaranteed when CONFIG_DEBUG_INFO_BTF is set) |
| lib/buildid.c | Removes __weak annotations from __start_notes/__stop_notes declarations |
| kernel/ksysfs.c | Removes __weak annotations from __start_notes/__stop_notes declarations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/approve |
|
/ok-to-test |
deepin pr auto review这是一个关于Linux内核链接脚本和符号处理的改进补丁。让我从多个角度分析这个变更:
建议和改进意见:
/*
* PRELIMINARY_SYMBOL_DEFINITIONS - 为kallsyms相关符号提供预定义值
*
* 这些符号在第一次链接时可能还不存在,但在最终链接时保证存在。
* 使用PROVIDE()提供预定义值可以避免使用弱外部符号,特别是在
* 位置无关代码中可以避免对GOT的依赖。
*/
#ifdef CONFIG_DEBUG_INFO_BTF
if (!__start_BTF || bin_attr_btf_vmlinux.size == 0) {
pr_warn("BTF information not available\n");
return -EINVAL;
}
#endif
/* 使用更具体的类型定义 */
extern const unsigned long kallsyms_addresses[] __attribute__((aligned(sizeof(unsigned long))));
/* 添加编译时检查确保符号对齐 */
#ifdef CONFIG_DEBUG_INFO_BTF
_Static_assert(__alignof__(__start_BTF) >= 8, "BTF symbols must be 8-byte aligned");
#endif
/* 考虑使用likely/unlikely提示优化热点路径 */
if (likely(bin_attr_btf_vmlinux.size > 0)) {
/* 处理正常情况 */
}这些变更总体上是积极的,提高了代码的可靠性、性能和安全性。建议在后续版本中考虑上述改进建议,进一步提升代码质量。 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Avenger-285714, opsiff The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[ Upstream commit 1f029b4 ] When PA Create Sync is enabled, advertising resumes unexpectedly. Therefore, it's necessary to check whether advertising is currently active before attempting to pause it. < HCI Command: LE Add Device To... (0x08|0x0011) plen 7 deepin-community#1345 [hci0] 48.306205 Address type: Random (0x01) Address: 4F:84:84:5F:88:17 (Resolvable) Identity type: Random (0x01) Identity: FC:5B:8C:F7:5D:FB (Static) < HCI Command: LE Set Address Re.. (0x08|0x002d) plen 1 deepin-community#1347 [hci0] 48.308023 Address resolution: Enabled (0x01) ... < HCI Command: LE Set Extended A.. (0x08|0x0039) plen 6 deepin-community#1349 [hci0] 48.309650 Extended advertising: Enabled (0x01) Number of sets: 1 (0x01) Entry 0 Handle: 0x01 Duration: 0 ms (0x00) Max ext adv events: 0 ... < HCI Command: LE Periodic Adve.. (0x08|0x0044) plen 14 deepin-community#1355 [hci0] 48.314575 Options: 0x0000 Use advertising SID, Advertiser Address Type and address Reporting initially enabled SID: 0x02 Adv address type: Random (0x01) Adv address: 4F:84:84:5F:88:17 (Resolvable) Identity type: Random (0x01) Identity: FC:5B:8C:F7:5D:FB (Static) Skip: 0x0000 Sync timeout: 20000 msec (0x07d0) Sync CTE type: 0x0000 Fixes: ad383c2 ("Bluetooth: hci_sync: Enable advertising when LL privacy is enabled") Signed-off-by: Yang Li <yang.li@amlogic.com> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> Signed-off-by: Sasha Levin <sashal@kernel.org> (cherry picked from commit 529281206f1167d875a8dde4a9eb85298d40fcf4)
This patchset is backported from upstream to avoid weak external linkage where possible.
Summary by Sourcery
Replace weak external linkage for linker-provided kernel symbols with stronger definitions and corresponding preliminary linker script symbols, and adjust related BTF and notes handling accordingly.
Enhancements: