* [PATCH v6 01/20] Revert "modpost: skip ELF local symbols during section mismatch check"
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-22 17:42 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 02/20] modpost: fix section mismatch message for R_ARM_ABS32 Masahiro Yamada
` (19 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
This reverts commit a4d26f1a0958bb1c2b60c6f1e67c6f5d43e2647b.
The variable 'fromsym' never starts with ".L" since commit 87e5b1e8f257
("module: Sync code of is_arm_mapping_symbol()").
In other words, Pattern 6 is now dead code.
Previously, the .LANCHOR1 hid the symbols listed in Pattern 2, but
a4d26f1a0958 was a cheesy workaround.
87e5b1e8f257 addressed the issue in a much better way.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 0d2c2aff2c03..71de14544432 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1034,14 +1034,6 @@ static const struct sectioncheck *section_mismatch(
* fromsec = text section
* refsymname = *.constprop.*
*
- * Pattern 6:
- * Hide section mismatch warnings for ELF local symbols. The goal
- * is to eliminate false positive modpost warnings caused by
- * compiler-generated ELF local symbol names such as ".LANCHOR1".
- * Autogenerated symbol names bypass modpost's "Pattern 2"
- * whitelisting, which relies on pattern-matching against symbol
- * names to work. (One situation where gcc can autogenerate ELF
- * local symbols is when "-fsection-anchors" is used.)
**/
static int secref_whitelist(const struct sectioncheck *mismatch,
const char *fromsec, const char *fromsym,
@@ -1092,10 +1084,6 @@ static int secref_whitelist(const struct sectioncheck *mismatch,
match(fromsym, optim_symbols))
return 0;
- /* Check for pattern 6 */
- if (strstarts(fromsym, ".L"))
- return 0;
-
return 1;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 01/20] Revert "modpost: skip ELF local symbols during section mismatch check"
2023-05-21 16:04 ` [PATCH v6 01/20] Revert "modpost: skip ELF local symbols during section mismatch check" Masahiro Yamada
@ 2023-05-22 17:42 ` Nick Desaulniers
0 siblings, 0 replies; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-22 17:42 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> This reverts commit a4d26f1a0958bb1c2b60c6f1e67c6f5d43e2647b.
a4d26f1a0958 mentions -fsection-anchors which I see used in:
arch/arc/Makefile
41:cflags-y += -fsection-anchors
Though based on the below, it looks like is_mapping_symbol() will
catch this case.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>
> The variable 'fromsym' never starts with ".L" since commit 87e5b1e8f257
> ("module: Sync code of is_arm_mapping_symbol()").
>
> In other words, Pattern 6 is now dead code.
>
> Previously, the .LANCHOR1 hid the symbols listed in Pattern 2, but
> a4d26f1a0958 was a cheesy workaround.
>
> 87e5b1e8f257 addressed the issue in a much better way.
is_arm_mapping_symbol is gone from scripts/mod/modpost.c. It was moved
then renamed to is_mapping_symbol. Maybe not relevant for the commit
message, but maybe helpful to other reviewers.
commit 987d2e0aaa55 ("module: Move is_arm_mapping_symbol() to module_symbol.h")
commit 0a3bf86092c3 ("module: Ignore L0 and rename is_arm_mapping_symbol()")
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> scripts/mod/modpost.c | 12 ------------
> 1 file changed, 12 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 0d2c2aff2c03..71de14544432 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1034,14 +1034,6 @@ static const struct sectioncheck *section_mismatch(
> * fromsec = text section
> * refsymname = *.constprop.*
> *
> - * Pattern 6:
> - * Hide section mismatch warnings for ELF local symbols. The goal
> - * is to eliminate false positive modpost warnings caused by
> - * compiler-generated ELF local symbol names such as ".LANCHOR1".
> - * Autogenerated symbol names bypass modpost's "Pattern 2"
> - * whitelisting, which relies on pattern-matching against symbol
> - * names to work. (One situation where gcc can autogenerate ELF
> - * local symbols is when "-fsection-anchors" is used.)
> **/
> static int secref_whitelist(const struct sectioncheck *mismatch,
> const char *fromsec, const char *fromsym,
> @@ -1092,10 +1084,6 @@ static int secref_whitelist(const struct sectioncheck *mismatch,
> match(fromsym, optim_symbols))
> return 0;
>
> - /* Check for pattern 6 */
> - if (strstarts(fromsym, ".L"))
> - return 0;
> -
> return 1;
> }
>
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 02/20] modpost: fix section mismatch message for R_ARM_ABS32
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
2023-05-21 16:04 ` [PATCH v6 01/20] Revert "modpost: skip ELF local symbols during section mismatch check" Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-22 17:56 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 03/20] modpost: detect section mismatch for R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS Masahiro Yamada
` (18 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
addend_arm_rel() processes R_ARM_ABS32 in a wrong way.
Here, simple test code.
[test code 1]
#include <linux/init.h>
int __initdata foo;
int get_foo(int x) { return foo; }
If you compile it with ARM versatile_defconfig, modpost will show the
symbol name, (unknown).
WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> (unknown) (section: .init.data)
If you compile it for other architectures, modpost will show the correct
symbol name.
WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> foo (section: .init.data)
For R_ARM_ABS32, addend_arm_rel() sets r->r_addend to a wrong value.
I just mimicked the code in arch/arm/kernel/module.c.
However, there is more difficulty for ARM.
Here, test code.
[test code 2]
#include <linux/init.h>
int __initdata foo;
int get_foo(int x) { return foo; }
int __initdata bar;
int get_bar(int x) { return bar; }
With this commit applied, modpost will show the following messages
for ARM versatile_defconfig:
WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> foo (section: .init.data)
WARNING: modpost: vmlinux.o: section mismatch in reference: get_bar (section: .text) -> foo (section: .init.data)
The reference from 'get_bar' to 'foo' seems wrong.
I have no solution for this because it is true in assembly level.
In the following output, relocation at 0x1c is no longer associated
with 'bar'. The two relocation entries point to the same symbol, and
the offset to 'bar' is encoded in the instruction 'r0, [r3, #4]'.
Disassembly of section .text:
00000000 <get_foo>:
0: e59f3004 ldr r3, [pc, #4] @ c <get_foo+0xc>
4: e5930000 ldr r0, [r3]
8: e12fff1e bx lr
c: 00000000 .word 0x00000000
00000010 <get_bar>:
10: e59f3004 ldr r3, [pc, #4] @ 1c <get_bar+0xc>
14: e5930004 ldr r0, [r3, #4]
18: e12fff1e bx lr
1c: 00000000 .word 0x00000000
Relocation section '.rel.text' at offset 0x244 contains 2 entries:
Offset Info Type Sym.Value Sym. Name
0000000c 00000c02 R_ARM_ABS32 00000000 .init.data
0000001c 00000c02 R_ARM_ABS32 00000000 .init.data
When find_elf_symbol() gets into a situation where relsym->st_name is
zero, there is no guarantee to get the symbol name as written in C.
I am keeping the current logic because it is useful in many architectures,
but the symbol name is not always correct depending on the optimization
of the relocation. I left some comments in find_tosym().
Fixes: 56a974fa2d59 ("kbuild: make better section mismatch reports on arm")
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
Changes in v6:
- More detailed commit log
scripts/mod/modpost.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 71de14544432..34fbbd85bfde 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1124,6 +1124,10 @@ static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
if (relsym->st_name != 0)
return relsym;
+ /*
+ * Strive to find a better symbol name, but the resulting name does not
+ * always match the symbol referenced in the original code.
+ */
relsym_secindex = get_secindex(elf, relsym);
for (sym = elf->symtab_start; sym < elf->symtab_stop; sym++) {
if (get_secindex(elf, sym) != relsym_secindex)
@@ -1306,12 +1310,12 @@ static int addend_386_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
static int addend_arm_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
{
unsigned int r_typ = ELF_R_TYPE(r->r_info);
+ Elf_Sym *sym = elf->symtab_start + ELF_R_SYM(r->r_info);
+ unsigned int inst = TO_NATIVE(*reloc_location(elf, sechdr, r));
switch (r_typ) {
case R_ARM_ABS32:
- /* From ARM ABI: (S + A) | T */
- r->r_addend = (int)(long)
- (elf->symtab_start + ELF_R_SYM(r->r_info));
+ r->r_addend = inst + sym->st_value;
break;
case R_ARM_PC24:
case R_ARM_CALL:
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 02/20] modpost: fix section mismatch message for R_ARM_ABS32
2023-05-21 16:04 ` [PATCH v6 02/20] modpost: fix section mismatch message for R_ARM_ABS32 Masahiro Yamada
@ 2023-05-22 17:56 ` Nick Desaulniers
2023-05-22 21:35 ` Ard Biesheuvel
0 siblings, 1 reply; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-22 17:56 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier,
Linux ARM, Ard Biesheuvel, Fangrui Song
+ linux-arm-kernel and some folks who might know another idea.
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> addend_arm_rel() processes R_ARM_ABS32 in a wrong way.
>
> Here, simple test code.
>
> [test code 1]
>
> #include <linux/init.h>
>
> int __initdata foo;
> int get_foo(int x) { return foo; }
>
> If you compile it with ARM versatile_defconfig, modpost will show the
> symbol name, (unknown).
>
> WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> (unknown) (section: .init.data)
>
> If you compile it for other architectures, modpost will show the correct
> symbol name.
>
> WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> foo (section: .init.data)
>
> For R_ARM_ABS32, addend_arm_rel() sets r->r_addend to a wrong value.
>
> I just mimicked the code in arch/arm/kernel/module.c.
>
> However, there is more difficulty for ARM.
>
> Here, test code.
>
> [test code 2]
>
> #include <linux/init.h>
>
> int __initdata foo;
> int get_foo(int x) { return foo; }
>
> int __initdata bar;
> int get_bar(int x) { return bar; }
>
> With this commit applied, modpost will show the following messages
> for ARM versatile_defconfig:
>
> WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> foo (section: .init.data)
> WARNING: modpost: vmlinux.o: section mismatch in reference: get_bar (section: .text) -> foo (section: .init.data)
>
> The reference from 'get_bar' to 'foo' seems wrong.
>
> I have no solution for this because it is true in assembly level.
>
> In the following output, relocation at 0x1c is no longer associated
> with 'bar'. The two relocation entries point to the same symbol, and
> the offset to 'bar' is encoded in the instruction 'r0, [r3, #4]'.
>
> Disassembly of section .text:
>
> 00000000 <get_foo>:
> 0: e59f3004 ldr r3, [pc, #4] @ c <get_foo+0xc>
> 4: e5930000 ldr r0, [r3]
> 8: e12fff1e bx lr
> c: 00000000 .word 0x00000000
>
> 00000010 <get_bar>:
> 10: e59f3004 ldr r3, [pc, #4] @ 1c <get_bar+0xc>
> 14: e5930004 ldr r0, [r3, #4]
> 18: e12fff1e bx lr
> 1c: 00000000 .word 0x00000000
>
> Relocation section '.rel.text' at offset 0x244 contains 2 entries:
> Offset Info Type Sym.Value Sym. Name
> 0000000c 00000c02 R_ARM_ABS32 00000000 .init.data
> 0000001c 00000c02 R_ARM_ABS32 00000000 .init.data
>
> When find_elf_symbol() gets into a situation where relsym->st_name is
> zero, there is no guarantee to get the symbol name as written in C.
>
> I am keeping the current logic because it is useful in many architectures,
> but the symbol name is not always correct depending on the optimization
> of the relocation. I left some comments in find_tosym().
>
> Fixes: 56a974fa2d59 ("kbuild: make better section mismatch reports on arm")
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> Changes in v6:
> - More detailed commit log
>
> scripts/mod/modpost.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 71de14544432..34fbbd85bfde 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1124,6 +1124,10 @@ static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
> if (relsym->st_name != 0)
> return relsym;
>
> + /*
> + * Strive to find a better symbol name, but the resulting name does not
> + * always match the symbol referenced in the original code.
> + */
> relsym_secindex = get_secindex(elf, relsym);
> for (sym = elf->symtab_start; sym < elf->symtab_stop; sym++) {
> if (get_secindex(elf, sym) != relsym_secindex)
> @@ -1306,12 +1310,12 @@ static int addend_386_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
> static int addend_arm_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
> {
> unsigned int r_typ = ELF_R_TYPE(r->r_info);
> + Elf_Sym *sym = elf->symtab_start + ELF_R_SYM(r->r_info);
> + unsigned int inst = TO_NATIVE(*reloc_location(elf, sechdr, r));
>
> switch (r_typ) {
> case R_ARM_ABS32:
> - /* From ARM ABI: (S + A) | T */
> - r->r_addend = (int)(long)
> - (elf->symtab_start + ELF_R_SYM(r->r_info));
> + r->r_addend = inst + sym->st_value;
> break;
> case R_ARM_PC24:
> case R_ARM_CALL:
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 02/20] modpost: fix section mismatch message for R_ARM_ABS32
2023-05-22 17:56 ` Nick Desaulniers
@ 2023-05-22 21:35 ` Ard Biesheuvel
2023-05-23 5:07 ` Masahiro Yamada
0 siblings, 1 reply; 51+ messages in thread
From: Ard Biesheuvel @ 2023-05-22 21:35 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Masahiro Yamada, linux-kbuild, linux-kernel, Nathan Chancellor,
Nicolas Schier, Linux ARM, Fangrui Song
On Mon, 22 May 2023 at 19:56, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> + linux-arm-kernel and some folks who might know another idea.
>
> On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > addend_arm_rel() processes R_ARM_ABS32 in a wrong way.
> >
> > Here, simple test code.
> >
> > [test code 1]
> >
> > #include <linux/init.h>
> >
> > int __initdata foo;
> > int get_foo(int x) { return foo; }
> >
> > If you compile it with ARM versatile_defconfig, modpost will show the
> > symbol name, (unknown).
> >
> > WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> (unknown) (section: .init.data)
> >
> > If you compile it for other architectures, modpost will show the correct
> > symbol name.
> >
> > WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> foo (section: .init.data)
> >
> > For R_ARM_ABS32, addend_arm_rel() sets r->r_addend to a wrong value.
> >
> > I just mimicked the code in arch/arm/kernel/module.c.
> >
> > However, there is more difficulty for ARM.
> >
> > Here, test code.
> >
> > [test code 2]
> >
> > #include <linux/init.h>
> >
> > int __initdata foo;
> > int get_foo(int x) { return foo; }
> >
> > int __initdata bar;
> > int get_bar(int x) { return bar; }
> >
> > With this commit applied, modpost will show the following messages
> > for ARM versatile_defconfig:
> >
> > WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> foo (section: .init.data)
> > WARNING: modpost: vmlinux.o: section mismatch in reference: get_bar (section: .text) -> foo (section: .init.data)
> >
> > The reference from 'get_bar' to 'foo' seems wrong.
> >
> > I have no solution for this because it is true in assembly level.
> >
> > In the following output, relocation at 0x1c is no longer associated
> > with 'bar'. The two relocation entries point to the same symbol, and
> > the offset to 'bar' is encoded in the instruction 'r0, [r3, #4]'.
> >
These are section relative relocations - this is unusual but not
incorrect. Normally, you only see this if the symbols in question have
static linkage.
It does mean that the symbol is not preemptible, which is what makes
this somewhat surprising.
Generally, you cannot resolve a relocation to a symbol without taking
the addend into account, so looking up the address of .init.data in
the symbol table is not quite the right approach here. If anything,
the symbol should be reported as [.init.data+0x4] in the second case.
> > Disassembly of section .text:
> >
> > 00000000 <get_foo>:
> > 0: e59f3004 ldr r3, [pc, #4] @ c <get_foo+0xc>
> > 4: e5930000 ldr r0, [r3]
> > 8: e12fff1e bx lr
> > c: 00000000 .word 0x00000000
> >
> > 00000010 <get_bar>:
> > 10: e59f3004 ldr r3, [pc, #4] @ 1c <get_bar+0xc>
> > 14: e5930004 ldr r0, [r3, #4]
> > 18: e12fff1e bx lr
> > 1c: 00000000 .word 0x00000000
> >
> > Relocation section '.rel.text' at offset 0x244 contains 2 entries:
> > Offset Info Type Sym.Value Sym. Name
> > 0000000c 00000c02 R_ARM_ABS32 00000000 .init.data
> > 0000001c 00000c02 R_ARM_ABS32 00000000 .init.data
> >
> > When find_elf_symbol() gets into a situation where relsym->st_name is
> > zero, there is no guarantee to get the symbol name as written in C.
> >
> > I am keeping the current logic because it is useful in many architectures,
> > but the symbol name is not always correct depending on the optimization
> > of the relocation. I left some comments in find_tosym().
> >
> > Fixes: 56a974fa2d59 ("kbuild: make better section mismatch reports on arm")
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> > Changes in v6:
> > - More detailed commit log
> >
> > scripts/mod/modpost.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 71de14544432..34fbbd85bfde 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1124,6 +1124,10 @@ static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
> > if (relsym->st_name != 0)
> > return relsym;
> >
> > + /*
> > + * Strive to find a better symbol name, but the resulting name does not
> > + * always match the symbol referenced in the original code.
> > + */
> > relsym_secindex = get_secindex(elf, relsym);
> > for (sym = elf->symtab_start; sym < elf->symtab_stop; sym++) {
> > if (get_secindex(elf, sym) != relsym_secindex)
> > @@ -1306,12 +1310,12 @@ static int addend_386_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
> > static int addend_arm_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
> > {
> > unsigned int r_typ = ELF_R_TYPE(r->r_info);
> > + Elf_Sym *sym = elf->symtab_start + ELF_R_SYM(r->r_info);
> > + unsigned int inst = TO_NATIVE(*reloc_location(elf, sechdr, r));
> >
> > switch (r_typ) {
> > case R_ARM_ABS32:
> > - /* From ARM ABI: (S + A) | T */
> > - r->r_addend = (int)(long)
> > - (elf->symtab_start + ELF_R_SYM(r->r_info));
> > + r->r_addend = inst + sym->st_value;
> > break;
> > case R_ARM_PC24:
> > case R_ARM_CALL:
> > --
> > 2.39.2
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 02/20] modpost: fix section mismatch message for R_ARM_ABS32
2023-05-22 21:35 ` Ard Biesheuvel
@ 2023-05-23 5:07 ` Masahiro Yamada
2023-05-23 7:13 ` Ard Biesheuvel
0 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-23 5:07 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Nick Desaulniers, linux-kbuild, linux-kernel, Nathan Chancellor,
Nicolas Schier, Linux ARM, Fangrui Song
On Tue, May 23, 2023 at 6:36 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 22 May 2023 at 19:56, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > + linux-arm-kernel and some folks who might know another idea.
> >
> > On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > addend_arm_rel() processes R_ARM_ABS32 in a wrong way.
> > >
> > > Here, simple test code.
> > >
> > > [test code 1]
> > >
> > > #include <linux/init.h>
> > >
> > > int __initdata foo;
> > > int get_foo(int x) { return foo; }
> > >
> > > If you compile it with ARM versatile_defconfig, modpost will show the
> > > symbol name, (unknown).
> > >
> > > WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> (unknown) (section: .init.data)
> > >
> > > If you compile it for other architectures, modpost will show the correct
> > > symbol name.
> > >
> > > WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> foo (section: .init.data)
> > >
> > > For R_ARM_ABS32, addend_arm_rel() sets r->r_addend to a wrong value.
> > >
> > > I just mimicked the code in arch/arm/kernel/module.c.
> > >
> > > However, there is more difficulty for ARM.
> > >
> > > Here, test code.
> > >
> > > [test code 2]
> > >
> > > #include <linux/init.h>
> > >
> > > int __initdata foo;
> > > int get_foo(int x) { return foo; }
> > >
> > > int __initdata bar;
> > > int get_bar(int x) { return bar; }
> > >
> > > With this commit applied, modpost will show the following messages
> > > for ARM versatile_defconfig:
> > >
> > > WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> foo (section: .init.data)
> > > WARNING: modpost: vmlinux.o: section mismatch in reference: get_bar (section: .text) -> foo (section: .init.data)
> > >
> > > The reference from 'get_bar' to 'foo' seems wrong.
> > >
> > > I have no solution for this because it is true in assembly level.
> > >
> > > In the following output, relocation at 0x1c is no longer associated
> > > with 'bar'. The two relocation entries point to the same symbol, and
> > > the offset to 'bar' is encoded in the instruction 'r0, [r3, #4]'.
> > >
>
> These are section relative relocations - this is unusual but not
> incorrect. Normally, you only see this if the symbols in question have
> static linkage.
I noticed this usually happens in reference to 'static',
but on ARM, it happens even without 'static'.
See the [test code 1].
> It does mean that the symbol is not preemptible, which is what makes
> this somewhat surprising.
>
> Generally, you cannot resolve a relocation to a symbol without taking
> the addend into account, so looking up the address of .init.data in
> the symbol table is not quite the right approach here. If anything,
> the symbol should be reported as [.init.data+0x4] in the second case.
In the old days, section mismatch warnings showed
only the referenced section name.
Since [1], modpost started to show the referenced symbol name too.
Modpost did it in the past 17 years.
It sometimes shows a wrong name, but works in most architectures.
Unfortunately, I noticed ARM was an unfortunate case.
Do you suggest removing it entirely?
If (elf->symtab_start + ELF_R_SYM(r.r_info)) has a sensible
symbol name, print it. Otherwise, print only the section name.
Is this what you mean?
That means, we will lose the symbol name info of 'static'
(and even global symbols on ARM)
That is what I wrote in the commit description.
"I am keeping the current logic because it is useful in many architectures,
but the symbol name is not always correct depending on the optimization
of the relocation. I left some comments in find_tosym()."
[1] 93684d3b8062d1cebdeaed398ec6d1f354cb41a9
"kbuild: include symbol names in section mismatch warnings
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 02/20] modpost: fix section mismatch message for R_ARM_ABS32
2023-05-23 5:07 ` Masahiro Yamada
@ 2023-05-23 7:13 ` Ard Biesheuvel
0 siblings, 0 replies; 51+ messages in thread
From: Ard Biesheuvel @ 2023-05-23 7:13 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Nick Desaulniers, linux-kbuild, linux-kernel, Nathan Chancellor,
Nicolas Schier, Linux ARM, Fangrui Song
On Tue, 23 May 2023 at 07:08, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Tue, May 23, 2023 at 6:36 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Mon, 22 May 2023 at 19:56, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > >
> > > + linux-arm-kernel and some folks who might know another idea.
> > >
> > > On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > >
> > > > addend_arm_rel() processes R_ARM_ABS32 in a wrong way.
> > > >
> > > > Here, simple test code.
> > > >
> > > > [test code 1]
> > > >
> > > > #include <linux/init.h>
> > > >
> > > > int __initdata foo;
> > > > int get_foo(int x) { return foo; }
> > > >
> > > > If you compile it with ARM versatile_defconfig, modpost will show the
> > > > symbol name, (unknown).
> > > >
> > > > WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> (unknown) (section: .init.data)
> > > >
> > > > If you compile it for other architectures, modpost will show the correct
> > > > symbol name.
> > > >
> > > > WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> foo (section: .init.data)
> > > >
> > > > For R_ARM_ABS32, addend_arm_rel() sets r->r_addend to a wrong value.
> > > >
> > > > I just mimicked the code in arch/arm/kernel/module.c.
> > > >
> > > > However, there is more difficulty for ARM.
> > > >
> > > > Here, test code.
> > > >
> > > > [test code 2]
> > > >
> > > > #include <linux/init.h>
> > > >
> > > > int __initdata foo;
> > > > int get_foo(int x) { return foo; }
> > > >
> > > > int __initdata bar;
> > > > int get_bar(int x) { return bar; }
> > > >
> > > > With this commit applied, modpost will show the following messages
> > > > for ARM versatile_defconfig:
> > > >
> > > > WARNING: modpost: vmlinux.o: section mismatch in reference: get_foo (section: .text) -> foo (section: .init.data)
> > > > WARNING: modpost: vmlinux.o: section mismatch in reference: get_bar (section: .text) -> foo (section: .init.data)
> > > >
> > > > The reference from 'get_bar' to 'foo' seems wrong.
> > > >
> > > > I have no solution for this because it is true in assembly level.
> > > >
> > > > In the following output, relocation at 0x1c is no longer associated
> > > > with 'bar'. The two relocation entries point to the same symbol, and
> > > > the offset to 'bar' is encoded in the instruction 'r0, [r3, #4]'.
> > > >
> >
> > These are section relative relocations - this is unusual but not
> > incorrect. Normally, you only see this if the symbols in question have
> > static linkage.
>
>
> I noticed this usually happens in reference to 'static',
> but on ARM, it happens even without 'static'.
> See the [test code 1].
>
>
> > It does mean that the symbol is not preemptible, which is what makes
> > this somewhat surprising.
> >
> > Generally, you cannot resolve a relocation to a symbol without taking
> > the addend into account, so looking up the address of .init.data in
> > the symbol table is not quite the right approach here. If anything,
> > the symbol should be reported as [.init.data+0x4] in the second case.
>
>
> In the old days, section mismatch warnings showed
> only the referenced section name.
>
> Since [1], modpost started to show the referenced symbol name too.
> Modpost did it in the past 17 years.
> It sometimes shows a wrong name, but works in most architectures.
> Unfortunately, I noticed ARM was an unfortunate case.
>
> Do you suggest removing it entirely?
>
No, not at all. But resolving the symbol should take the addend into
account, and this is essentially what you are doing in your patch.
The point is really that the relocation in question does not refer to
the symbol - it refers to a section+offset that we /think/ corresponds
with a certain symbol. But for example, if the symbol is weak and
another definition exists, the section based relocation will refer to
one version, and a relocation that references the symbol name will
refer to the other version.
>
> If (elf->symtab_start + ELF_R_SYM(r.r_info)) has a sensible
> symbol name, print it. Otherwise, print only the section name.
> Is this what you mean?
>
> That means, we will lose the symbol name info of 'static'
> (and even global symbols on ARM)
>
>
> That is what I wrote in the commit description.
>
> "I am keeping the current logic because it is useful in many architectures,
> but the symbol name is not always correct depending on the optimization
> of the relocation. I left some comments in find_tosym()."
>
Fair enough.
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 03/20] modpost: detect section mismatch for R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
2023-05-21 16:04 ` [PATCH v6 01/20] Revert "modpost: skip ELF local symbols during section mismatch check" Masahiro Yamada
2023-05-21 16:04 ` [PATCH v6 02/20] modpost: fix section mismatch message for R_ARM_ABS32 Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-22 18:03 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 04/20] modpost: remove unused argument from secref_whitelist() Masahiro Yamada
` (17 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
ARM defconfig misses to detect some section mismatches.
[test code]
#include <linux/init.h>
int __initdata foo;
int get_foo(int x) { return foo; }
It is apparently a bad reference, but modpost does not report anything
for ARM defconfig (i.e. multi_v7_defconfig).
The test code above produces the following relocations.
Relocation section '.rel.text' at offset 0x200 contains 2 entries:
Offset Info Type Sym.Value Sym. Name
00000000 0000062b R_ARM_MOVW_ABS_NC 00000000 .LANCHOR0
00000004 0000062c R_ARM_MOVT_ABS 00000000 .LANCHOR0
Relocation section '.rel.ARM.exidx' at offset 0x210 contains 2 entries:
Offset Info Type Sym.Value Sym. Name
00000000 0000022a R_ARM_PREL31 00000000 .text
00000000 00001000 R_ARM_NONE 00000000 __aeabi_unwind_cpp_pr0
Currently, R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS are just skipped.
Add code to handle them. I checked arch/arm/kernel/module.c to learn
how the offset is encoded in the instruction.
The referenced symbol in relocation might be a local anchor.
If is_valid_name() returns false, let's search for a better symbol name.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 34fbbd85bfde..ed2301e951a9 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1108,7 +1108,7 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
/**
* Find symbol based on relocation record info.
* In some cases the symbol supplied is a valid symbol so
- * return refsym. If st_name != 0 we assume this is a valid symbol.
+ * return refsym. If is_valid_name() == true, we assume this is a valid symbol.
* In other cases the symbol needs to be looked up in the symbol table
* based on section and address.
* **/
@@ -1121,7 +1121,7 @@ static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
Elf64_Sword d;
unsigned int relsym_secindex;
- if (relsym->st_name != 0)
+ if (is_valid_name(elf, relsym))
return relsym;
/*
@@ -1312,11 +1312,19 @@ static int addend_arm_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
unsigned int r_typ = ELF_R_TYPE(r->r_info);
Elf_Sym *sym = elf->symtab_start + ELF_R_SYM(r->r_info);
unsigned int inst = TO_NATIVE(*reloc_location(elf, sechdr, r));
+ int offset;
switch (r_typ) {
case R_ARM_ABS32:
r->r_addend = inst + sym->st_value;
break;
+ case R_ARM_MOVW_ABS_NC:
+ case R_ARM_MOVT_ABS:
+ offset = ((inst & 0xf0000) >> 4) | (inst & 0xfff);
+ offset = (offset ^ 0x8000) - 0x8000;
+ offset += sym->st_value;
+ r->r_addend = offset;
+ break;
case R_ARM_PC24:
case R_ARM_CALL:
case R_ARM_JUMP24:
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 03/20] modpost: detect section mismatch for R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS
2023-05-21 16:04 ` [PATCH v6 03/20] modpost: detect section mismatch for R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS Masahiro Yamada
@ 2023-05-22 18:03 ` Nick Desaulniers
2023-05-22 21:50 ` Ard Biesheuvel
2023-05-24 0:04 ` Masahiro Yamada
0 siblings, 2 replies; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-22 18:03 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier,
Linux ARM, Ard Biesheuvel, Fangrui Song
+ linux-arm-kernel
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> ARM defconfig misses to detect some section mismatches.
>
> [test code]
>
> #include <linux/init.h>
>
> int __initdata foo;
> int get_foo(int x) { return foo; }
>
> It is apparently a bad reference, but modpost does not report anything
> for ARM defconfig (i.e. multi_v7_defconfig).
>
> The test code above produces the following relocations.
>
> Relocation section '.rel.text' at offset 0x200 contains 2 entries:
> Offset Info Type Sym.Value Sym. Name
> 00000000 0000062b R_ARM_MOVW_ABS_NC 00000000 .LANCHOR0
> 00000004 0000062c R_ARM_MOVT_ABS 00000000 .LANCHOR0
>
> Relocation section '.rel.ARM.exidx' at offset 0x210 contains 2 entries:
> Offset Info Type Sym.Value Sym. Name
> 00000000 0000022a R_ARM_PREL31 00000000 .text
> 00000000 00001000 R_ARM_NONE 00000000 __aeabi_unwind_cpp_pr0
>
> Currently, R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS are just skipped.
>
> Add code to handle them. I checked arch/arm/kernel/module.c to learn
> how the offset is encoded in the instruction.
>
> The referenced symbol in relocation might be a local anchor.
> If is_valid_name() returns false, let's search for a better symbol name.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> scripts/mod/modpost.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 34fbbd85bfde..ed2301e951a9 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1108,7 +1108,7 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
> /**
> * Find symbol based on relocation record info.
> * In some cases the symbol supplied is a valid symbol so
> - * return refsym. If st_name != 0 we assume this is a valid symbol.
> + * return refsym. If is_valid_name() == true, we assume this is a valid symbol.
> * In other cases the symbol needs to be looked up in the symbol table
> * based on section and address.
> * **/
> @@ -1121,7 +1121,7 @@ static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
> Elf64_Sword d;
> unsigned int relsym_secindex;
>
> - if (relsym->st_name != 0)
> + if (is_valid_name(elf, relsym))
> return relsym;
>
> /*
> @@ -1312,11 +1312,19 @@ static int addend_arm_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
> unsigned int r_typ = ELF_R_TYPE(r->r_info);
> Elf_Sym *sym = elf->symtab_start + ELF_R_SYM(r->r_info);
> unsigned int inst = TO_NATIVE(*reloc_location(elf, sechdr, r));
> + int offset;
>
> switch (r_typ) {
> case R_ARM_ABS32:
> r->r_addend = inst + sym->st_value;
> break;
> + case R_ARM_MOVW_ABS_NC:
> + case R_ARM_MOVT_ABS:
> + offset = ((inst & 0xf0000) >> 4) | (inst & 0xfff);
> + offset = (offset ^ 0x8000) - 0x8000;
The code in arch/arm/kernel/module.c then right shifts the offset by
16 for R_ARM_MOVT_ABS. Is that necessary?
> + offset += sym->st_value;
> + r->r_addend = offset;
> + break;
> case R_ARM_PC24:
> case R_ARM_CALL:
> case R_ARM_JUMP24:
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 03/20] modpost: detect section mismatch for R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS
2023-05-22 18:03 ` Nick Desaulniers
@ 2023-05-22 21:50 ` Ard Biesheuvel
2023-05-23 11:58 ` Masahiro Yamada
2023-05-24 0:04 ` Masahiro Yamada
1 sibling, 1 reply; 51+ messages in thread
From: Ard Biesheuvel @ 2023-05-22 21:50 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Masahiro Yamada, linux-kbuild, linux-kernel, Nathan Chancellor,
Nicolas Schier, Linux ARM, Fangrui Song
On Mon, 22 May 2023 at 20:03, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> + linux-arm-kernel
>
> On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > ARM defconfig misses to detect some section mismatches.
> >
> > [test code]
> >
> > #include <linux/init.h>
> >
> > int __initdata foo;
> > int get_foo(int x) { return foo; }
> >
> > It is apparently a bad reference, but modpost does not report anything
> > for ARM defconfig (i.e. multi_v7_defconfig).
> >
> > The test code above produces the following relocations.
> >
> > Relocation section '.rel.text' at offset 0x200 contains 2 entries:
> > Offset Info Type Sym.Value Sym. Name
> > 00000000 0000062b R_ARM_MOVW_ABS_NC 00000000 .LANCHOR0
> > 00000004 0000062c R_ARM_MOVT_ABS 00000000 .LANCHOR0
> >
> > Relocation section '.rel.ARM.exidx' at offset 0x210 contains 2 entries:
> > Offset Info Type Sym.Value Sym. Name
> > 00000000 0000022a R_ARM_PREL31 00000000 .text
> > 00000000 00001000 R_ARM_NONE 00000000 __aeabi_unwind_cpp_pr0
> >
> > Currently, R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS are just skipped.
> >
> > Add code to handle them. I checked arch/arm/kernel/module.c to learn
> > how the offset is encoded in the instruction.
> >
> > The referenced symbol in relocation might be a local anchor.
> > If is_valid_name() returns false, let's search for a better symbol name.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> > scripts/mod/modpost.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 34fbbd85bfde..ed2301e951a9 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1108,7 +1108,7 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
> > /**
> > * Find symbol based on relocation record info.
> > * In some cases the symbol supplied is a valid symbol so
> > - * return refsym. If st_name != 0 we assume this is a valid symbol.
> > + * return refsym. If is_valid_name() == true, we assume this is a valid symbol.
> > * In other cases the symbol needs to be looked up in the symbol table
> > * based on section and address.
> > * **/
> > @@ -1121,7 +1121,7 @@ static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
> > Elf64_Sword d;
> > unsigned int relsym_secindex;
> >
> > - if (relsym->st_name != 0)
> > + if (is_valid_name(elf, relsym))
> > return relsym;
> >
> > /*
> > @@ -1312,11 +1312,19 @@ static int addend_arm_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
> > unsigned int r_typ = ELF_R_TYPE(r->r_info);
> > Elf_Sym *sym = elf->symtab_start + ELF_R_SYM(r->r_info);
> > unsigned int inst = TO_NATIVE(*reloc_location(elf, sechdr, r));
> > + int offset;
> >
> > switch (r_typ) {
> > case R_ARM_ABS32:
> > r->r_addend = inst + sym->st_value;
> > break;
> > + case R_ARM_MOVW_ABS_NC:
> > + case R_ARM_MOVT_ABS:
> > + offset = ((inst & 0xf0000) >> 4) | (inst & 0xfff);
> > + offset = (offset ^ 0x8000) - 0x8000;
>
> The code in arch/arm/kernel/module.c then right shifts the offset by
> 16 for R_ARM_MOVT_ABS. Is that necessary?
>
MOVW/MOVT pairs are limited to an addend of -/+ 32 KiB, and the same
value must be encoded in both instructions.
When constructing the actual immediate value from the symbol value and
the addend, only the top 16 bits are used in MOVT and the bottom 16
bits in MOVW.
However, this code seems to borrow the Elf_Rela::addend field (which
ARM does not use natively) to record the intermediate value, which
would need to be split if it is used to fix up instruction opcodes.
Btw the Thumb2 encodings of MOVT and MOVW seem to be missing here.
> > + offset += sym->st_value;
> > + r->r_addend = offset;
> > + break;
> > case R_ARM_PC24:
> > case R_ARM_CALL:
> > case R_ARM_JUMP24:
> > --
> > 2.39.2
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 03/20] modpost: detect section mismatch for R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS
2023-05-22 21:50 ` Ard Biesheuvel
@ 2023-05-23 11:58 ` Masahiro Yamada
2023-05-23 12:20 ` Ard Biesheuvel
0 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-23 11:58 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Nick Desaulniers, linux-kbuild, linux-kernel, Nathan Chancellor,
Nicolas Schier, Linux ARM, Fangrui Song
On Tue, May 23, 2023 at 6:50 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 22 May 2023 at 20:03, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > + linux-arm-kernel
> >
> > On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > ARM defconfig misses to detect some section mismatches.
> > >
> > > [test code]
> > >
> > > #include <linux/init.h>
> > >
> > > int __initdata foo;
> > > int get_foo(int x) { return foo; }
> > >
> > > It is apparently a bad reference, but modpost does not report anything
> > > for ARM defconfig (i.e. multi_v7_defconfig).
> > >
> > > The test code above produces the following relocations.
> > >
> > > Relocation section '.rel.text' at offset 0x200 contains 2 entries:
> > > Offset Info Type Sym.Value Sym. Name
> > > 00000000 0000062b R_ARM_MOVW_ABS_NC 00000000 .LANCHOR0
> > > 00000004 0000062c R_ARM_MOVT_ABS 00000000 .LANCHOR0
> > >
> > > Relocation section '.rel.ARM.exidx' at offset 0x210 contains 2 entries:
> > > Offset Info Type Sym.Value Sym. Name
> > > 00000000 0000022a R_ARM_PREL31 00000000 .text
> > > 00000000 00001000 R_ARM_NONE 00000000 __aeabi_unwind_cpp_pr0
> > >
> > > Currently, R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS are just skipped.
> > >
> > > Add code to handle them. I checked arch/arm/kernel/module.c to learn
> > > how the offset is encoded in the instruction.
> > >
> > > The referenced symbol in relocation might be a local anchor.
> > > If is_valid_name() returns false, let's search for a better symbol name.
> > >
> > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > ---
> > >
> > > scripts/mod/modpost.c | 12 ++++++++++--
> > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > > index 34fbbd85bfde..ed2301e951a9 100644
> > > --- a/scripts/mod/modpost.c
> > > +++ b/scripts/mod/modpost.c
> > > @@ -1108,7 +1108,7 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
> > > /**
> > > * Find symbol based on relocation record info.
> > > * In some cases the symbol supplied is a valid symbol so
> > > - * return refsym. If st_name != 0 we assume this is a valid symbol.
> > > + * return refsym. If is_valid_name() == true, we assume this is a valid symbol.
> > > * In other cases the symbol needs to be looked up in the symbol table
> > > * based on section and address.
> > > * **/
> > > @@ -1121,7 +1121,7 @@ static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
> > > Elf64_Sword d;
> > > unsigned int relsym_secindex;
> > >
> > > - if (relsym->st_name != 0)
> > > + if (is_valid_name(elf, relsym))
> > > return relsym;
> > >
> > > /*
> > > @@ -1312,11 +1312,19 @@ static int addend_arm_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
> > > unsigned int r_typ = ELF_R_TYPE(r->r_info);
> > > Elf_Sym *sym = elf->symtab_start + ELF_R_SYM(r->r_info);
> > > unsigned int inst = TO_NATIVE(*reloc_location(elf, sechdr, r));
> > > + int offset;
> > >
> > > switch (r_typ) {
> > > case R_ARM_ABS32:
> > > r->r_addend = inst + sym->st_value;
> > > break;
> > > + case R_ARM_MOVW_ABS_NC:
> > > + case R_ARM_MOVT_ABS:
> > > + offset = ((inst & 0xf0000) >> 4) | (inst & 0xfff);
> > > + offset = (offset ^ 0x8000) - 0x8000;
> >
> > The code in arch/arm/kernel/module.c then right shifts the offset by
> > 16 for R_ARM_MOVT_ABS. Is that necessary?
> >
>
> MOVW/MOVT pairs are limited to an addend of -/+ 32 KiB, and the same
> value must be encoded in both instructions.
In my understanding, 'movt' loads the immediate value to
the upper 16-bit of the register.
I am just curious about the code in arch/arm/kernel/module.c.
Please see 'case R_ARM_MOVT_ABS:' part.
[1] 'offset' is the immediate value encoded in instruction
[2] Add sym->st_value
[3] Right-shift 'offset' by 16
[4] Write it back to the instruction
So, the immediate value encoded in the instruction
is divided by 65536.
I guess we need something like the following?
(left-shift by 16).
if (ELF32_R_TYPE(rel->r_info) == R_ARM_MOVT_ABS ||
ELF32_R_TYPE(rel->r_info) == R_ARM_MOVT_PREL)
offset <<= 16;
>
> When constructing the actual immediate value from the symbol value and
> the addend, only the top 16 bits are used in MOVT and the bottom 16
> bits in MOVW.
>
> However, this code seems to borrow the Elf_Rela::addend field (which
> ARM does not use natively) to record the intermediate value, which
> would need to be split if it is used to fix up instruction opcodes.
At first, modpost supported only RELA for section mismatch checks.
Later, 2c1a51f39d95 ("[PATCH] kbuild: check SHT_REL sections")
added REL support.
But, the common code still used Elf_Rela.
modpost does not need to write back the fixed instruction.
modpost is only interested in the offset address.
Currently, modpost saves the offset address in
r->r_offset even for Rel. I do not like this code.
So, I am trying to reduce the use of Elf_Rela.
For example, this patch.
https://patchwork.kernel.org/project/linux-kbuild/patch/20230521160426.1881124-8-masahiroy@kernel.org/
> Btw the Thumb2 encodings of MOVT and MOVW seem to be missing here.
Right, if CONFIG_THUMB2_KERNEL=y, section mismatch check.
Several relocation types are just skipped.
>
>
> > > + offset += sym->st_value;
> > > + r->r_addend = offset;
> > > + break;
> > > case R_ARM_PC24:
> > > case R_ARM_CALL:
> > > case R_ARM_JUMP24:
> > > --
> > > 2.39.2
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 03/20] modpost: detect section mismatch for R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS
2023-05-23 11:58 ` Masahiro Yamada
@ 2023-05-23 12:20 ` Ard Biesheuvel
2023-05-24 0:02 ` Masahiro Yamada
0 siblings, 1 reply; 51+ messages in thread
From: Ard Biesheuvel @ 2023-05-23 12:20 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Nick Desaulniers, linux-kbuild, linux-kernel, Nathan Chancellor,
Nicolas Schier, Linux ARM, Fangrui Song
On Tue, 23 May 2023 at 13:59, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Tue, May 23, 2023 at 6:50 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Mon, 22 May 2023 at 20:03, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > >
> > > + linux-arm-kernel
> > >
> > > On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > >
> > > > ARM defconfig misses to detect some section mismatches.
> > > >
> > > > [test code]
> > > >
> > > > #include <linux/init.h>
> > > >
> > > > int __initdata foo;
> > > > int get_foo(int x) { return foo; }
> > > >
> > > > It is apparently a bad reference, but modpost does not report anything
> > > > for ARM defconfig (i.e. multi_v7_defconfig).
> > > >
> > > > The test code above produces the following relocations.
> > > >
> > > > Relocation section '.rel.text' at offset 0x200 contains 2 entries:
> > > > Offset Info Type Sym.Value Sym. Name
> > > > 00000000 0000062b R_ARM_MOVW_ABS_NC 00000000 .LANCHOR0
> > > > 00000004 0000062c R_ARM_MOVT_ABS 00000000 .LANCHOR0
> > > >
> > > > Relocation section '.rel.ARM.exidx' at offset 0x210 contains 2 entries:
> > > > Offset Info Type Sym.Value Sym. Name
> > > > 00000000 0000022a R_ARM_PREL31 00000000 .text
> > > > 00000000 00001000 R_ARM_NONE 00000000 __aeabi_unwind_cpp_pr0
> > > >
> > > > Currently, R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS are just skipped.
> > > >
> > > > Add code to handle them. I checked arch/arm/kernel/module.c to learn
> > > > how the offset is encoded in the instruction.
> > > >
> > > > The referenced symbol in relocation might be a local anchor.
> > > > If is_valid_name() returns false, let's search for a better symbol name.
> > > >
> > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > > ---
> > > >
> > > > scripts/mod/modpost.c | 12 ++++++++++--
> > > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > > > index 34fbbd85bfde..ed2301e951a9 100644
> > > > --- a/scripts/mod/modpost.c
> > > > +++ b/scripts/mod/modpost.c
> > > > @@ -1108,7 +1108,7 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
> > > > /**
> > > > * Find symbol based on relocation record info.
> > > > * In some cases the symbol supplied is a valid symbol so
> > > > - * return refsym. If st_name != 0 we assume this is a valid symbol.
> > > > + * return refsym. If is_valid_name() == true, we assume this is a valid symbol.
> > > > * In other cases the symbol needs to be looked up in the symbol table
> > > > * based on section and address.
> > > > * **/
> > > > @@ -1121,7 +1121,7 @@ static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
> > > > Elf64_Sword d;
> > > > unsigned int relsym_secindex;
> > > >
> > > > - if (relsym->st_name != 0)
> > > > + if (is_valid_name(elf, relsym))
> > > > return relsym;
> > > >
> > > > /*
> > > > @@ -1312,11 +1312,19 @@ static int addend_arm_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
> > > > unsigned int r_typ = ELF_R_TYPE(r->r_info);
> > > > Elf_Sym *sym = elf->symtab_start + ELF_R_SYM(r->r_info);
> > > > unsigned int inst = TO_NATIVE(*reloc_location(elf, sechdr, r));
> > > > + int offset;
> > > >
> > > > switch (r_typ) {
> > > > case R_ARM_ABS32:
> > > > r->r_addend = inst + sym->st_value;
> > > > break;
> > > > + case R_ARM_MOVW_ABS_NC:
> > > > + case R_ARM_MOVT_ABS:
> > > > + offset = ((inst & 0xf0000) >> 4) | (inst & 0xfff);
> > > > + offset = (offset ^ 0x8000) - 0x8000;
> > >
> > > The code in arch/arm/kernel/module.c then right shifts the offset by
> > > 16 for R_ARM_MOVT_ABS. Is that necessary?
> > >
> >
> > MOVW/MOVT pairs are limited to an addend of -/+ 32 KiB, and the same
> > value must be encoded in both instructions.
>
>
> In my understanding, 'movt' loads the immediate value to
> the upper 16-bit of the register.
>
Correct. It sets the upper 16 bits of a register without corrupting
the lower 16 bits.
> I am just curious about the code in arch/arm/kernel/module.c.
>
> Please see 'case R_ARM_MOVT_ABS:' part.
>
> [1] 'offset' is the immediate value encoded in instruction
> [2] Add sym->st_value
> [3] Right-shift 'offset' by 16
> [4] Write it back to the instruction
>
> So, the immediate value encoded in the instruction
> is divided by 65536.
>
> I guess we need something like the following?
> (left-shift by 16).
>
> if (ELF32_R_TYPE(rel->r_info) == R_ARM_MOVT_ABS ||
> ELF32_R_TYPE(rel->r_info) == R_ARM_MOVT_PREL)
> offset <<= 16;
>
No. The addend is not encoded in the same way as the effective immediate value.
The addend is limited to -/+ 32 KiB (range of s16), and the MOVT
instruction must use the same addend value as the MOVW instruction it
is paired with, without shifting.
This is necessary because otherwise, there is no way to handle an
addend/symbol combination that results in a carry between the lower
and upper 16 bit words. This is a consequence of the use of REL format
rather than RELA, where the addend is part of the relocation and not
encoded in the instructions.
>
>
>
> >
> > When constructing the actual immediate value from the symbol value and
> > the addend, only the top 16 bits are used in MOVT and the bottom 16
> > bits in MOVW.
> >
> > However, this code seems to borrow the Elf_Rela::addend field (which
> > ARM does not use natively) to record the intermediate value, which
> > would need to be split if it is used to fix up instruction opcodes.
>
> At first, modpost supported only RELA for section mismatch checks.
>
> Later, 2c1a51f39d95 ("[PATCH] kbuild: check SHT_REL sections")
> added REL support.
>
> But, the common code still used Elf_Rela.
>
>
> modpost does not need to write back the fixed instruction.
> modpost is only interested in the offset address.
>
> Currently, modpost saves the offset address in
> r->r_offset even for Rel. I do not like this code.
>
> So, I am trying to reduce the use of Elf_Rela.
> For example, this patch.
> https://patchwork.kernel.org/project/linux-kbuild/patch/20230521160426.1881124-8-masahiroy@kernel.org/
>
Yeah, that looks better to me.
>
> > Btw the Thumb2 encodings of MOVT and MOVW seem to be missing here.
>
> Right, if CONFIG_THUMB2_KERNEL=y, section mismatch check.
>
> Several relocation types are just skipped.
>
Skipped entirely? Or only for the diagnostic print that outputs the symbol name?
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 03/20] modpost: detect section mismatch for R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS
2023-05-23 12:20 ` Ard Biesheuvel
@ 2023-05-24 0:02 ` Masahiro Yamada
0 siblings, 0 replies; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-24 0:02 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Nick Desaulniers, linux-kbuild, linux-kernel, Nathan Chancellor,
Nicolas Schier, Linux ARM, Fangrui Song
On Tue, May 23, 2023 at 9:21 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 23 May 2023 at 13:59, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Tue, May 23, 2023 at 6:50 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Mon, 22 May 2023 at 20:03, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > > >
> > > > + linux-arm-kernel
> > > >
> > > > On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > >
> > > > > ARM defconfig misses to detect some section mismatches.
> > > > >
> > > > > [test code]
> > > > >
> > > > > #include <linux/init.h>
> > > > >
> > > > > int __initdata foo;
> > > > > int get_foo(int x) { return foo; }
> > > > >
> > > > > It is apparently a bad reference, but modpost does not report anything
> > > > > for ARM defconfig (i.e. multi_v7_defconfig).
> > > > >
> > > > > The test code above produces the following relocations.
> > > > >
> > > > > Relocation section '.rel.text' at offset 0x200 contains 2 entries:
> > > > > Offset Info Type Sym.Value Sym. Name
> > > > > 00000000 0000062b R_ARM_MOVW_ABS_NC 00000000 .LANCHOR0
> > > > > 00000004 0000062c R_ARM_MOVT_ABS 00000000 .LANCHOR0
> > > > >
> > > > > Relocation section '.rel.ARM.exidx' at offset 0x210 contains 2 entries:
> > > > > Offset Info Type Sym.Value Sym. Name
> > > > > 00000000 0000022a R_ARM_PREL31 00000000 .text
> > > > > 00000000 00001000 R_ARM_NONE 00000000 __aeabi_unwind_cpp_pr0
> > > > >
> > > > > Currently, R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS are just skipped.
> > > > >
> > > > > Add code to handle them. I checked arch/arm/kernel/module.c to learn
> > > > > how the offset is encoded in the instruction.
> > > > >
> > > > > The referenced symbol in relocation might be a local anchor.
> > > > > If is_valid_name() returns false, let's search for a better symbol name.
> > > > >
> > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > > > ---
> > > > >
> > > > > scripts/mod/modpost.c | 12 ++++++++++--
> > > > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > > > > index 34fbbd85bfde..ed2301e951a9 100644
> > > > > --- a/scripts/mod/modpost.c
> > > > > +++ b/scripts/mod/modpost.c
> > > > > @@ -1108,7 +1108,7 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
> > > > > /**
> > > > > * Find symbol based on relocation record info.
> > > > > * In some cases the symbol supplied is a valid symbol so
> > > > > - * return refsym. If st_name != 0 we assume this is a valid symbol.
> > > > > + * return refsym. If is_valid_name() == true, we assume this is a valid symbol.
> > > > > * In other cases the symbol needs to be looked up in the symbol table
> > > > > * based on section and address.
> > > > > * **/
> > > > > @@ -1121,7 +1121,7 @@ static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
> > > > > Elf64_Sword d;
> > > > > unsigned int relsym_secindex;
> > > > >
> > > > > - if (relsym->st_name != 0)
> > > > > + if (is_valid_name(elf, relsym))
> > > > > return relsym;
> > > > >
> > > > > /*
> > > > > @@ -1312,11 +1312,19 @@ static int addend_arm_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
> > > > > unsigned int r_typ = ELF_R_TYPE(r->r_info);
> > > > > Elf_Sym *sym = elf->symtab_start + ELF_R_SYM(r->r_info);
> > > > > unsigned int inst = TO_NATIVE(*reloc_location(elf, sechdr, r));
> > > > > + int offset;
> > > > >
> > > > > switch (r_typ) {
> > > > > case R_ARM_ABS32:
> > > > > r->r_addend = inst + sym->st_value;
> > > > > break;
> > > > > + case R_ARM_MOVW_ABS_NC:
> > > > > + case R_ARM_MOVT_ABS:
> > > > > + offset = ((inst & 0xf0000) >> 4) | (inst & 0xfff);
> > > > > + offset = (offset ^ 0x8000) - 0x8000;
> > > >
> > > > The code in arch/arm/kernel/module.c then right shifts the offset by
> > > > 16 for R_ARM_MOVT_ABS. Is that necessary?
> > > >
> > >
> > > MOVW/MOVT pairs are limited to an addend of -/+ 32 KiB, and the same
> > > value must be encoded in both instructions.
> >
> >
> > In my understanding, 'movt' loads the immediate value to
> > the upper 16-bit of the register.
> >
>
> Correct. It sets the upper 16 bits of a register without corrupting
> the lower 16 bits.
>
> > I am just curious about the code in arch/arm/kernel/module.c.
> >
> > Please see 'case R_ARM_MOVT_ABS:' part.
> >
> > [1] 'offset' is the immediate value encoded in instruction
> > [2] Add sym->st_value
> > [3] Right-shift 'offset' by 16
> > [4] Write it back to the instruction
> >
> > So, the immediate value encoded in the instruction
> > is divided by 65536.
> >
> > I guess we need something like the following?
> > (left-shift by 16).
> >
> > if (ELF32_R_TYPE(rel->r_info) == R_ARM_MOVT_ABS ||
> > ELF32_R_TYPE(rel->r_info) == R_ARM_MOVT_PREL)
> > offset <<= 16;
> >
>
> No. The addend is not encoded in the same way as the effective immediate value.
>
> The addend is limited to -/+ 32 KiB (range of s16), and the MOVT
> instruction must use the same addend value as the MOVW instruction it
> is paired with, without shifting.
>
> This is necessary because otherwise, there is no way to handle an
> addend/symbol combination that results in a carry between the lower
> and upper 16 bit words. This is a consequence of the use of REL format
> rather than RELA, where the addend is part of the relocation and not
> encoded in the instructions.
Ah, OK.
Now I understand.
> >
> >
> >
> > >
> > > When constructing the actual immediate value from the symbol value and
> > > the addend, only the top 16 bits are used in MOVT and the bottom 16
> > > bits in MOVW.
> > >
> > > However, this code seems to borrow the Elf_Rela::addend field (which
> > > ARM does not use natively) to record the intermediate value, which
> > > would need to be split if it is used to fix up instruction opcodes.
> >
> > At first, modpost supported only RELA for section mismatch checks.
> >
> > Later, 2c1a51f39d95 ("[PATCH] kbuild: check SHT_REL sections")
> > added REL support.
> >
> > But, the common code still used Elf_Rela.
> >
> >
> > modpost does not need to write back the fixed instruction.
> > modpost is only interested in the offset address.
> >
> > Currently, modpost saves the offset address in
> > r->r_offset even for Rel. I do not like this code.
> >
> > So, I am trying to reduce the use of Elf_Rela.
> > For example, this patch.
> > https://patchwork.kernel.org/project/linux-kbuild/patch/20230521160426.1881124-8-masahiroy@kernel.org/
> >
>
> Yeah, that looks better to me.
>
> >
> > > Btw the Thumb2 encodings of MOVT and MOVW seem to be missing here.
> >
> > Right, if CONFIG_THUMB2_KERNEL=y, section mismatch check.
> >
> > Several relocation types are just skipped.
> >
>
> Skipped entirely? Or only for the diagnostic print that outputs the symbol name?
Skipped entirely.
modpost cannot detect section mismatches
if you enable CONFIG_THUMB2_KERNEL.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v6 03/20] modpost: detect section mismatch for R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS
2023-05-22 18:03 ` Nick Desaulniers
2023-05-22 21:50 ` Ard Biesheuvel
@ 2023-05-24 0:04 ` Masahiro Yamada
1 sibling, 0 replies; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-24 0:04 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier,
Linux ARM, Ard Biesheuvel, Fangrui Song
On Tue, May 23, 2023 at 3:03 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> + linux-arm-kernel
>
> On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > ARM defconfig misses to detect some section mismatches.
> >
> > [test code]
> >
> > #include <linux/init.h>
> >
> > int __initdata foo;
> > int get_foo(int x) { return foo; }
> >
> > It is apparently a bad reference, but modpost does not report anything
> > for ARM defconfig (i.e. multi_v7_defconfig).
> >
> > The test code above produces the following relocations.
> >
> > Relocation section '.rel.text' at offset 0x200 contains 2 entries:
> > Offset Info Type Sym.Value Sym. Name
> > 00000000 0000062b R_ARM_MOVW_ABS_NC 00000000 .LANCHOR0
> > 00000004 0000062c R_ARM_MOVT_ABS 00000000 .LANCHOR0
> >
> > Relocation section '.rel.ARM.exidx' at offset 0x210 contains 2 entries:
> > Offset Info Type Sym.Value Sym. Name
> > 00000000 0000022a R_ARM_PREL31 00000000 .text
> > 00000000 00001000 R_ARM_NONE 00000000 __aeabi_unwind_cpp_pr0
> >
> > Currently, R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS are just skipped.
> >
> > Add code to handle them. I checked arch/arm/kernel/module.c to learn
> > how the offset is encoded in the instruction.
> >
> > The referenced symbol in relocation might be a local anchor.
> > If is_valid_name() returns false, let's search for a better symbol name.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> > scripts/mod/modpost.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 34fbbd85bfde..ed2301e951a9 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1108,7 +1108,7 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
> > /**
> > * Find symbol based on relocation record info.
> > * In some cases the symbol supplied is a valid symbol so
> > - * return refsym. If st_name != 0 we assume this is a valid symbol.
> > + * return refsym. If is_valid_name() == true, we assume this is a valid symbol.
> > * In other cases the symbol needs to be looked up in the symbol table
> > * based on section and address.
> > * **/
> > @@ -1121,7 +1121,7 @@ static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
> > Elf64_Sword d;
> > unsigned int relsym_secindex;
> >
> > - if (relsym->st_name != 0)
> > + if (is_valid_name(elf, relsym))
> > return relsym;
> >
> > /*
> > @@ -1312,11 +1312,19 @@ static int addend_arm_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
> > unsigned int r_typ = ELF_R_TYPE(r->r_info);
> > Elf_Sym *sym = elf->symtab_start + ELF_R_SYM(r->r_info);
> > unsigned int inst = TO_NATIVE(*reloc_location(elf, sechdr, r));
> > + int offset;
> >
> > switch (r_typ) {
> > case R_ARM_ABS32:
> > r->r_addend = inst + sym->st_value;
> > break;
> > + case R_ARM_MOVW_ABS_NC:
> > + case R_ARM_MOVT_ABS:
> > + offset = ((inst & 0xf0000) >> 4) | (inst & 0xfff);
> > + offset = (offset ^ 0x8000) - 0x8000;
>
> The code in arch/arm/kernel/module.c then right shifts the offset by
> 16 for R_ARM_MOVT_ABS. Is that necessary?
I replied to Ard's email, but just in case.
modpost does not need to write back the fixed instruction.
modpost is only interested in the offset address.
So, the right-shift by 16 is unneeded.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 04/20] modpost: remove unused argument from secref_whitelist()
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (2 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 03/20] modpost: detect section mismatch for R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-22 18:10 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 05/20] modpost: refactor find_fromsym() and find_tosym() Masahiro Yamada
` (16 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
secref_whitelist() does not use the argument 'mismatch'.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index ed2301e951a9..d2329ac32177 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1035,8 +1035,7 @@ static const struct sectioncheck *section_mismatch(
* refsymname = *.constprop.*
*
**/
-static int secref_whitelist(const struct sectioncheck *mismatch,
- const char *fromsec, const char *fromsym,
+static int secref_whitelist(const char *fromsec, const char *fromsym,
const char *tosec, const char *tosym)
{
/* Check for pattern 1 */
@@ -1206,7 +1205,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
tosym = sym_name(elf, to);
/* check whitelist - we may ignore it */
- if (!secref_whitelist(mismatch, fromsec, fromsym, tosec, tosym))
+ if (!secref_whitelist(fromsec, fromsym, tosec, tosym))
return;
sec_mismatch_count++;
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 04/20] modpost: remove unused argument from secref_whitelist()
2023-05-21 16:04 ` [PATCH v6 04/20] modpost: remove unused argument from secref_whitelist() Masahiro Yamada
@ 2023-05-22 18:10 ` Nick Desaulniers
0 siblings, 0 replies; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-22 18:10 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> secref_whitelist() does not use the argument 'mismatch'.
Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> scripts/mod/modpost.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index ed2301e951a9..d2329ac32177 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1035,8 +1035,7 @@ static const struct sectioncheck *section_mismatch(
> * refsymname = *.constprop.*
> *
> **/
> -static int secref_whitelist(const struct sectioncheck *mismatch,
> - const char *fromsec, const char *fromsym,
> +static int secref_whitelist(const char *fromsec, const char *fromsym,
> const char *tosec, const char *tosym)
> {
> /* Check for pattern 1 */
> @@ -1206,7 +1205,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> tosym = sym_name(elf, to);
>
> /* check whitelist - we may ignore it */
> - if (!secref_whitelist(mismatch, fromsec, fromsym, tosec, tosym))
> + if (!secref_whitelist(fromsec, fromsym, tosec, tosym))
> return;
>
> sec_mismatch_count++;
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 05/20] modpost: refactor find_fromsym() and find_tosym()
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (3 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 04/20] modpost: remove unused argument from secref_whitelist() Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-22 18:18 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 06/20] modpost: unify 'sym' and 'to' in default_mismatch_handler() Masahiro Yamada
` (15 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
find_fromsym() and find_tosym() are similar - both of them iterate
in the .symtab section and return the nearest symbol.
The difference between them is that find_tosym() allows a negative
distance, but the distance must be less than 20.
Factor out the common part into find_nearest_sym().
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
Changes in v6:
- Revive the check for distance less than 20
scripts/mod/modpost.c | 95 ++++++++++++++++---------------------------
1 file changed, 36 insertions(+), 59 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index d2329ac32177..6ac0d571542c 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1104,81 +1104,58 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
return !is_mapping_symbol(name);
}
-/**
- * Find symbol based on relocation record info.
- * In some cases the symbol supplied is a valid symbol so
- * return refsym. If is_valid_name() == true, we assume this is a valid symbol.
- * In other cases the symbol needs to be looked up in the symbol table
- * based on section and address.
- * **/
-static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
- Elf_Sym *relsym)
+/* Look up the nearest symbol based on the section and the address */
+static Elf_Sym *find_nearest_sym(struct elf_info *elf, Elf_Addr addr,
+ unsigned int secndx, bool allow_negative,
+ Elf_Addr min_distance)
{
Elf_Sym *sym;
Elf_Sym *near = NULL;
- Elf64_Sword distance = 20;
- Elf64_Sword d;
- unsigned int relsym_secindex;
-
- if (is_valid_name(elf, relsym))
- return relsym;
-
- /*
- * Strive to find a better symbol name, but the resulting name does not
- * always match the symbol referenced in the original code.
- */
- relsym_secindex = get_secindex(elf, relsym);
- for (sym = elf->symtab_start; sym < elf->symtab_stop; sym++) {
- if (get_secindex(elf, sym) != relsym_secindex)
- continue;
- if (ELF_ST_TYPE(sym->st_info) == STT_SECTION)
- continue;
- if (!is_valid_name(elf, sym))
- continue;
- if (sym->st_value == addr)
- return sym;
- /* Find a symbol nearby - addr are maybe negative */
- d = sym->st_value - addr;
- if (d < 0)
- d = addr - sym->st_value;
- if (d < distance) {
- distance = d;
- near = sym;
- }
- }
- /* We need a close match */
- if (distance < 20)
- return near;
- else
- return NULL;
-}
-
-/*
- * Find symbols before or equal addr and after addr - in the section sec.
- * If we find two symbols with equal offset prefer one with a valid name.
- * The ELF format may have a better way to detect what type of symbol
- * it is, but this works for now.
- **/
-static Elf_Sym *find_fromsym(struct elf_info *elf, Elf_Addr addr,
- unsigned int secndx)
-{
- Elf_Sym *sym;
- Elf_Sym *near = NULL;
- Elf_Addr distance = ~0;
+ Elf_Addr distance;
for (sym = elf->symtab_start; sym < elf->symtab_stop; sym++) {
if (get_secindex(elf, sym) != secndx)
continue;
if (!is_valid_name(elf, sym))
continue;
- if (sym->st_value <= addr && addr - sym->st_value <= distance) {
+
+ if (addr >= sym->st_value)
distance = addr - sym->st_value;
+ else if (allow_negative)
+ distance = sym->st_value - addr;
+ else
+ continue;
+
+ if (distance <= min_distance) {
+ min_distance = distance;
near = sym;
}
+
+ if (min_distance == 0)
+ break;
}
return near;
}
+static Elf_Sym *find_fromsym(struct elf_info *elf, Elf_Addr addr,
+ unsigned int secndx)
+{
+ return find_nearest_sym(elf, addr, secndx, false, ~0);
+}
+
+static Elf_Sym *find_tosym(struct elf_info *elf, Elf_Addr addr, Elf_Sym *sym)
+{
+ /* If the supplied symbol has a valid name, return it */
+ if (is_valid_name(elf, sym))
+ return sym;
+
+ /*
+ * Strive to find a better symbol name, but the resulting name does not
+ * always match the symbol referenced in the original code.
+ */
+ return find_nearest_sym(elf, addr, get_secindex(elf, sym), true, 20);
+}
+
static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
{
if (secndx > elf->num_sections)
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 05/20] modpost: refactor find_fromsym() and find_tosym()
2023-05-21 16:04 ` [PATCH v6 05/20] modpost: refactor find_fromsym() and find_tosym() Masahiro Yamada
@ 2023-05-22 18:18 ` Nick Desaulniers
0 siblings, 0 replies; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-22 18:18 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> find_fromsym() and find_tosym() are similar - both of them iterate
> in the .symtab section and return the nearest symbol.
>
> The difference between them is that find_tosym() allows a negative
> distance, but the distance must be less than 20.
>
> Factor out the common part into find_nearest_sym().
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>
> Changes in v6:
> - Revive the check for distance less than 20
>
> scripts/mod/modpost.c | 95 ++++++++++++++++---------------------------
> 1 file changed, 36 insertions(+), 59 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index d2329ac32177..6ac0d571542c 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1104,81 +1104,58 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
> return !is_mapping_symbol(name);
> }
>
> -/**
> - * Find symbol based on relocation record info.
> - * In some cases the symbol supplied is a valid symbol so
> - * return refsym. If is_valid_name() == true, we assume this is a valid symbol.
> - * In other cases the symbol needs to be looked up in the symbol table
> - * based on section and address.
> - * **/
> -static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
> - Elf_Sym *relsym)
> +/* Look up the nearest symbol based on the section and the address */
> +static Elf_Sym *find_nearest_sym(struct elf_info *elf, Elf_Addr addr,
> + unsigned int secndx, bool allow_negative,
> + Elf_Addr min_distance)
> {
> Elf_Sym *sym;
> Elf_Sym *near = NULL;
> - Elf64_Sword distance = 20;
> - Elf64_Sword d;
> - unsigned int relsym_secindex;
> -
> - if (is_valid_name(elf, relsym))
> - return relsym;
> -
> - /*
> - * Strive to find a better symbol name, but the resulting name does not
> - * always match the symbol referenced in the original code.
> - */
> - relsym_secindex = get_secindex(elf, relsym);
> - for (sym = elf->symtab_start; sym < elf->symtab_stop; sym++) {
> - if (get_secindex(elf, sym) != relsym_secindex)
> - continue;
> - if (ELF_ST_TYPE(sym->st_info) == STT_SECTION)
> - continue;
> - if (!is_valid_name(elf, sym))
> - continue;
> - if (sym->st_value == addr)
> - return sym;
> - /* Find a symbol nearby - addr are maybe negative */
> - d = sym->st_value - addr;
> - if (d < 0)
> - d = addr - sym->st_value;
> - if (d < distance) {
> - distance = d;
> - near = sym;
> - }
> - }
> - /* We need a close match */
> - if (distance < 20)
> - return near;
> - else
> - return NULL;
> -}
> -
> -/*
> - * Find symbols before or equal addr and after addr - in the section sec.
> - * If we find two symbols with equal offset prefer one with a valid name.
> - * The ELF format may have a better way to detect what type of symbol
> - * it is, but this works for now.
> - **/
> -static Elf_Sym *find_fromsym(struct elf_info *elf, Elf_Addr addr,
> - unsigned int secndx)
> -{
> - Elf_Sym *sym;
> - Elf_Sym *near = NULL;
> - Elf_Addr distance = ~0;
> + Elf_Addr distance;
>
> for (sym = elf->symtab_start; sym < elf->symtab_stop; sym++) {
> if (get_secindex(elf, sym) != secndx)
> continue;
> if (!is_valid_name(elf, sym))
> continue;
> - if (sym->st_value <= addr && addr - sym->st_value <= distance) {
> +
> + if (addr >= sym->st_value)
> distance = addr - sym->st_value;
> + else if (allow_negative)
> + distance = sym->st_value - addr;
> + else
> + continue;
> +
> + if (distance <= min_distance) {
> + min_distance = distance;
> near = sym;
> }
> +
> + if (min_distance == 0)
> + break;
> }
> return near;
> }
>
> +static Elf_Sym *find_fromsym(struct elf_info *elf, Elf_Addr addr,
> + unsigned int secndx)
> +{
> + return find_nearest_sym(elf, addr, secndx, false, ~0);
> +}
> +
> +static Elf_Sym *find_tosym(struct elf_info *elf, Elf_Addr addr, Elf_Sym *sym)
> +{
> + /* If the supplied symbol has a valid name, return it */
> + if (is_valid_name(elf, sym))
> + return sym;
> +
> + /*
> + * Strive to find a better symbol name, but the resulting name does not
> + * always match the symbol referenced in the original code.
> + */
> + return find_nearest_sym(elf, addr, get_secindex(elf, sym), true, 20);
> +}
> +
> static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
> {
> if (secndx > elf->num_sections)
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 06/20] modpost: unify 'sym' and 'to' in default_mismatch_handler()
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (4 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 05/20] modpost: refactor find_fromsym() and find_tosym() Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-22 18:23 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 07/20] modpost: replace r->r_offset, r->r_addend with faddr, taddr Masahiro Yamada
` (14 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
find_tosym() takes 'sym' and stores the return value to another
variable 'to'. You can use the same variable because we want to
replace the original one when appropriate.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 6ac0d571542c..7848bacd4b42 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1166,11 +1166,10 @@ static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
static void default_mismatch_handler(const char *modname, struct elf_info *elf,
const struct sectioncheck* const mismatch,
- Elf_Rela *r, Elf_Sym *sym,
+ Elf_Rela *r, Elf_Sym *tsym,
unsigned int fsecndx, const char *fromsec,
const char *tosec)
{
- Elf_Sym *to;
Elf_Sym *from;
const char *tosym;
const char *fromsym;
@@ -1178,8 +1177,8 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
from = find_fromsym(elf, r->r_offset, fsecndx);
fromsym = sym_name(elf, from);
- to = find_tosym(elf, r->r_addend, sym);
- tosym = sym_name(elf, to);
+ tsym = find_tosym(elf, r->r_addend, tsym);
+ tosym = sym_name(elf, tsym);
/* check whitelist - we may ignore it */
if (!secref_whitelist(fromsec, fromsym, tosec, tosym))
@@ -1214,7 +1213,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
"You might get more information about where this is\n"
"coming from by using scripts/check_extable.sh %s\n",
fromsec, (long)r->r_offset, tosec, modname);
- else if (is_executable_section(elf, get_secindex(elf, sym)))
+ else if (is_executable_section(elf, get_secindex(elf, tsym)))
warn("The relocation at %s+0x%lx references\n"
"section \"%s\" which is not in the list of\n"
"authorized sections. If you're adding a new section\n"
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 06/20] modpost: unify 'sym' and 'to' in default_mismatch_handler()
2023-05-21 16:04 ` [PATCH v6 06/20] modpost: unify 'sym' and 'to' in default_mismatch_handler() Masahiro Yamada
@ 2023-05-22 18:23 ` Nick Desaulniers
0 siblings, 0 replies; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-22 18:23 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> find_tosym() takes 'sym' and stores the return value to another
> variable 'to'. You can use the same variable because we want to
> replace the original one when appropriate.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>
> scripts/mod/modpost.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 6ac0d571542c..7848bacd4b42 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1166,11 +1166,10 @@ static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
>
> static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> const struct sectioncheck* const mismatch,
> - Elf_Rela *r, Elf_Sym *sym,
> + Elf_Rela *r, Elf_Sym *tsym,
> unsigned int fsecndx, const char *fromsec,
> const char *tosec)
> {
> - Elf_Sym *to;
> Elf_Sym *from;
> const char *tosym;
> const char *fromsym;
> @@ -1178,8 +1177,8 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> from = find_fromsym(elf, r->r_offset, fsecndx);
> fromsym = sym_name(elf, from);
>
> - to = find_tosym(elf, r->r_addend, sym);
> - tosym = sym_name(elf, to);
> + tsym = find_tosym(elf, r->r_addend, tsym);
> + tosym = sym_name(elf, tsym);
>
> /* check whitelist - we may ignore it */
> if (!secref_whitelist(fromsec, fromsym, tosec, tosym))
> @@ -1214,7 +1213,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> "You might get more information about where this is\n"
> "coming from by using scripts/check_extable.sh %s\n",
> fromsec, (long)r->r_offset, tosec, modname);
> - else if (is_executable_section(elf, get_secindex(elf, sym)))
> + else if (is_executable_section(elf, get_secindex(elf, tsym)))
> warn("The relocation at %s+0x%lx references\n"
> "section \"%s\" which is not in the list of\n"
> "authorized sections. If you're adding a new section\n"
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 07/20] modpost: replace r->r_offset, r->r_addend with faddr, taddr
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (5 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 06/20] modpost: unify 'sym' and 'to' in default_mismatch_handler() Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-22 18:31 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 08/20] modpost: remove is_shndx_special() check from section_rel(a) Masahiro Yamada
` (13 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
r_offset/r_addend holds the offset address from/to which a symbol is
referenced. It is unclear unless you are familiar with ELF.
Rename them to faddr, taddr, respectively. The prefix 'f' means 'from',
't' means 'to'.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 7848bacd4b42..f364738a236e 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1166,18 +1166,18 @@ static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
static void default_mismatch_handler(const char *modname, struct elf_info *elf,
const struct sectioncheck* const mismatch,
- Elf_Rela *r, Elf_Sym *tsym,
- unsigned int fsecndx, const char *fromsec,
- const char *tosec)
+ Elf_Sym *tsym,
+ unsigned int fsecndx, const char *fromsec, Elf_Addr faddr,
+ const char *tosec, Elf_Addr taddr)
{
Elf_Sym *from;
const char *tosym;
const char *fromsym;
- from = find_fromsym(elf, r->r_offset, fsecndx);
+ from = find_fromsym(elf, faddr, fsecndx);
fromsym = sym_name(elf, from);
- tsym = find_tosym(elf, r->r_addend, tsym);
+ tsym = find_tosym(elf, taddr, tsym);
tosym = sym_name(elf, tsym);
/* check whitelist - we may ignore it */
@@ -1204,7 +1204,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
break;
case EXTABLE_TO_NON_TEXT:
warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
- modname, fromsec, (long)r->r_offset, tosec, tosym);
+ modname, fromsec, (long)faddr, tosec, tosym);
if (match(tosec, mismatch->bad_tosec))
fatal("The relocation at %s+0x%lx references\n"
@@ -1212,7 +1212,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
"Something is seriously wrong and should be fixed.\n"
"You might get more information about where this is\n"
"coming from by using scripts/check_extable.sh %s\n",
- fromsec, (long)r->r_offset, tosec, modname);
+ fromsec, (long)faddr, tosec, modname);
else if (is_executable_section(elf, get_secindex(elf, tsym)))
warn("The relocation at %s+0x%lx references\n"
"section \"%s\" which is not in the list of\n"
@@ -1221,17 +1221,18 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
"list of authorized sections to jump to on fault.\n"
"This can be achieved by adding \"%s\" to\n"
"OTHER_TEXT_SECTIONS in scripts/mod/modpost.c.\n",
- fromsec, (long)r->r_offset, tosec, tosec, tosec);
+ fromsec, (long)faddr, tosec, tosec, tosec);
else
error("%s+0x%lx references non-executable section '%s'\n",
- fromsec, (long)r->r_offset, tosec);
+ fromsec, (long)faddr, tosec);
break;
}
}
static void check_section_mismatch(const char *modname, struct elf_info *elf,
- Elf_Rela *r, Elf_Sym *sym,
- unsigned int fsecndx, const char *fromsec)
+ Elf_Sym *sym,
+ unsigned int fsecndx, const char *fromsec,
+ Elf_Addr faddr, Elf_Addr taddr)
{
const char *tosec = sec_name(elf, get_secindex(elf, sym));
const struct sectioncheck *mismatch = section_mismatch(fromsec, tosec);
@@ -1239,8 +1240,9 @@ static void check_section_mismatch(const char *modname, struct elf_info *elf,
if (!mismatch)
return;
- default_mismatch_handler(modname, elf, mismatch, r, sym, fsecndx, fromsec,
- tosec);
+ default_mismatch_handler(modname, elf, mismatch, sym,
+ fsecndx, fromsec, faddr,
+ tosec, taddr);
}
static unsigned int *reloc_location(struct elf_info *elf,
@@ -1406,7 +1408,8 @@ static void section_rela(const char *modname, struct elf_info *elf,
/* Skip special sections */
if (is_shndx_special(sym->st_shndx))
continue;
- check_section_mismatch(modname, elf, &r, sym, fsecndx, fromsec);
+ check_section_mismatch(modname, elf, sym,
+ fsecndx, fromsec, r.r_offset, r.r_addend);
}
}
@@ -1464,7 +1467,8 @@ static void section_rel(const char *modname, struct elf_info *elf,
/* Skip special sections */
if (is_shndx_special(sym->st_shndx))
continue;
- check_section_mismatch(modname, elf, &r, sym, fsecndx, fromsec);
+ check_section_mismatch(modname, elf, sym,
+ fsecndx, fromsec, r.r_offset, r.r_addend);
}
}
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 07/20] modpost: replace r->r_offset, r->r_addend with faddr, taddr
2023-05-21 16:04 ` [PATCH v6 07/20] modpost: replace r->r_offset, r->r_addend with faddr, taddr Masahiro Yamada
@ 2023-05-22 18:31 ` Nick Desaulniers
0 siblings, 0 replies; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-22 18:31 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> r_offset/r_addend holds the offset address from/to which a symbol is
> referenced. It is unclear unless you are familiar with ELF.
>
> Rename them to faddr, taddr, respectively. The prefix 'f' means 'from',
> 't' means 'to'.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Do you care to rewrap the parameter lists?
```
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 4da96746a03b..8a787521963d 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1115,8 +1115,8 @@ static bool is_executable_section(struct
elf_info *elf, unsigned int secndx)
static void default_mismatch_handler(const char *modname, struct elf_info *elf,
const struct sectioncheck* const mismatch,
- Elf_Sym *tsym,
- unsigned int fsecndx, const char
*fromsec, Elf_Addr faddr,
+ Elf_Sym *tsym, unsigned int fsecndx,
+ const char *fromsec, Elf_Addr faddr,
const char *tosec, Elf_Addr taddr)
{
Elf_Sym *from;
@@ -1217,9 +1217,9 @@ static void check_export_symbol(struct module
*mod, struct elf_info *elf,
}
static void check_section_mismatch(struct module *mod, struct elf_info *elf,
- Elf_Sym *sym,
- unsigned int fsecndx, const char *fromsec,
- Elf_Addr faddr, Elf_Addr taddr)
+ Elf_Sym *sym, unsigned int fsecndx,
+ const char *fromsec, Elf_Addr faddr,
+ Elf_Addr taddr)
{
const char *tosec = sec_name(elf, get_secindex(elf, sym));
const struct sectioncheck *mismatch;
```
Either way:
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>
> scripts/mod/modpost.c | 34 +++++++++++++++++++---------------
> 1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 7848bacd4b42..f364738a236e 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1166,18 +1166,18 @@ static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
>
> static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> const struct sectioncheck* const mismatch,
> - Elf_Rela *r, Elf_Sym *tsym,
> - unsigned int fsecndx, const char *fromsec,
> - const char *tosec)
> + Elf_Sym *tsym,
> + unsigned int fsecndx, const char *fromsec, Elf_Addr faddr,
> + const char *tosec, Elf_Addr taddr)
> {
> Elf_Sym *from;
> const char *tosym;
> const char *fromsym;
>
> - from = find_fromsym(elf, r->r_offset, fsecndx);
> + from = find_fromsym(elf, faddr, fsecndx);
> fromsym = sym_name(elf, from);
>
> - tsym = find_tosym(elf, r->r_addend, tsym);
> + tsym = find_tosym(elf, taddr, tsym);
> tosym = sym_name(elf, tsym);
>
> /* check whitelist - we may ignore it */
> @@ -1204,7 +1204,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> break;
> case EXTABLE_TO_NON_TEXT:
> warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
> - modname, fromsec, (long)r->r_offset, tosec, tosym);
> + modname, fromsec, (long)faddr, tosec, tosym);
>
> if (match(tosec, mismatch->bad_tosec))
> fatal("The relocation at %s+0x%lx references\n"
> @@ -1212,7 +1212,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> "Something is seriously wrong and should be fixed.\n"
> "You might get more information about where this is\n"
> "coming from by using scripts/check_extable.sh %s\n",
> - fromsec, (long)r->r_offset, tosec, modname);
> + fromsec, (long)faddr, tosec, modname);
> else if (is_executable_section(elf, get_secindex(elf, tsym)))
> warn("The relocation at %s+0x%lx references\n"
> "section \"%s\" which is not in the list of\n"
> @@ -1221,17 +1221,18 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> "list of authorized sections to jump to on fault.\n"
> "This can be achieved by adding \"%s\" to\n"
> "OTHER_TEXT_SECTIONS in scripts/mod/modpost.c.\n",
> - fromsec, (long)r->r_offset, tosec, tosec, tosec);
> + fromsec, (long)faddr, tosec, tosec, tosec);
> else
> error("%s+0x%lx references non-executable section '%s'\n",
> - fromsec, (long)r->r_offset, tosec);
> + fromsec, (long)faddr, tosec);
> break;
> }
> }
>
> static void check_section_mismatch(const char *modname, struct elf_info *elf,
> - Elf_Rela *r, Elf_Sym *sym,
> - unsigned int fsecndx, const char *fromsec)
> + Elf_Sym *sym,
> + unsigned int fsecndx, const char *fromsec,
> + Elf_Addr faddr, Elf_Addr taddr)
> {
> const char *tosec = sec_name(elf, get_secindex(elf, sym));
> const struct sectioncheck *mismatch = section_mismatch(fromsec, tosec);
> @@ -1239,8 +1240,9 @@ static void check_section_mismatch(const char *modname, struct elf_info *elf,
> if (!mismatch)
> return;
>
> - default_mismatch_handler(modname, elf, mismatch, r, sym, fsecndx, fromsec,
> - tosec);
> + default_mismatch_handler(modname, elf, mismatch, sym,
> + fsecndx, fromsec, faddr,
> + tosec, taddr);
> }
>
> static unsigned int *reloc_location(struct elf_info *elf,
> @@ -1406,7 +1408,8 @@ static void section_rela(const char *modname, struct elf_info *elf,
> /* Skip special sections */
> if (is_shndx_special(sym->st_shndx))
> continue;
> - check_section_mismatch(modname, elf, &r, sym, fsecndx, fromsec);
> + check_section_mismatch(modname, elf, sym,
> + fsecndx, fromsec, r.r_offset, r.r_addend);
> }
> }
>
> @@ -1464,7 +1467,8 @@ static void section_rel(const char *modname, struct elf_info *elf,
> /* Skip special sections */
> if (is_shndx_special(sym->st_shndx))
> continue;
> - check_section_mismatch(modname, elf, &r, sym, fsecndx, fromsec);
> + check_section_mismatch(modname, elf, sym,
> + fsecndx, fromsec, r.r_offset, r.r_addend);
> }
> }
>
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply related [flat|nested] 51+ messages in thread
* [PATCH v6 08/20] modpost: remove is_shndx_special() check from section_rel(a)
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (6 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 07/20] modpost: replace r->r_offset, r->r_addend with faddr, taddr Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-25 17:20 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 09/20] modpost: pass struct module pointer to check_section_mismatch() Masahiro Yamada
` (12 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
This check is unneeded. Without it, sec_name() will returns the null
string "", then section_mismatch() will return immediately.
Anyway, special section indices rarely appear in these loops.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
Changes in v6:
- Remove is_shndx_special() definition
scripts/mod/modpost.c | 16 ++++------------
scripts/mod/modpost.h | 5 -----
2 files changed, 4 insertions(+), 17 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f364738a236e..40fac4f64fcd 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1361,7 +1361,6 @@ static int addend_mips_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
static void section_rela(const char *modname, struct elf_info *elf,
Elf_Shdr *sechdr)
{
- Elf_Sym *sym;
Elf_Rela *rela;
Elf_Rela r;
unsigned int r_sym;
@@ -1404,11 +1403,8 @@ static void section_rela(const char *modname, struct elf_info *elf,
continue;
break;
}
- sym = elf->symtab_start + r_sym;
- /* Skip special sections */
- if (is_shndx_special(sym->st_shndx))
- continue;
- check_section_mismatch(modname, elf, sym,
+
+ check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
fsecndx, fromsec, r.r_offset, r.r_addend);
}
}
@@ -1416,7 +1412,6 @@ static void section_rela(const char *modname, struct elf_info *elf,
static void section_rel(const char *modname, struct elf_info *elf,
Elf_Shdr *sechdr)
{
- Elf_Sym *sym;
Elf_Rel *rel;
Elf_Rela r;
unsigned int r_sym;
@@ -1463,11 +1458,8 @@ static void section_rel(const char *modname, struct elf_info *elf,
default:
fatal("Please add code to calculate addend for this architecture\n");
}
- sym = elf->symtab_start + r_sym;
- /* Skip special sections */
- if (is_shndx_special(sym->st_shndx))
- continue;
- check_section_mismatch(modname, elf, sym,
+
+ check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
fsecndx, fromsec, r.r_offset, r.r_addend);
}
}
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 1178f40a73f3..b1e2d95f8047 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -151,11 +151,6 @@ struct elf_info {
Elf32_Word *symtab_shndx_stop;
};
-static inline int is_shndx_special(unsigned int i)
-{
- return i != SHN_XINDEX && i >= SHN_LORESERVE && i <= SHN_HIRESERVE;
-}
-
/* Accessor for sym->st_shndx, hides ugliness of "64k sections" */
static inline unsigned int get_secindex(const struct elf_info *info,
const Elf_Sym *sym)
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 08/20] modpost: remove is_shndx_special() check from section_rel(a)
2023-05-21 16:04 ` [PATCH v6 08/20] modpost: remove is_shndx_special() check from section_rel(a) Masahiro Yamada
@ 2023-05-25 17:20 ` Nick Desaulniers
0 siblings, 0 replies; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-25 17:20 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> This check is unneeded. Without it, sec_name() will returns the null
> string "", then section_mismatch() will return immediately.
>
> Anyway, special section indices rarely appear in these loops.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>
> Changes in v6:
> - Remove is_shndx_special() definition
>
> scripts/mod/modpost.c | 16 ++++------------
> scripts/mod/modpost.h | 5 -----
> 2 files changed, 4 insertions(+), 17 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index f364738a236e..40fac4f64fcd 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1361,7 +1361,6 @@ static int addend_mips_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
> static void section_rela(const char *modname, struct elf_info *elf,
> Elf_Shdr *sechdr)
> {
> - Elf_Sym *sym;
> Elf_Rela *rela;
> Elf_Rela r;
> unsigned int r_sym;
> @@ -1404,11 +1403,8 @@ static void section_rela(const char *modname, struct elf_info *elf,
> continue;
> break;
> }
> - sym = elf->symtab_start + r_sym;
> - /* Skip special sections */
> - if (is_shndx_special(sym->st_shndx))
> - continue;
> - check_section_mismatch(modname, elf, sym,
> +
> + check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
> fsecndx, fromsec, r.r_offset, r.r_addend);
> }
> }
> @@ -1416,7 +1412,6 @@ static void section_rela(const char *modname, struct elf_info *elf,
> static void section_rel(const char *modname, struct elf_info *elf,
> Elf_Shdr *sechdr)
> {
> - Elf_Sym *sym;
> Elf_Rel *rel;
> Elf_Rela r;
> unsigned int r_sym;
> @@ -1463,11 +1458,8 @@ static void section_rel(const char *modname, struct elf_info *elf,
> default:
> fatal("Please add code to calculate addend for this architecture\n");
> }
> - sym = elf->symtab_start + r_sym;
> - /* Skip special sections */
> - if (is_shndx_special(sym->st_shndx))
> - continue;
> - check_section_mismatch(modname, elf, sym,
> +
> + check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
> fsecndx, fromsec, r.r_offset, r.r_addend);
> }
> }
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index 1178f40a73f3..b1e2d95f8047 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -151,11 +151,6 @@ struct elf_info {
> Elf32_Word *symtab_shndx_stop;
> };
>
> -static inline int is_shndx_special(unsigned int i)
> -{
> - return i != SHN_XINDEX && i >= SHN_LORESERVE && i <= SHN_HIRESERVE;
> -}
> -
> /* Accessor for sym->st_shndx, hides ugliness of "64k sections" */
> static inline unsigned int get_secindex(const struct elf_info *info,
> const Elf_Sym *sym)
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 09/20] modpost: pass struct module pointer to check_section_mismatch()
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (7 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 08/20] modpost: remove is_shndx_special() check from section_rel(a) Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-25 17:23 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 10/20] kbuild: generate KSYMTAB entries by modpost Masahiro Yamada
` (11 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
The next commit will use it.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 40fac4f64fcd..378fb9649545 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1229,7 +1229,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
}
}
-static void check_section_mismatch(const char *modname, struct elf_info *elf,
+static void check_section_mismatch(struct module *mod, struct elf_info *elf,
Elf_Sym *sym,
unsigned int fsecndx, const char *fromsec,
Elf_Addr faddr, Elf_Addr taddr)
@@ -1240,7 +1240,7 @@ static void check_section_mismatch(const char *modname, struct elf_info *elf,
if (!mismatch)
return;
- default_mismatch_handler(modname, elf, mismatch, sym,
+ default_mismatch_handler(mod->name, elf, mismatch, sym,
fsecndx, fromsec, faddr,
tosec, taddr);
}
@@ -1358,7 +1358,7 @@ static int addend_mips_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
#define R_LARCH_SUB32 55
#endif
-static void section_rela(const char *modname, struct elf_info *elf,
+static void section_rela(struct module *mod, struct elf_info *elf,
Elf_Shdr *sechdr)
{
Elf_Rela *rela;
@@ -1404,12 +1404,12 @@ static void section_rela(const char *modname, struct elf_info *elf,
break;
}
- check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
+ check_section_mismatch(mod, elf, elf->symtab_start + r_sym,
fsecndx, fromsec, r.r_offset, r.r_addend);
}
}
-static void section_rel(const char *modname, struct elf_info *elf,
+static void section_rel(struct module *mod, struct elf_info *elf,
Elf_Shdr *sechdr)
{
Elf_Rel *rel;
@@ -1459,7 +1459,7 @@ static void section_rel(const char *modname, struct elf_info *elf,
fatal("Please add code to calculate addend for this architecture\n");
}
- check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
+ check_section_mismatch(mod, elf, elf->symtab_start + r_sym,
fsecndx, fromsec, r.r_offset, r.r_addend);
}
}
@@ -1476,19 +1476,19 @@ static void section_rel(const char *modname, struct elf_info *elf,
* to find all references to a section that reference a section that will
* be discarded and warns about it.
**/
-static void check_sec_ref(const char *modname, struct elf_info *elf)
+static void check_sec_ref(struct module *mod, struct elf_info *elf)
{
int i;
Elf_Shdr *sechdrs = elf->sechdrs;
/* Walk through all sections */
for (i = 0; i < elf->num_sections; i++) {
- check_section(modname, elf, &elf->sechdrs[i]);
+ check_section(mod->name, elf, &elf->sechdrs[i]);
/* We want to process only relocation sections and not .init */
if (sechdrs[i].sh_type == SHT_RELA)
- section_rela(modname, elf, &elf->sechdrs[i]);
+ section_rela(mod, elf, &elf->sechdrs[i]);
else if (sechdrs[i].sh_type == SHT_REL)
- section_rel(modname, elf, &elf->sechdrs[i]);
+ section_rel(mod, elf, &elf->sechdrs[i]);
}
}
@@ -1659,7 +1659,7 @@ static void read_symbols(const char *modname)
sym_get_data(&info, sym));
}
- check_sec_ref(modname, &info);
+ check_sec_ref(mod, &info);
if (!mod->is_vmlinux) {
version = get_modinfo(&info, "version");
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 09/20] modpost: pass struct module pointer to check_section_mismatch()
2023-05-21 16:04 ` [PATCH v6 09/20] modpost: pass struct module pointer to check_section_mismatch() Masahiro Yamada
@ 2023-05-25 17:23 ` Nick Desaulniers
0 siblings, 0 replies; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-25 17:23 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> The next commit will use it.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>
> scripts/mod/modpost.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 40fac4f64fcd..378fb9649545 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1229,7 +1229,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> }
> }
>
> -static void check_section_mismatch(const char *modname, struct elf_info *elf,
> +static void check_section_mismatch(struct module *mod, struct elf_info *elf,
> Elf_Sym *sym,
> unsigned int fsecndx, const char *fromsec,
> Elf_Addr faddr, Elf_Addr taddr)
> @@ -1240,7 +1240,7 @@ static void check_section_mismatch(const char *modname, struct elf_info *elf,
> if (!mismatch)
> return;
>
> - default_mismatch_handler(modname, elf, mismatch, sym,
> + default_mismatch_handler(mod->name, elf, mismatch, sym,
> fsecndx, fromsec, faddr,
> tosec, taddr);
> }
> @@ -1358,7 +1358,7 @@ static int addend_mips_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
> #define R_LARCH_SUB32 55
> #endif
>
> -static void section_rela(const char *modname, struct elf_info *elf,
> +static void section_rela(struct module *mod, struct elf_info *elf,
> Elf_Shdr *sechdr)
> {
> Elf_Rela *rela;
> @@ -1404,12 +1404,12 @@ static void section_rela(const char *modname, struct elf_info *elf,
> break;
> }
>
> - check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
> + check_section_mismatch(mod, elf, elf->symtab_start + r_sym,
> fsecndx, fromsec, r.r_offset, r.r_addend);
> }
> }
>
> -static void section_rel(const char *modname, struct elf_info *elf,
> +static void section_rel(struct module *mod, struct elf_info *elf,
> Elf_Shdr *sechdr)
> {
> Elf_Rel *rel;
> @@ -1459,7 +1459,7 @@ static void section_rel(const char *modname, struct elf_info *elf,
> fatal("Please add code to calculate addend for this architecture\n");
> }
>
> - check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
> + check_section_mismatch(mod, elf, elf->symtab_start + r_sym,
> fsecndx, fromsec, r.r_offset, r.r_addend);
> }
> }
> @@ -1476,19 +1476,19 @@ static void section_rel(const char *modname, struct elf_info *elf,
> * to find all references to a section that reference a section that will
> * be discarded and warns about it.
> **/
> -static void check_sec_ref(const char *modname, struct elf_info *elf)
> +static void check_sec_ref(struct module *mod, struct elf_info *elf)
> {
> int i;
> Elf_Shdr *sechdrs = elf->sechdrs;
>
> /* Walk through all sections */
> for (i = 0; i < elf->num_sections; i++) {
> - check_section(modname, elf, &elf->sechdrs[i]);
> + check_section(mod->name, elf, &elf->sechdrs[i]);
> /* We want to process only relocation sections and not .init */
> if (sechdrs[i].sh_type == SHT_RELA)
> - section_rela(modname, elf, &elf->sechdrs[i]);
> + section_rela(mod, elf, &elf->sechdrs[i]);
> else if (sechdrs[i].sh_type == SHT_REL)
> - section_rel(modname, elf, &elf->sechdrs[i]);
> + section_rel(mod, elf, &elf->sechdrs[i]);
> }
> }
>
> @@ -1659,7 +1659,7 @@ static void read_symbols(const char *modname)
> sym_get_data(&info, sym));
> }
>
> - check_sec_ref(modname, &info);
> + check_sec_ref(mod, &info);
>
> if (!mod->is_vmlinux) {
> version = get_modinfo(&info, "version");
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 10/20] kbuild: generate KSYMTAB entries by modpost
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (8 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 09/20] modpost: pass struct module pointer to check_section_mismatch() Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-25 17:50 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 11/20] ia64,export.h: replace EXPORT_DATA_SYMBOL* with EXPORT_SYMBOL* Masahiro Yamada
` (10 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
Commit 7b4537199a4a ("kbuild: link symbol CRCs at final link, removing
CONFIG_MODULE_REL_CRCS") made modpost output CRCs in the same way
whether the EXPORT_SYMBOL() is placed in *.c or *.S.
This commit applies a similar approach to the entire data structure of
EXPORT_SYMBOL() for further cleanups. The EXPORT_SYMBOL() compilation
is split into two stages.
When a source file is compiled, EXPORT_SYMBOL() is converted into a
dummy symbol in the .export_symbol section.
For example,
EXPORT_SYMBOL(foo);
EXPORT_SYMBOL_NS_GPL(bar, BAR_NAMESPACE);
will be encoded into the following assembly code:
.section ".export_symbol","a"
__export_symbol__foo:
.asciz ""
.balign 8
.quad foo
.previous
.section ".export_symbol","a"
__export_symbol_gpl_bar:
.asciz "BAR_NAMESPACE"
.balign 8
.quad bar
.previous
They are just markers to tell modpost the name, license, and namespace
of the symbols. They will be dropped from the final vmlinux and modules
because the *(.export_symbol) will go into /DISCARD/ in the linker script.
Then, modpost extracts all the information about EXPORT_SYMBOL() from the
.export_symbol section, and generates C code:
KSYMTAB_FUNC(foo, "", "");
KSYMTAB_FUNC(bar, "_gpl", "BAR_NAMESPACE");
KSYMTAB_FUNC() (or KSYMTAB_DATA() if it is data) is expanded to struct
kernel_symbol that will be linked to the vmlinux or a module.
With this change, EXPORT_SYMBOL() works in the same way for *.c and *.S
files, providing the following benefits.
[1] Deprecate EXPORT_DATA_SYMBOL()
In the old days, EXPORT_SYMBOL() was only available in C files. To export
a symbol in *.S, EXPORT_SYMBOL() was placed in a separate *.c file.
arch/arm/kernel/armksyms.c is one example written in the classic manner.
Commit 22823ab419d8 ("EXPORT_SYMBOL() for asm") removed this limitation.
Since then, EXPORT_SYMBOL() can be placed close to the symbol definition
in *.S files. It was a nice improvement.
However, as that commit mentioned, you need to use EXPORT_DATA_SYMBOL()
for data objects on some architectures.
In the new approach, modpost checks symbol's type (STT_FUNC or not),
and outputs KSYMTAB_FUNC() or KSYMTAB_DATA() accordingly.
There are only two users of EXPORT_DATA_SYMBOL:
EXPORT_DATA_SYMBOL_GPL(empty_zero_page) (arch/ia64/kernel/head.S)
EXPORT_DATA_SYMBOL(ia64_ivt) (arch/ia64/kernel/ivt.S)
They are transformed as follows and output into .vmlinux.export.c
KSYMTAB_DATA(empty_zero_page, "_gpl", "");
KSYMTAB_DATA(ia64_ivt, "", "");
The other EXPORT_SYMBOL users in ia64 assembly are output as
KSYMTAB_FUNC().
EXPORT_DATA_SYMBOL() is now deprecated.
[2] merge <linux/export.h> and <asm-generic/export.h>
There are two similar header implementations:
include/linux/export.h for .c files
include/asm-generic/export.h for .S files
Ideally, the functionality should be consistent between them, but they
tend to diverge.
Commit 8651ec01daed ("module: add support for symbol namespaces.") did
not support the namespace for *.S files.
This commit shifts the essential implementation part to C, which supports
EXPORT_SYMBOL_NS() for *.S files.
<asm/export.h> and <asm-generic/export.h> will remain as a wrapper of
<linux/export.h> for a while.
They will be removed after #include <asm/export.h> directives are all
replaced with #include <linux/export.h>.
[3] Implement CONFIG_TRIM_UNUSED_KSYMS in one-pass algorithm (by a later commit)
When CONFIG_TRIM_UNUSED_KSYMS is enabled, Kbuild recursively traverses
the directory tree to determine which EXPORT_SYMBOL to trim. If an
EXPORT_SYMBOL turns out to be unused by anyone, Kbuild begins the
second traverse, where some source files are recompiled with their
EXPORT_SYMBOL() tuned into a no-op.
We can do this better now; modpost can selectively emit KSYMTAB entries
that are really used by modules.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
Changes in v6:
- Fix build error on UML
Changes in v5:
- Fix build error on ARM
Changes in v4:
- Version 3 did not work if a same name symbol exists in a different compilation unit
Fix it.
Changes in v3:
- Move struct kernel_symbol to kernel/module/internal.h
Changes in v2:
- Use KSYMTAB_FUNC and KSYMTAB_DATA for functions and data, respectively
This distinction is needed for ia64.
arch/ia64/include/asm/Kbuild | 1 +
arch/ia64/include/asm/export.h | 3 -
arch/um/os-Linux/user_syms.c | 9 ++-
include/asm-generic/export.h | 84 ++-----------------------
include/asm-generic/vmlinux.lds.h | 1 +
include/linux/export-internal.h | 49 +++++++++++++++
include/linux/export.h | 98 +++++++++++------------------
include/linux/pm.h | 8 +--
kernel/module/internal.h | 12 ++++
scripts/Makefile.build | 8 +--
scripts/check-local-export | 4 +-
scripts/mod/modpost.c | 100 ++++++++++++++++++++----------
scripts/mod/modpost.h | 1 +
13 files changed, 188 insertions(+), 190 deletions(-)
delete mode 100644 arch/ia64/include/asm/export.h
diff --git a/arch/ia64/include/asm/Kbuild b/arch/ia64/include/asm/Kbuild
index aefae2efde9f..33733245f42b 100644
--- a/arch/ia64/include/asm/Kbuild
+++ b/arch/ia64/include/asm/Kbuild
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
generated-y += syscall_table.h
generic-y += agp.h
+generic-y += export.h
generic-y += kvm_para.h
generic-y += mcs_spinlock.h
generic-y += vtime.h
diff --git a/arch/ia64/include/asm/export.h b/arch/ia64/include/asm/export.h
deleted file mode 100644
index ad18c6583252..000000000000
--- a/arch/ia64/include/asm/export.h
+++ /dev/null
@@ -1,3 +0,0 @@
-/* EXPORT_DATA_SYMBOL != EXPORT_SYMBOL here */
-#define KSYM_FUNC(name) @fptr(name)
-#include <asm-generic/export.h>
diff --git a/arch/um/os-Linux/user_syms.c b/arch/um/os-Linux/user_syms.c
index 9b62a9d352b3..0b3a82aedbfa 100644
--- a/arch/um/os-Linux/user_syms.c
+++ b/arch/um/os-Linux/user_syms.c
@@ -38,10 +38,15 @@ EXPORT_SYMBOL(vsyscall_end);
#endif
/* Export symbols used by GCC for the stack protector. */
-extern void __stack_smash_handler(void *) __attribute__((weak));
+/*
+ * REVISIT:
+ * ARCH=um cannot enable CONFIG_STACKPROTECTOR because
+ * 'select HAVE_STACKPROTECTOR' is missing. Is this already broken?
+ */
+void (*__stack_smash_handler)(void *) __attribute__((weak));
EXPORT_SYMBOL(__stack_smash_handler);
-extern long __guard __attribute__((weak));
+long __guard __attribute__((weak));
EXPORT_SYMBOL(__guard);
#ifdef _FORTIFY_SOURCE
diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
index 5e4b1f2369d2..0ae9f38a904c 100644
--- a/include/asm-generic/export.h
+++ b/include/asm-generic/export.h
@@ -3,86 +3,12 @@
#define __ASM_GENERIC_EXPORT_H
/*
- * This comment block is used by fixdep. Please do not remove.
- *
- * When CONFIG_MODVERSIONS is changed from n to y, all source files having
- * EXPORT_SYMBOL variants must be re-compiled because genksyms is run as a
- * side effect of the *.o build rule.
+ * <asm/export.h> and <asm-generic/export.h> are deprecated.
+ * Please include <linux/export.h> directly.
*/
+#include <linux/export.h>
-#ifndef KSYM_FUNC
-#define KSYM_FUNC(x) x
-#endif
-#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
-#define KSYM_ALIGN 4
-#elif defined(CONFIG_64BIT)
-#define KSYM_ALIGN 8
-#else
-#define KSYM_ALIGN 4
-#endif
-
-.macro __put, val, name
-#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
- .long \val - ., \name - ., 0
-#elif defined(CONFIG_64BIT)
- .quad \val, \name, 0
-#else
- .long \val, \name, 0
-#endif
-.endm
-
-/*
- * note on .section use: we specify progbits since usage of the "M" (SHF_MERGE)
- * section flag requires it. Use '%progbits' instead of '@progbits' since the
- * former apparently works on all arches according to the binutils source.
- */
-
-.macro ___EXPORT_SYMBOL name,val,sec
-#if defined(CONFIG_MODULES) && !defined(__DISABLE_EXPORTS)
- .section ___ksymtab\sec+\name,"a"
- .balign KSYM_ALIGN
-__ksymtab_\name:
- __put \val, __kstrtab_\name
- .previous
- .section __ksymtab_strings,"aMS",%progbits,1
-__kstrtab_\name:
- .asciz "\name"
- .previous
-#endif
-.endm
-
-#if defined(CONFIG_TRIM_UNUSED_KSYMS)
-
-#include <linux/kconfig.h>
-#include <generated/autoksyms.h>
-
-.macro __ksym_marker sym
- .section ".discard.ksym","a"
-__ksym_marker_\sym:
- .previous
-.endm
-
-#define __EXPORT_SYMBOL(sym, val, sec) \
- __ksym_marker sym; \
- __cond_export_sym(sym, val, sec, __is_defined(__KSYM_##sym))
-#define __cond_export_sym(sym, val, sec, conf) \
- ___cond_export_sym(sym, val, sec, conf)
-#define ___cond_export_sym(sym, val, sec, enabled) \
- __cond_export_sym_##enabled(sym, val, sec)
-#define __cond_export_sym_1(sym, val, sec) ___EXPORT_SYMBOL sym, val, sec
-#define __cond_export_sym_0(sym, val, sec) /* nothing */
-
-#else
-#define __EXPORT_SYMBOL(sym, val, sec) ___EXPORT_SYMBOL sym, val, sec
-#endif
-
-#define EXPORT_SYMBOL(name) \
- __EXPORT_SYMBOL(name, KSYM_FUNC(name),)
-#define EXPORT_SYMBOL_GPL(name) \
- __EXPORT_SYMBOL(name, KSYM_FUNC(name), _gpl)
-#define EXPORT_DATA_SYMBOL(name) \
- __EXPORT_SYMBOL(name, name,)
-#define EXPORT_DATA_SYMBOL_GPL(name) \
- __EXPORT_SYMBOL(name, name,_gpl)
+#define EXPORT_DATA_SYMBOL(name) EXPORT_SYMBOL(name)
+#define EXPORT_DATA_SYMBOL_GPL(name) EXPORT_SYMBOL_GPL(name)
#endif
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index d1f57e4868ed..e65d55e8819c 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -1006,6 +1006,7 @@
PATCHABLE_DISCARDS \
*(.discard) \
*(.discard.*) \
+ *(.export_symbol) \
*(.modinfo) \
/* ld.bfd warns about .gnu.version* even when not emitted */ \
*(.gnu.version*) \
diff --git a/include/linux/export-internal.h b/include/linux/export-internal.h
index fe7e6ba918f1..1c849db953a5 100644
--- a/include/linux/export-internal.h
+++ b/include/linux/export-internal.h
@@ -10,6 +10,55 @@
#include <linux/compiler.h>
#include <linux/types.h>
+#if defined(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)
+/*
+ * relative reference: this reduces the size by half on 64-bit architectures,
+ * and eliminates the need for absolute relocations that require runtime
+ * processing on relocatable kernels.
+ */
+#define __KSYM_REF(sym) ".long " #sym "- ."
+#elif defined(CONFIG_64BIT)
+#define __KSYM_REF(sym) ".quad " #sym
+#else
+#define __KSYM_REF(sym) ".long " #sym
+#endif
+
+/*
+ * For every exported symbol, do the following:
+ *
+ * - Put the name of the symbol and namespace (empty string "" for none) in
+ * __ksymtab_strings.
+ * - Place a struct kernel_symbol entry in the __ksymtab section.
+ *
+ * Note on .section use: we specify progbits since usage of the "M" (SHF_MERGE)
+ * section flag requires it. Use '%progbits' instead of '@progbits' since the
+ * former apparently works on all arches according to the binutils source.
+ */
+#define __KSYMTAB(name, sym, sec, ns) \
+ asm(" .section \"__ksymtab_strings\",\"aMS\",%progbits,1" "\n" \
+ "__kstrtab_" #name ":" "\n" \
+ " .asciz \"" #name "\"" "\n" \
+ "__kstrtabns_" #name ":" "\n" \
+ " .asciz \"" ns "\"" "\n" \
+ " .previous" "\n" \
+ " .section \"___ksymtab" sec "+" #name "\", \"a\"" "\n" \
+ " .balign 4" "\n" \
+ "__ksymtab_" #name ":" "\n" \
+ __KSYM_REF(sym) "\n" \
+ __KSYM_REF(__kstrtab_ ##name) "\n" \
+ __KSYM_REF(__kstrtabns_ ##name) "\n" \
+ " .previous" "\n" \
+ )
+
+#ifdef CONFIG_IA64
+#define KSYM_FUNC(name) @fptr(name)
+#else
+#define KSYM_FUNC(name) name
+#endif
+
+#define KSYMTAB_FUNC(name, sec, ns) __KSYMTAB(name, KSYM_FUNC(name), sec, ns)
+#define KSYMTAB_DATA(name, sec, ns) __KSYMTAB(name, name, sec, ns)
+
#define SYMBOL_CRC(sym, crc, sec) \
asm(".section \"___kcrctab" sec "+" #sym "\",\"a\"" "\n" \
"__crc_" #sym ":" "\n" \
diff --git a/include/linux/export.h b/include/linux/export.h
index 3f31ced0d977..32461a01608c 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -2,6 +2,7 @@
#ifndef _LINUX_EXPORT_H
#define _LINUX_EXPORT_H
+#include <linux/compiler.h>
#include <linux/stringify.h>
/*
@@ -28,72 +29,35 @@ extern struct module __this_module;
#else
#define THIS_MODULE ((struct module *)0)
#endif
+#endif /* __ASSEMBLY__ */
-#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
-#include <linux/compiler.h>
-/*
- * Emit the ksymtab entry as a pair of relative references: this reduces
- * the size by half on 64-bit architectures, and eliminates the need for
- * absolute relocations that require runtime processing on relocatable
- * kernels.
- */
-#define __KSYMTAB_ENTRY(sym, sec) \
- __ADDRESSABLE(sym) \
- asm(" .section \"___ksymtab" sec "+" #sym "\", \"a\" \n" \
- " .balign 4 \n" \
- "__ksymtab_" #sym ": \n" \
- " .long " #sym "- . \n" \
- " .long __kstrtab_" #sym "- . \n" \
- " .long __kstrtabns_" #sym "- . \n" \
- " .previous \n")
-
-struct kernel_symbol {
- int value_offset;
- int name_offset;
- int namespace_offset;
-};
+#ifdef CONFIG_64BIT
+#define __EXPORT_SYMBOL_REF(sym) .balign 8; .quad sym
#else
-#define __KSYMTAB_ENTRY(sym, sec) \
- static const struct kernel_symbol __ksymtab_##sym \
- __attribute__((section("___ksymtab" sec "+" #sym), used)) \
- __aligned(sizeof(void *)) \
- = { (unsigned long)&sym, __kstrtab_##sym, __kstrtabns_##sym }
-
-struct kernel_symbol {
- unsigned long value;
- const char *name;
- const char *namespace;
-};
+#define __EXPORT_SYMBOL_REF(sym) .balign 4; .long sym
#endif
+#define ____EXPORT_SYMBOL(sym, license, ns) \
+ .section ".export_symbol","a" ; \
+ __export_symbol_##license##_##sym: ; \
+ .asciz ns ; \
+ __EXPORT_SYMBOL_REF(sym) ; \
+ .previous
+
#ifdef __GENKSYMS__
#define ___EXPORT_SYMBOL(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
+#elif defined(__ASSEMBLY__)
+
+#define ___EXPORT_SYMBOL(sym, license, ns) \
+ ____EXPORT_SYMBOL(sym, license, ns)
+
#else
-/*
- * For every exported symbol, do the following:
- *
- * - Put the name of the symbol and namespace (empty string "" for none) in
- * __ksymtab_strings.
- * - Place a struct kernel_symbol entry in the __ksymtab section.
- *
- * note on .section use: we specify progbits since usage of the "M" (SHF_MERGE)
- * section flag requires it. Use '%progbits' instead of '@progbits' since the
- * former apparently works on all arches according to the binutils source.
- */
-#define ___EXPORT_SYMBOL(sym, sec, ns) \
- extern typeof(sym) sym; \
- extern const char __kstrtab_##sym[]; \
- extern const char __kstrtabns_##sym[]; \
- asm(" .section \"__ksymtab_strings\",\"aMS\",%progbits,1 \n" \
- "__kstrtab_" #sym ": \n" \
- " .asciz \"" #sym "\" \n" \
- "__kstrtabns_" #sym ": \n" \
- " .asciz \"" ns "\" \n" \
- " .previous \n"); \
- __KSYMTAB_ENTRY(sym, sec)
+#define ___EXPORT_SYMBOL(sym, license, ns) \
+ __ADDRESSABLE(sym) \
+ asm(__stringify(____EXPORT_SYMBOL(sym, license, ns)))
#endif
@@ -117,9 +81,21 @@ struct kernel_symbol {
* from the $(NM) output (see scripts/gen_ksymdeps.sh). These symbols are
* discarded in the final link stage.
*/
+
+#ifdef __ASSEMBLY__
+
+#define __ksym_marker(sym) \
+ .section ".discard.ksym","a" ; \
+__ksym_marker_##sym: ; \
+ .previous
+
+#else
+
#define __ksym_marker(sym) \
static int __ksym_marker_##sym[0] __section(".discard.ksym") __used
+#endif
+
#define __EXPORT_SYMBOL(sym, sec, ns) \
__ksym_marker(sym); \
__cond_export_sym(sym, sec, ns, __is_defined(__KSYM_##sym))
@@ -147,11 +123,9 @@ struct kernel_symbol {
#define _EXPORT_SYMBOL(sym, sec) __EXPORT_SYMBOL(sym, sec, "")
#endif
-#define EXPORT_SYMBOL(sym) _EXPORT_SYMBOL(sym, "")
-#define EXPORT_SYMBOL_GPL(sym) _EXPORT_SYMBOL(sym, "_gpl")
-#define EXPORT_SYMBOL_NS(sym, ns) __EXPORT_SYMBOL(sym, "", __stringify(ns))
-#define EXPORT_SYMBOL_NS_GPL(sym, ns) __EXPORT_SYMBOL(sym, "_gpl", __stringify(ns))
-
-#endif /* !__ASSEMBLY__ */
+#define EXPORT_SYMBOL(sym) _EXPORT_SYMBOL(sym,)
+#define EXPORT_SYMBOL_GPL(sym) _EXPORT_SYMBOL(sym,gpl)
+#define EXPORT_SYMBOL_NS(sym, ns) __EXPORT_SYMBOL(sym,, __stringify(ns))
+#define EXPORT_SYMBOL_NS_GPL(sym, ns) __EXPORT_SYMBOL(sym,gpl, __stringify(ns))
#endif /* _LINUX_EXPORT_H */
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 035d9649eba4..aabb6bd8f89e 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -388,10 +388,10 @@ const struct dev_pm_ops name = { \
#define EXPORT_PM_FN_NS_GPL(name, ns)
#endif
-#define EXPORT_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(name, "", "")
-#define EXPORT_GPL_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(name, "_gpl", "")
-#define EXPORT_NS_DEV_PM_OPS(name, ns) _EXPORT_DEV_PM_OPS(name, "", #ns)
-#define EXPORT_NS_GPL_DEV_PM_OPS(name, ns) _EXPORT_DEV_PM_OPS(name, "_gpl", #ns)
+#define EXPORT_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(name,, "")
+#define EXPORT_GPL_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(name,gpl, "")
+#define EXPORT_NS_DEV_PM_OPS(name, ns) _EXPORT_DEV_PM_OPS(name,, #ns)
+#define EXPORT_NS_GPL_DEV_PM_OPS(name, ns) _EXPORT_DEV_PM_OPS(name,gpl, #ns)
/*
* Use this if you want to use the same suspend and resume callbacks for suspend
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index dc7b0160c480..c8b7b4dcf782 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -32,6 +32,18 @@
/* Maximum number of characters written by module_flags() */
#define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
+struct kernel_symbol {
+#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
+ int value_offset;
+ int name_offset;
+ int namespace_offset;
+#else
+ unsigned long value;
+ const char *name;
+ const char *namespace;
+#endif
+};
+
extern struct mutex module_mutex;
extern struct list_head modules;
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 9f94fc83f086..6bf026a304e4 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -161,7 +161,7 @@ quiet_cmd_cc_o_c = CC $(quiet_modtag) $@
ifdef CONFIG_MODVERSIONS
# When module versioning is enabled the following steps are executed:
# o compile a <file>.o from <file>.c
-# o if <file>.o doesn't contain a __ksymtab version, i.e. does
+# o if <file>.o doesn't contain a __export_symbol*, i.e. does
# not export symbols, it's done.
# o otherwise, we calculate symbol versions using the good old
# genksyms on the preprocessed source and dump them into the .cmd file.
@@ -169,7 +169,7 @@ ifdef CONFIG_MODVERSIONS
# be compiled and linked to the kernel and/or modules.
gen_symversions = \
- if $(NM) $@ 2>/dev/null | grep -q __ksymtab; then \
+ if $(NM) $@ 2>/dev/null | grep -q '__export_symbol_[^_]*_'; then \
$(call cmd_gensymtypes_$(1),$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \
>> $(dot-target).cmd; \
fi
@@ -340,9 +340,7 @@ $(obj)/%.ll: $(src)/%.rs FORCE
cmd_gensymtypes_S = \
{ echo "\#include <linux/kernel.h>" ; \
echo "\#include <asm/asm-prototypes.h>" ; \
- $(CPP) $(a_flags) $< | \
- grep "\<___EXPORT_SYMBOL\>" | \
- sed 's/.*___EXPORT_SYMBOL[[:space:]]*\([a-zA-Z0-9_]*\)[[:space:]]*,.*/EXPORT_SYMBOL(\1);/' ; } | \
+ $(NM) $@ | sed -n 's/.*__export_symbol_[^_]*_\(.*\)/EXPORT_SYMBOL(\1);/p' ; } | \
$(CPP) -D__GENKSYMS__ $(c_flags) -xc - | $(genksyms)
quiet_cmd_cc_symtypes_S = SYM $(quiet_modtag) $@
diff --git a/scripts/check-local-export b/scripts/check-local-export
index f90b5a9c67b3..969a313b9299 100755
--- a/scripts/check-local-export
+++ b/scripts/check-local-export
@@ -46,9 +46,9 @@ BEGIN {
{ symbol_types[$3]=$2 }
# append the exported symbol to the array
-($3 ~ /^__ksymtab_/) {
+($3 ~ /^__export_symbol_(gpl)?_.*/) {
export_symbols[i] = $3
- sub(/^__ksymtab_/, "", export_symbols[i])
+ sub(/^__export_symbol_(gpl)?_/, "", export_symbols[i])
i++
}
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 378fb9649545..8b94090d0743 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -215,6 +215,7 @@ struct symbol {
unsigned int crc;
bool crc_valid;
bool weak;
+ bool is_func;
bool is_gpl_only; /* exported by EXPORT_SYMBOL_GPL */
char name[];
};
@@ -531,6 +532,8 @@ static int parse_elf(struct elf_info *info, const char *filename)
fatal("%s has NOBITS .modinfo\n", filename);
info->modinfo = (void *)hdr + sechdrs[i].sh_offset;
info->modinfo_len = sechdrs[i].sh_size;
+ } else if (!strcmp(secname, ".export_symbol")) {
+ info->export_symbol_secndx = i;
}
if (sechdrs[i].sh_type == SHT_SYMTAB) {
@@ -653,18 +656,6 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
ELF_ST_BIND(sym->st_info) == STB_WEAK);
break;
default:
- /* All exported symbols */
- if (strstarts(symname, "__ksymtab_")) {
- const char *name, *secname;
-
- name = symname + strlen("__ksymtab_");
- secname = sec_name(info, get_secindex(info, sym));
-
- if (strstarts(secname, "___ksymtab_gpl+"))
- sym_add_exported(name, mod, true);
- else if (strstarts(secname, "___ksymtab+"))
- sym_add_exported(name, mod, false);
- }
if (strcmp(symname, "init_module") == 0)
mod->has_init = true;
if (strcmp(symname, "cleanup_module") == 0)
@@ -865,7 +856,6 @@ enum mismatch {
XXXEXIT_TO_SOME_EXIT,
ANY_INIT_TO_ANY_EXIT,
ANY_EXIT_TO_ANY_INIT,
- EXPORT_TO_INIT_EXIT,
EXTABLE_TO_NON_TEXT,
};
@@ -947,12 +937,6 @@ static const struct sectioncheck sectioncheck[] = {
.bad_tosec = { INIT_SECTIONS, NULL },
.mismatch = ANY_INIT_TO_ANY_EXIT,
},
-/* Do not export init/exit functions or data */
-{
- .fromsec = { "___ksymtab*", NULL },
- .bad_tosec = { INIT_SECTIONS, EXIT_SECTIONS, NULL },
- .mismatch = EXPORT_TO_INIT_EXIT,
-},
{
.fromsec = { "__ex_table", NULL },
/* If you're adding any new black-listed sections in here, consider
@@ -1198,10 +1182,6 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
warn("%s: section mismatch in reference: %s (section: %s) -> %s (section: %s)\n",
modname, fromsym, fromsec, tosym, tosec);
break;
- case EXPORT_TO_INIT_EXIT:
- warn("%s: EXPORT_SYMBOL used for init/exit symbol: %s (section: %s)\n",
- modname, tosym, tosec);
- break;
case EXTABLE_TO_NON_TEXT:
warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
modname, fromsec, (long)faddr, tosec, tosym);
@@ -1229,14 +1209,69 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
}
}
+static void check_export_symbol(struct module *mod, struct elf_info *elf,
+ Elf_Addr faddr, const char *secname,
+ Elf_Sym *sym)
+{
+ const char *label_name, *name, *prefix;
+ Elf_Sym *label;
+ struct symbol *s;
+ bool is_gpl;
+
+ label = find_fromsym(elf, faddr, elf->export_symbol_secndx);
+ label_name = sym_name(elf, label);
+
+ name = sym_name(elf, sym);
+
+ if (strstarts(label_name, "__export_symbol_gpl_")) {
+ prefix = "__export_symbol_gpl_";
+ is_gpl = true;
+ } else if (strstarts(label_name, "__export_symbol__")) {
+ prefix = "__export_symbol__";
+ is_gpl = false;
+ } else {
+ error("%s: .export_symbol section contains strange symbol '%s'\n",
+ mod->name, label_name);
+ return;
+ }
+
+ if (strcmp(label_name + strlen(prefix), name)) {
+ error("%s: .export_symbol section references '%s', but it does not seem to be an export symbol\n",
+ mod->name, name);
+ return;
+ }
+
+ s = sym_add_exported(name, mod, is_gpl);
+ sym_update_namespace(name, sym_get_data(elf, label));
+
+ /*
+ * We need to be aware whether we are exporting a function or
+ * a data on some architectures.
+ */
+ s->is_func = (ELF_ST_TYPE(sym->st_info) == STT_FUNC);
+
+ if (match(secname, PATTERNS(INIT_SECTIONS)))
+ error("%s: %s: EXPORT_SYMBOL used for init symbol. Remove __init or EXPORT_SYMBOL.\n",
+ mod->name, name);
+ else if (match(secname, PATTERNS(EXIT_SECTIONS)))
+ error("%s: %s: EXPORT_SYMBOL used for exit symbol. Remove __exit or EXPORT_SYMBOL.\n",
+ mod->name, name);
+}
+
static void check_section_mismatch(struct module *mod, struct elf_info *elf,
Elf_Sym *sym,
unsigned int fsecndx, const char *fromsec,
Elf_Addr faddr, Elf_Addr taddr)
{
const char *tosec = sec_name(elf, get_secindex(elf, sym));
- const struct sectioncheck *mismatch = section_mismatch(fromsec, tosec);
+ const struct sectioncheck *mismatch;
+ if (elf->export_symbol_secndx == fsecndx) {
+ check_export_symbol(mod, elf, faddr, tosec, sym);
+ return;
+ }
+
+ mismatch = section_mismatch(fromsec, tosec);
if (!mismatch)
return;
@@ -1650,15 +1685,6 @@ static void read_symbols(const char *modname)
handle_moddevtable(mod, &info, sym, symname);
}
- for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
- symname = remove_dot(info.strtab + sym->st_name);
-
- /* Apply symbol namespaces from __kstrtabns_<symbol> entries. */
- if (strstarts(symname, "__kstrtabns_"))
- sym_update_namespace(symname + strlen("__kstrtabns_"),
- sym_get_data(&info, sym));
- }
-
check_sec_ref(mod, &info);
if (!mod->is_vmlinux) {
@@ -1842,6 +1868,14 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod)
{
struct symbol *sym;
+ /* generate struct for exported symbols */
+ buf_printf(buf, "\n");
+ list_for_each_entry(sym, &mod->exported_symbols, list)
+ buf_printf(buf, "KSYMTAB_%s(%s, \"%s\", \"%s\");\n",
+ sym->is_func ? "FUNC" : "DATA", sym->name,
+ sym->is_gpl_only ? "_gpl" : "",
+ sym->namespace ?: "");
+
if (!modversions)
return;
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index b1e2d95f8047..dfdb9484e325 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -137,6 +137,7 @@ struct elf_info {
Elf_Shdr *sechdrs;
Elf_Sym *symtab_start;
Elf_Sym *symtab_stop;
+ unsigned int export_symbol_secndx; /* .export_symbol section */
char *strtab;
char *modinfo;
unsigned int modinfo_len;
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 10/20] kbuild: generate KSYMTAB entries by modpost
2023-05-21 16:04 ` [PATCH v6 10/20] kbuild: generate KSYMTAB entries by modpost Masahiro Yamada
@ 2023-05-25 17:50 ` Nick Desaulniers
2023-06-02 13:51 ` Masahiro Yamada
0 siblings, 1 reply; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-25 17:50 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Commit 7b4537199a4a ("kbuild: link symbol CRCs at final link, removing
> CONFIG_MODULE_REL_CRCS") made modpost output CRCs in the same way
> whether the EXPORT_SYMBOL() is placed in *.c or *.S.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Patch LGTM, but two questions, below.
> diff --git a/arch/um/os-Linux/user_syms.c b/arch/um/os-Linux/user_syms.c
> index 9b62a9d352b3..0b3a82aedbfa 100644
> --- a/arch/um/os-Linux/user_syms.c
> +++ b/arch/um/os-Linux/user_syms.c
> @@ -38,10 +38,15 @@ EXPORT_SYMBOL(vsyscall_end);
> #endif
>
> /* Export symbols used by GCC for the stack protector. */
> -extern void __stack_smash_handler(void *) __attribute__((weak));
> +/*
> + * REVISIT:
> + * ARCH=um cannot enable CONFIG_STACKPROTECTOR because
> + * 'select HAVE_STACKPROTECTOR' is missing. Is this already broken?
^ Is there something better we can do here?
Maybe this code should be deleted first?
> diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
> index 5e4b1f2369d2..0ae9f38a904c 100644
> --- a/include/asm-generic/export.h
> +++ b/include/asm-generic/export.h
> @@ -3,86 +3,12 @@
> #define __ASM_GENERIC_EXPORT_H
>
> /*
> - * This comment block is used by fixdep. Please do not remove.
^ Sounds ominous. What are the implications of removing this comment block?
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 10/20] kbuild: generate KSYMTAB entries by modpost
2023-05-25 17:50 ` Nick Desaulniers
@ 2023-06-02 13:51 ` Masahiro Yamada
0 siblings, 0 replies; 51+ messages in thread
From: Masahiro Yamada @ 2023-06-02 13:51 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Fri, May 26, 2023 at 2:51 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > Commit 7b4537199a4a ("kbuild: link symbol CRCs at final link, removing
> > CONFIG_MODULE_REL_CRCS") made modpost output CRCs in the same way
> > whether the EXPORT_SYMBOL() is placed in *.c or *.S.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>
> Patch LGTM, but two questions, below.
>
> > diff --git a/arch/um/os-Linux/user_syms.c b/arch/um/os-Linux/user_syms.c
> > index 9b62a9d352b3..0b3a82aedbfa 100644
> > --- a/arch/um/os-Linux/user_syms.c
> > +++ b/arch/um/os-Linux/user_syms.c
> > @@ -38,10 +38,15 @@ EXPORT_SYMBOL(vsyscall_end);
> > #endif
> >
> > /* Export symbols used by GCC for the stack protector. */
> > -extern void __stack_smash_handler(void *) __attribute__((weak));
> > +/*
> > + * REVISIT:
> > + * ARCH=um cannot enable CONFIG_STACKPROTECTOR because
> > + * 'select HAVE_STACKPROTECTOR' is missing. Is this already broken?
>
> ^ Is there something better we can do here?
>
> Maybe this code should be deleted first?
Yes, we can delete the lines first.
>
> > diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
> > index 5e4b1f2369d2..0ae9f38a904c 100644
> > --- a/include/asm-generic/export.h
> > +++ b/include/asm-generic/export.h
> > @@ -3,86 +3,12 @@
> > #define __ASM_GENERIC_EXPORT_H
> >
> > /*
> > - * This comment block is used by fixdep. Please do not remove.
>
> ^ Sounds ominous. What are the implications of removing this comment block?
The same comment existed in <linux/export.h> and <asm-generic/export.h>
because they were independent from each other.
This commit changes <asm-generic/export.h> to a wrapper of <linux/export.h>.
So, the comment in <linux/export.h> triggers rebuilding when
CONFIG_MODVERSIONS is toggled.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 11/20] ia64,export.h: replace EXPORT_DATA_SYMBOL* with EXPORT_SYMBOL*
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (9 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 10/20] kbuild: generate KSYMTAB entries by modpost Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-25 17:52 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 12/20] modpost: check static EXPORT_SYMBOL* by modpost again Masahiro Yamada
` (9 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
With the previous refactoring, you can always use EXPORT_SYMBOL*.
Replace two instances in ia64, then remove EXPORT_DATA_SYMBOL*.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
arch/ia64/kernel/head.S | 2 +-
arch/ia64/kernel/ivt.S | 2 +-
include/asm-generic/export.h | 3 ---
3 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/arch/ia64/kernel/head.S b/arch/ia64/kernel/head.S
index f22469f1c1fc..c096500590e9 100644
--- a/arch/ia64/kernel/head.S
+++ b/arch/ia64/kernel/head.S
@@ -170,7 +170,7 @@ RestRR: \
__PAGE_ALIGNED_DATA
.global empty_zero_page
-EXPORT_DATA_SYMBOL_GPL(empty_zero_page)
+EXPORT_SYMBOL_GPL(empty_zero_page)
empty_zero_page:
.skip PAGE_SIZE
diff --git a/arch/ia64/kernel/ivt.S b/arch/ia64/kernel/ivt.S
index d6d4229b28db..7a418e324d30 100644
--- a/arch/ia64/kernel/ivt.S
+++ b/arch/ia64/kernel/ivt.S
@@ -87,7 +87,7 @@
.align 32768 // align on 32KB boundary
.global ia64_ivt
- EXPORT_DATA_SYMBOL(ia64_ivt)
+ EXPORT_SYMBOL(ia64_ivt)
ia64_ivt:
/////////////////////////////////////////////////////////////////////////////////////////
// 0x0000 Entry 0 (size 64 bundles) VHPT Translation (8,20,47)
diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
index 0ae9f38a904c..570cd4da7210 100644
--- a/include/asm-generic/export.h
+++ b/include/asm-generic/export.h
@@ -8,7 +8,4 @@
*/
#include <linux/export.h>
-#define EXPORT_DATA_SYMBOL(name) EXPORT_SYMBOL(name)
-#define EXPORT_DATA_SYMBOL_GPL(name) EXPORT_SYMBOL_GPL(name)
-
#endif
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 11/20] ia64,export.h: replace EXPORT_DATA_SYMBOL* with EXPORT_SYMBOL*
2023-05-21 16:04 ` [PATCH v6 11/20] ia64,export.h: replace EXPORT_DATA_SYMBOL* with EXPORT_SYMBOL* Masahiro Yamada
@ 2023-05-25 17:52 ` Nick Desaulniers
0 siblings, 0 replies; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-25 17:52 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> With the previous refactoring, you can always use EXPORT_SYMBOL*.
>
> Replace two instances in ia64, then remove EXPORT_DATA_SYMBOL*.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>
> arch/ia64/kernel/head.S | 2 +-
> arch/ia64/kernel/ivt.S | 2 +-
> include/asm-generic/export.h | 3 ---
> 3 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/ia64/kernel/head.S b/arch/ia64/kernel/head.S
> index f22469f1c1fc..c096500590e9 100644
> --- a/arch/ia64/kernel/head.S
> +++ b/arch/ia64/kernel/head.S
> @@ -170,7 +170,7 @@ RestRR: \
> __PAGE_ALIGNED_DATA
>
> .global empty_zero_page
> -EXPORT_DATA_SYMBOL_GPL(empty_zero_page)
> +EXPORT_SYMBOL_GPL(empty_zero_page)
> empty_zero_page:
> .skip PAGE_SIZE
>
> diff --git a/arch/ia64/kernel/ivt.S b/arch/ia64/kernel/ivt.S
> index d6d4229b28db..7a418e324d30 100644
> --- a/arch/ia64/kernel/ivt.S
> +++ b/arch/ia64/kernel/ivt.S
> @@ -87,7 +87,7 @@
>
> .align 32768 // align on 32KB boundary
> .global ia64_ivt
> - EXPORT_DATA_SYMBOL(ia64_ivt)
> + EXPORT_SYMBOL(ia64_ivt)
> ia64_ivt:
> /////////////////////////////////////////////////////////////////////////////////////////
> // 0x0000 Entry 0 (size 64 bundles) VHPT Translation (8,20,47)
> diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
> index 0ae9f38a904c..570cd4da7210 100644
> --- a/include/asm-generic/export.h
> +++ b/include/asm-generic/export.h
> @@ -8,7 +8,4 @@
> */
> #include <linux/export.h>
>
> -#define EXPORT_DATA_SYMBOL(name) EXPORT_SYMBOL(name)
> -#define EXPORT_DATA_SYMBOL_GPL(name) EXPORT_SYMBOL_GPL(name)
> -
> #endif
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 12/20] modpost: check static EXPORT_SYMBOL* by modpost again
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (10 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 11/20] ia64,export.h: replace EXPORT_DATA_SYMBOL* with EXPORT_SYMBOL* Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-25 18:18 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 13/20] modpost: squash sym_update_namespace() into sym_add_exported() Masahiro Yamada
` (8 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
Commit 31cb50b5590f ("kbuild: check static EXPORT_SYMBOL* by script
instead of modpost") moved the static EXPORT_SYMBOL* check from the
mostpost to a shell script because I thought it must be checked per
compilation unit to avoid false negatives.
I came up with an idea to do this in modpost, against combined ELF
files. The relocation entries in ELF will find the correct exported
symbol even if there exist symbols with the same name in different
compilation units.
Again, the same sample code.
Makefile:
obj-y += foo1.o foo2.o
foo1.c:
#include <linux/export.h>
static void foo(void) {}
EXPORT_SYMBOL(foo);
foo2.c:
void foo(void) {}
Then, modpost can catch it correctly.
MODPOST Module.symvers
ERROR: modpost: vmlinux: local symbol 'foo' was exported
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
Changes in v6:
- Make the symbol name in the warning more precise
scripts/Makefile.build | 4 ---
scripts/check-local-export | 70 --------------------------------------
scripts/mod/modpost.c | 7 ++++
3 files changed, 7 insertions(+), 74 deletions(-)
delete mode 100755 scripts/check-local-export
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 6bf026a304e4..bd4123795299 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -220,8 +220,6 @@ cmd_gen_ksymdeps = \
$(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ >> $(dot-target).cmd
endif
-cmd_check_local_export = $(srctree)/scripts/check-local-export $@
-
ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
cmd_warn_shared_object = $(if $(word 2, $(modname-multi)),$(warning $(kbuild-file): $*.o is added to multiple modules: $(modname-multi)))
endif
@@ -229,7 +227,6 @@ endif
define rule_cc_o_c
$(call cmd_and_fixdep,cc_o_c)
$(call cmd,gen_ksymdeps)
- $(call cmd,check_local_export)
$(call cmd,checksrc)
$(call cmd,checkdoc)
$(call cmd,gen_objtooldep)
@@ -241,7 +238,6 @@ endef
define rule_as_o_S
$(call cmd_and_fixdep,as_o_S)
$(call cmd,gen_ksymdeps)
- $(call cmd,check_local_export)
$(call cmd,gen_objtooldep)
$(call cmd,gen_symversions_S)
$(call cmd,warn_shared_object)
diff --git a/scripts/check-local-export b/scripts/check-local-export
deleted file mode 100755
index 969a313b9299..000000000000
--- a/scripts/check-local-export
+++ /dev/null
@@ -1,70 +0,0 @@
-#!/bin/sh
-# SPDX-License-Identifier: GPL-2.0-only
-#
-# Copyright (C) 2022 Masahiro Yamada <masahiroy@kernel.org>
-# Copyright (C) 2022 Owen Rafferty <owen@owenrafferty.com>
-#
-# Exit with error if a local exported symbol is found.
-# EXPORT_SYMBOL should be used for global symbols.
-
-set -e
-pid=$$
-
-# If there is no symbol in the object, ${NM} (both GNU nm and llvm-nm) shows
-# 'no symbols' diagnostic (but exits with 0). It is harmless and hidden by
-# '2>/dev/null'. However, it suppresses real error messages as well. Add a
-# hand-crafted error message here.
-#
-# TODO:
-# Use --quiet instead of 2>/dev/null when we upgrade the minimum version of
-# binutils to 2.37, llvm to 13.0.0.
-# Then, the following line will be simpler:
-# { ${NM} --quiet ${1} || kill 0; } |
-
-{ ${NM} ${1} 2>/dev/null || { echo "${0}: ${NM} failed" >&2; kill $pid; } } |
-${AWK} -v "file=${1}" '
-BEGIN {
- i = 0
-}
-
-# Skip the line if the number of fields is less than 3.
-#
-# case 1)
-# For undefined symbols, the first field (value) is empty.
-# The outout looks like this:
-# " U _printk"
-# It is unneeded to record undefined symbols.
-#
-# case 2)
-# For Clang LTO, llvm-nm outputs a line with type t but empty name:
-# "---------------- t"
-!length($3) {
- next
-}
-
-# save (name, type) in the associative array
-{ symbol_types[$3]=$2 }
-
-# append the exported symbol to the array
-($3 ~ /^__export_symbol_(gpl)?_.*/) {
- export_symbols[i] = $3
- sub(/^__export_symbol_(gpl)?_/, "", export_symbols[i])
- i++
-}
-
-END {
- exit_code = 0
- for (j = 0; j < i; ++j) {
- name = export_symbols[j]
- # nm(3) says "If lowercase, the symbol is usually local"
- if (symbol_types[name] ~ /[a-z]/) {
- printf "%s: error: local symbol %s was exported\n",
- file, name | "cat 1>&2"
- exit_code = 1
- }
- }
-
- exit exit_code
-}'
-
-exit $?
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 8b94090d0743..dd1d066f1214 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1235,6 +1235,13 @@ static void check_export_symbol(struct module *mod, struct elf_info *elf,
return;
}
+ if (ELF_ST_BIND(sym->st_info) != STB_GLOBAL &&
+ ELF_ST_BIND(sym->st_info) != STB_WEAK) {
+ error("%s: local symbol '%s' was exported\n", mod->name,
+ label_name + strlen(prefix));
+ return;
+ }
+
if (strcmp(label_name + strlen(prefix), name)) {
error("%s: .export_symbol section references '%s', but it does not seem to be an export symbol\n",
mod->name, name);
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 12/20] modpost: check static EXPORT_SYMBOL* by modpost again
2023-05-21 16:04 ` [PATCH v6 12/20] modpost: check static EXPORT_SYMBOL* by modpost again Masahiro Yamada
@ 2023-05-25 18:18 ` Nick Desaulniers
0 siblings, 0 replies; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-25 18:18 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier, owen
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Commit 31cb50b5590f ("kbuild: check static EXPORT_SYMBOL* by script
> instead of modpost") moved the static EXPORT_SYMBOL* check from the
> mostpost to a shell script because I thought it must be checked per
> compilation unit to avoid false negatives.
>
> I came up with an idea to do this in modpost, against combined ELF
> files. The relocation entries in ELF will find the correct exported
> symbol even if there exist symbols with the same name in different
> compilation units.
>
> Again, the same sample code.
>
> Makefile:
>
> obj-y += foo1.o foo2.o
>
> foo1.c:
>
> #include <linux/export.h>
> static void foo(void) {}
> EXPORT_SYMBOL(foo);
>
> foo2.c:
>
> void foo(void) {}
>
> Then, modpost can catch it correctly.
>
> MODPOST Module.symvers
> ERROR: modpost: vmlinux: local symbol 'foo' was exported
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>
> Changes in v6:
> - Make the symbol name in the warning more precise
>
> scripts/Makefile.build | 4 ---
> scripts/check-local-export | 70 --------------------------------------
> scripts/mod/modpost.c | 7 ++++
> 3 files changed, 7 insertions(+), 74 deletions(-)
> delete mode 100755 scripts/check-local-export
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 6bf026a304e4..bd4123795299 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -220,8 +220,6 @@ cmd_gen_ksymdeps = \
> $(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ >> $(dot-target).cmd
> endif
>
> -cmd_check_local_export = $(srctree)/scripts/check-local-export $@
> -
> ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
> cmd_warn_shared_object = $(if $(word 2, $(modname-multi)),$(warning $(kbuild-file): $*.o is added to multiple modules: $(modname-multi)))
> endif
> @@ -229,7 +227,6 @@ endif
> define rule_cc_o_c
> $(call cmd_and_fixdep,cc_o_c)
> $(call cmd,gen_ksymdeps)
> - $(call cmd,check_local_export)
> $(call cmd,checksrc)
> $(call cmd,checkdoc)
> $(call cmd,gen_objtooldep)
> @@ -241,7 +238,6 @@ endef
> define rule_as_o_S
> $(call cmd_and_fixdep,as_o_S)
> $(call cmd,gen_ksymdeps)
> - $(call cmd,check_local_export)
> $(call cmd,gen_objtooldep)
> $(call cmd,gen_symversions_S)
> $(call cmd,warn_shared_object)
> diff --git a/scripts/check-local-export b/scripts/check-local-export
> deleted file mode 100755
> index 969a313b9299..000000000000
> --- a/scripts/check-local-export
> +++ /dev/null
> @@ -1,70 +0,0 @@
> -#!/bin/sh
> -# SPDX-License-Identifier: GPL-2.0-only
> -#
> -# Copyright (C) 2022 Masahiro Yamada <masahiroy@kernel.org>
> -# Copyright (C) 2022 Owen Rafferty <owen@owenrafferty.com>
> -#
> -# Exit with error if a local exported symbol is found.
> -# EXPORT_SYMBOL should be used for global symbols.
> -
> -set -e
> -pid=$$
> -
> -# If there is no symbol in the object, ${NM} (both GNU nm and llvm-nm) shows
> -# 'no symbols' diagnostic (but exits with 0). It is harmless and hidden by
> -# '2>/dev/null'. However, it suppresses real error messages as well. Add a
> -# hand-crafted error message here.
> -#
> -# TODO:
> -# Use --quiet instead of 2>/dev/null when we upgrade the minimum version of
> -# binutils to 2.37, llvm to 13.0.0.
> -# Then, the following line will be simpler:
> -# { ${NM} --quiet ${1} || kill 0; } |
> -
> -{ ${NM} ${1} 2>/dev/null || { echo "${0}: ${NM} failed" >&2; kill $pid; } } |
> -${AWK} -v "file=${1}" '
> -BEGIN {
> - i = 0
> -}
> -
> -# Skip the line if the number of fields is less than 3.
> -#
> -# case 1)
> -# For undefined symbols, the first field (value) is empty.
> -# The outout looks like this:
> -# " U _printk"
> -# It is unneeded to record undefined symbols.
> -#
> -# case 2)
> -# For Clang LTO, llvm-nm outputs a line with type t but empty name:
> -# "---------------- t"
> -!length($3) {
> - next
> -}
> -
> -# save (name, type) in the associative array
> -{ symbol_types[$3]=$2 }
> -
> -# append the exported symbol to the array
> -($3 ~ /^__export_symbol_(gpl)?_.*/) {
> - export_symbols[i] = $3
> - sub(/^__export_symbol_(gpl)?_/, "", export_symbols[i])
> - i++
> -}
> -
> -END {
> - exit_code = 0
> - for (j = 0; j < i; ++j) {
> - name = export_symbols[j]
> - # nm(3) says "If lowercase, the symbol is usually local"
> - if (symbol_types[name] ~ /[a-z]/) {
> - printf "%s: error: local symbol %s was exported\n",
> - file, name | "cat 1>&2"
> - exit_code = 1
> - }
> - }
> -
> - exit exit_code
> -}'
> -
> -exit $?
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 8b94090d0743..dd1d066f1214 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1235,6 +1235,13 @@ static void check_export_symbol(struct module *mod, struct elf_info *elf,
> return;
> }
>
> + if (ELF_ST_BIND(sym->st_info) != STB_GLOBAL &&
> + ELF_ST_BIND(sym->st_info) != STB_WEAK) {
> + error("%s: local symbol '%s' was exported\n", mod->name,
> + label_name + strlen(prefix));
> + return;
> + }
> +
> if (strcmp(label_name + strlen(prefix), name)) {
> error("%s: .export_symbol section references '%s', but it does not seem to be an export symbol\n",
> mod->name, name);
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 13/20] modpost: squash sym_update_namespace() into sym_add_exported()
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (11 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 12/20] modpost: check static EXPORT_SYMBOL* by modpost again Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-21 16:04 ` [PATCH v6 14/20] modpost: use null string instead of NULL pointer for default namespace Masahiro Yamada
` (7 subsequent siblings)
20 siblings, 0 replies; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
Pass a set of the name, license, and namespace to sym_add_exported().
sym_update_namespace() is unneeded.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 27 ++++-----------------------
1 file changed, 4 insertions(+), 23 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index dd1d066f1214..8fe2aa7bf71b 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -353,26 +353,8 @@ static const char *sec_name(const struct elf_info *info, unsigned int secindex)
#define strstarts(str, prefix) (strncmp(str, prefix, strlen(prefix)) == 0)
-static void sym_update_namespace(const char *symname, const char *namespace)
-{
- struct symbol *s = find_symbol(symname);
-
- /*
- * That symbol should have been created earlier and thus this is
- * actually an assertion.
- */
- if (!s) {
- error("Could not update namespace(%s) for symbol %s\n",
- namespace, symname);
- return;
- }
-
- free(s->namespace);
- s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
-}
-
static struct symbol *sym_add_exported(const char *name, struct module *mod,
- bool gpl_only)
+ bool gpl_only, const char *namespace)
{
struct symbol *s = find_symbol(name);
@@ -385,6 +367,7 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
s = alloc_symbol(name);
s->module = mod;
s->is_gpl_only = gpl_only;
+ s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
list_add_tail(&s->list, &mod->exported_symbols);
hash_add_symbol(s);
@@ -1248,8 +1231,7 @@ static void check_export_symbol(struct module *mod, struct elf_info *elf,
return;
}
- s = sym_add_exported(name, mod, is_gpl);
- sym_update_namespace(name, sym_get_data(elf, label));
+ s = sym_add_exported(name, mod, is_gpl, sym_get_data(elf, label));
/*
* We need to be aware whether we are exporting a function or
@@ -2126,9 +2108,8 @@ static void read_dump(const char *fname)
mod = new_module(modname, strlen(modname));
mod->from_dump = true;
}
- s = sym_add_exported(symname, mod, gpl_only);
+ s = sym_add_exported(symname, mod, gpl_only, namespace);
sym_set_crc(s, crc);
- sym_update_namespace(symname, namespace);
}
free(buf);
return;
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH v6 14/20] modpost: use null string instead of NULL pointer for default namespace
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (12 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 13/20] modpost: squash sym_update_namespace() into sym_add_exported() Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-21 16:04 ` [PATCH v6 15/20] kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion Masahiro Yamada
` (6 subsequent siblings)
20 siblings, 0 replies; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
The default namespace is the null string, "".
When set, the null string "" is converted to NULL:
s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
When printed, the NULL pointer is get back to the null string:
sym->namespace ?: ""
This saves 1 byte memory allocated for "", but loses the readability.
In kernel-space, we strive to save memory, but modpost is a userspace
tool used to build the kernel. On modern systems, such small piece of
memory is not a big deal.
Handle the namespace string as is.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 8fe2aa7bf71b..f14fe9301ae6 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -298,6 +298,13 @@ static bool contains_namespace(struct list_head *head, const char *namespace)
{
struct namespace_list *list;
+ /*
+ * The default namespace is null string "", which is always implicitly
+ * contained.
+ */
+ if (!namespace[0])
+ return true;
+
list_for_each_entry(list, head, list) {
if (!strcmp(list->namespace, namespace))
return true;
@@ -367,7 +374,7 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
s = alloc_symbol(name);
s->module = mod;
s->is_gpl_only = gpl_only;
- s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
+ s->namespace = NOFAIL(strdup(namespace));
list_add_tail(&s->list, &mod->exported_symbols);
hash_add_symbol(s);
@@ -1775,8 +1782,7 @@ static void check_exports(struct module *mod)
else
basename = mod->name;
- if (exp->namespace &&
- !contains_namespace(&mod->imported_namespaces, exp->namespace)) {
+ if (!contains_namespace(&mod->imported_namespaces, exp->namespace)) {
modpost_log(allow_missing_ns_imports ? LOG_WARN : LOG_ERROR,
"module %s uses symbol %s from namespace %s, but does not import it.\n",
basename, exp->name, exp->namespace);
@@ -1862,8 +1868,7 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod)
list_for_each_entry(sym, &mod->exported_symbols, list)
buf_printf(buf, "KSYMTAB_%s(%s, \"%s\", \"%s\");\n",
sym->is_func ? "FUNC" : "DATA", sym->name,
- sym->is_gpl_only ? "_gpl" : "",
- sym->namespace ?: "");
+ sym->is_gpl_only ? "_gpl" : "", sym->namespace);
if (!modversions)
return;
@@ -2131,7 +2136,7 @@ static void write_dump(const char *fname)
buf_printf(&buf, "0x%08x\t%s\t%s\tEXPORT_SYMBOL%s\t%s\n",
sym->crc, sym->name, mod->name,
sym->is_gpl_only ? "_GPL" : "",
- sym->namespace ?: "");
+ sym->namespace);
}
}
write_buf(&buf, fname);
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH v6 15/20] kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (13 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 14/20] modpost: use null string instead of NULL pointer for default namespace Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-25 18:14 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 16/20] modpost: merge fromsec=DATA_SECTIONS entries in sectioncheck table Masahiro Yamada
` (5 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
When CONFIG_TRIM_UNUSED_KSYMS is enabled, Kbuild recursively traverses
the directory tree to determine which EXPORT_SYMBOL to trim. If an
EXPORT_SYMBOL turns out to be unused by anyone, Kbuild begins the
second traverse, where some source files are recompiled with their
EXPORT_SYMBOL() tuned into a no-op.
Linus stated negative opinions about this slowness in commits:
- 5cf0fd591f2e ("Kbuild: disable TRIM_UNUSED_KSYMS option")
- a555bdd0c58c ("Kbuild: enable TRIM_UNUSED_KSYMS again, with some guarding")
We can do this better now. The final data structures of EXPORT_SYMBOL
are generated by the modpost stage, so modpost can selectively emit
KSYMTAB entries that are really used by modules.
Commit f73edc8951b2 ("kbuild: unify two modpost invocations") is another
ground-work to do this in a one-pass algorithm. With the list of modules,
modpost sets sym->used if it is used by a module. modpost emits KSYMTAB
only for symbols with sym->used==true.
BTW, Nicolas explained why the trimming was implemented with recursion:
https://lore.kernel.org/all/2o2rpn97-79nq-p7s2-nq5-8p83391473r@syhkavp.arg/
Actually, we never achieved that level of optimization where the chain
reaction of trimming comes into play because:
- CONFIG_LTO_CLANG cannot remove any unused symbols
- CONFIG_LD_DEAD_CODE_DATA_ELIMINATION is enabled only for vmlinux,
but not modules
If deeper trimming is required, we need to revisit this, but I guess
that is unlikely to happen.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
Changes in v5:
- Clean up more
.gitignore | 1 -
Makefile | 19 +---------
include/linux/export.h | 65 +++++----------------------------
scripts/Makefile.build | 7 ----
scripts/Makefile.modpost | 7 ++++
scripts/adjust_autoksyms.sh | 73 -------------------------------------
scripts/basic/fixdep.c | 3 +-
scripts/gen_ksymdeps.sh | 30 ---------------
scripts/mod/modpost.c | 54 ++++++++++++++++++++++++---
scripts/remove-stale-files | 2 +
10 files changed, 70 insertions(+), 191 deletions(-)
delete mode 100755 scripts/adjust_autoksyms.sh
delete mode 100755 scripts/gen_ksymdeps.sh
diff --git a/.gitignore b/.gitignore
index 7f86e0837909..172e3874adfd 100644
--- a/.gitignore
+++ b/.gitignore
@@ -112,7 +112,6 @@ modules.order
#
/include/config/
/include/generated/
-/include/ksym/
/arch/*/include/generated/
# stgit generated dirs
diff --git a/Makefile b/Makefile
index f836936fb4d8..ffc2c9b632fd 100644
--- a/Makefile
+++ b/Makefile
@@ -1193,28 +1193,13 @@ endif
export KBUILD_VMLINUX_LIBS
export KBUILD_LDS := arch/$(SRCARCH)/kernel/vmlinux.lds
-# Recurse until adjust_autoksyms.sh is satisfied
-PHONY += autoksyms_recursive
ifdef CONFIG_TRIM_UNUSED_KSYMS
# For the kernel to actually contain only the needed exported symbols,
# we have to build modules as well to determine what those symbols are.
# (this can be evaluated only once include/config/auto.conf has been included)
KBUILD_MODULES := 1
-
-autoksyms_recursive: $(build-dir) modules.order
- $(Q)$(CONFIG_SHELL) $(srctree)/scripts/adjust_autoksyms.sh \
- "$(MAKE) -f $(srctree)/Makefile autoksyms_recursive"
endif
-autoksyms_h := $(if $(CONFIG_TRIM_UNUSED_KSYMS), include/generated/autoksyms.h)
-
-quiet_cmd_autoksyms_h = GEN $@
- cmd_autoksyms_h = mkdir -p $(dir $@); \
- $(CONFIG_SHELL) $(srctree)/scripts/gen_autoksyms.sh $@
-
-$(autoksyms_h):
- $(call cmd,autoksyms_h)
-
# '$(AR) mPi' needs 'T' to workaround the bug of llvm-ar <= 14
quiet_cmd_ar_vmlinux.a = AR $@
cmd_ar_vmlinux.a = \
@@ -1223,7 +1208,7 @@ quiet_cmd_ar_vmlinux.a = AR $@
$(AR) mPiT $$($(AR) t $@ | sed -n 1p) $@ $$($(AR) t $@ | grep -F -f $(srctree)/scripts/head-object-list.txt)
targets += vmlinux.a
-vmlinux.a: $(KBUILD_VMLINUX_OBJS) scripts/head-object-list.txt autoksyms_recursive FORCE
+vmlinux.a: $(KBUILD_VMLINUX_OBJS) scripts/head-object-list.txt FORCE
$(call if_changed,ar_vmlinux.a)
PHONY += vmlinux_o
@@ -1279,7 +1264,7 @@ scripts: scripts_basic scripts_dtc
PHONY += prepare archprepare
archprepare: outputmakefile archheaders archscripts scripts include/config/kernel.release \
- asm-generic $(version_h) $(autoksyms_h) include/generated/utsrelease.h \
+ asm-generic $(version_h) include/generated/utsrelease.h \
include/generated/compile.h include/generated/autoconf.h remove-stale-files
prepare0: archprepare
diff --git a/include/linux/export.h b/include/linux/export.h
index 32461a01608c..9bf081ff9903 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -37,30 +37,13 @@ extern struct module __this_module;
#define __EXPORT_SYMBOL_REF(sym) .balign 4; .long sym
#endif
-#define ____EXPORT_SYMBOL(sym, license, ns) \
+#define ___EXPORT_SYMBOL(sym, license, ns) \
.section ".export_symbol","a" ; \
__export_symbol_##license##_##sym: ; \
.asciz ns ; \
__EXPORT_SYMBOL_REF(sym) ; \
.previous
-#ifdef __GENKSYMS__
-
-#define ___EXPORT_SYMBOL(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
-
-#elif defined(__ASSEMBLY__)
-
-#define ___EXPORT_SYMBOL(sym, license, ns) \
- ____EXPORT_SYMBOL(sym, license, ns)
-
-#else
-
-#define ___EXPORT_SYMBOL(sym, license, ns) \
- __ADDRESSABLE(sym) \
- asm(__stringify(____EXPORT_SYMBOL(sym, license, ns)))
-
-#endif
-
#if !defined(CONFIG_MODULES) || defined(__DISABLE_EXPORTS)
/*
@@ -70,50 +53,20 @@ extern struct module __this_module;
*/
#define __EXPORT_SYMBOL(sym, sec, ns)
-#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
+#elif defined(__GENKSYMS__)
-#include <generated/autoksyms.h>
+#define __EXPORT_SYMBOL(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
-/*
- * For fine grained build dependencies, we want to tell the build system
- * about each possible exported symbol even if they're not actually exported.
- * We use a symbol pattern __ksym_marker_<symbol> that the build system filters
- * from the $(NM) output (see scripts/gen_ksymdeps.sh). These symbols are
- * discarded in the final link stage.
- */
+#elif defined(__ASSEMBLY__)
-#ifdef __ASSEMBLY__
-
-#define __ksym_marker(sym) \
- .section ".discard.ksym","a" ; \
-__ksym_marker_##sym: ; \
- .previous
+#define __EXPORT_SYMBOL(sym, license, ns) \
+ ___EXPORT_SYMBOL(sym, license, ns)
#else
-#define __ksym_marker(sym) \
- static int __ksym_marker_##sym[0] __section(".discard.ksym") __used
-
-#endif
-
-#define __EXPORT_SYMBOL(sym, sec, ns) \
- __ksym_marker(sym); \
- __cond_export_sym(sym, sec, ns, __is_defined(__KSYM_##sym))
-#define __cond_export_sym(sym, sec, ns, conf) \
- ___cond_export_sym(sym, sec, ns, conf)
-#define ___cond_export_sym(sym, sec, ns, enabled) \
- __cond_export_sym_##enabled(sym, sec, ns)
-#define __cond_export_sym_1(sym, sec, ns) ___EXPORT_SYMBOL(sym, sec, ns)
-
-#ifdef __GENKSYMS__
-#define __cond_export_sym_0(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
-#else
-#define __cond_export_sym_0(sym, sec, ns) /* nothing */
-#endif
-
-#else
-
-#define __EXPORT_SYMBOL(sym, sec, ns) ___EXPORT_SYMBOL(sym, sec, ns)
+#define __EXPORT_SYMBOL(sym, license, ns) \
+ __ADDRESSABLE(sym) \
+ asm(__stringify(___EXPORT_SYMBOL(sym, license, ns)))
#endif /* CONFIG_MODULES */
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index bd4123795299..8154bd962eea 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -215,18 +215,12 @@ is-standard-object = $(if $(filter-out y%, $(OBJECT_FILES_NON_STANDARD_$(basetar
$(obj)/%.o: objtool-enabled = $(if $(is-standard-object),$(if $(delay-objtool),$(is-single-obj-m),y))
-ifdef CONFIG_TRIM_UNUSED_KSYMS
-cmd_gen_ksymdeps = \
- $(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ >> $(dot-target).cmd
-endif
-
ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
cmd_warn_shared_object = $(if $(word 2, $(modname-multi)),$(warning $(kbuild-file): $*.o is added to multiple modules: $(modname-multi)))
endif
define rule_cc_o_c
$(call cmd_and_fixdep,cc_o_c)
- $(call cmd,gen_ksymdeps)
$(call cmd,checksrc)
$(call cmd,checkdoc)
$(call cmd,gen_objtooldep)
@@ -237,7 +231,6 @@ endef
define rule_as_o_S
$(call cmd_and_fixdep,as_o_S)
- $(call cmd,gen_ksymdeps)
$(call cmd,gen_objtooldep)
$(call cmd,gen_symversions_S)
$(call cmd,warn_shared_object)
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 0980c58d8afc..1e0b47cbabd9 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -90,6 +90,13 @@ targets += .vmlinux.objs
.vmlinux.objs: vmlinux.a $(KBUILD_VMLINUX_LIBS) FORCE
$(call if_changed,vmlinux_objs)
+ifdef CONFIG_TRIM_UNUSED_KSYMS
+ksym-wl := $(CONFIG_UNUSED_KSYMS_WHITELIST)
+ksym-wl := $(if $(filter-out /%, $(ksym-wl)),$(srctree)/)$(ksym-wl)
+modpost-args += -t $(addprefix -W, $(ksym-wl))
+modpost-deps += $(ksym-wl)
+endif
+
ifeq ($(wildcard vmlinux.o),)
missing-input := vmlinux.o
output-symdump := modules-only.symvers
diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
deleted file mode 100755
index f1b5ac818411..000000000000
--- a/scripts/adjust_autoksyms.sh
+++ /dev/null
@@ -1,73 +0,0 @@
-#!/bin/sh
-# SPDX-License-Identifier: GPL-2.0-only
-
-# Script to update include/generated/autoksyms.h and dependency files
-#
-# Copyright: (C) 2016 Linaro Limited
-# Created by: Nicolas Pitre, January 2016
-#
-
-# Update the include/generated/autoksyms.h file.
-#
-# For each symbol being added or removed, the corresponding dependency
-# file's timestamp is updated to force a rebuild of the affected source
-# file. All arguments passed to this script are assumed to be a command
-# to be exec'd to trigger a rebuild of those files.
-
-set -e
-
-cur_ksyms_file="include/generated/autoksyms.h"
-new_ksyms_file="include/generated/autoksyms.h.tmpnew"
-
-info() {
- if [ "$quiet" != "silent_" ]; then
- printf " %-7s %s\n" "$1" "$2"
- fi
-}
-
-info "CHK" "$cur_ksyms_file"
-
-# Use "make V=1" to debug this script.
-case "$KBUILD_VERBOSE" in
-*1*)
- set -x
- ;;
-esac
-
-# Generate a new symbol list file
-$CONFIG_SHELL $srctree/scripts/gen_autoksyms.sh --modorder "$new_ksyms_file"
-
-# Extract changes between old and new list and touch corresponding
-# dependency files.
-changed=$(
-count=0
-sort "$cur_ksyms_file" "$new_ksyms_file" | uniq -u |
-sed -n 's/^#define __KSYM_\(.*\) 1/\1/p' |
-while read sympath; do
- if [ -z "$sympath" ]; then continue; fi
- depfile="include/ksym/${sympath}"
- mkdir -p "$(dirname "$depfile")"
- touch "$depfile"
- # Filesystems with coarse time precision may create timestamps
- # equal to the one from a file that was very recently built and that
- # needs to be rebuild. Let's guard against that by making sure our
- # dep files are always newer than the first file we created here.
- while [ ! "$depfile" -nt "$new_ksyms_file" ]; do
- touch "$depfile"
- done
- echo $((count += 1))
-done | tail -1 )
-changed=${changed:-0}
-
-if [ $changed -gt 0 ]; then
- # Replace the old list with tne new one
- old=$(grep -c "^#define __KSYM_" "$cur_ksyms_file" || true)
- new=$(grep -c "^#define __KSYM_" "$new_ksyms_file" || true)
- info "KSYMS" "symbols: before=$old, after=$new, changed=$changed"
- info "UPD" "$cur_ksyms_file"
- mv -f "$new_ksyms_file" "$cur_ksyms_file"
- # Then trigger a rebuild of affected source files
- exec $@
-else
- rm -f "$new_ksyms_file"
-fi
diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index fa562806c2be..84b6efa849f4 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -246,8 +246,7 @@ static void *read_file(const char *filename)
/* Ignore certain dependencies */
static int is_ignored_file(const char *s, int len)
{
- return str_ends_with(s, len, "include/generated/autoconf.h") ||
- str_ends_with(s, len, "include/generated/autoksyms.h");
+ return str_ends_with(s, len, "include/generated/autoconf.h");
}
/* Do not parse these files */
diff --git a/scripts/gen_ksymdeps.sh b/scripts/gen_ksymdeps.sh
deleted file mode 100755
index 8ee533f33659..000000000000
--- a/scripts/gen_ksymdeps.sh
+++ /dev/null
@@ -1,30 +0,0 @@
-#!/bin/sh
-# SPDX-License-Identifier: GPL-2.0
-
-set -e
-
-# List of exported symbols
-#
-# If the object has no symbol, $NM warns 'no symbols'.
-# Suppress the stderr.
-# TODO:
-# Use -q instead of 2>/dev/null when we upgrade the minimum version of
-# binutils to 2.37, llvm to 13.0.0.
-ksyms=$($NM $1 2>/dev/null | sed -n 's/.*__ksym_marker_\(.*\)/\1/p')
-
-if [ -z "$ksyms" ]; then
- exit 0
-fi
-
-echo
-echo "ksymdeps_$1 := \\"
-
-for s in $ksyms
-do
- printf ' $(wildcard include/ksym/%s) \\\n' "$s"
-done
-
-echo
-echo "$1: \$(ksymdeps_$1)"
-echo
-echo "\$(ksymdeps_$1):"
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f14fe9301ae6..516323c3910a 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -35,6 +35,9 @@ static bool warn_unresolved;
static int sec_mismatch_count;
static bool sec_mismatch_warn_only = true;
+/* Trim EXPORT_SYMBOLs that are unused by in-tree modules */
+static bool trim_unused_exports;
+
/* ignore missing files */
static bool ignore_missing_files;
/* If set to 1, only warn (instead of error) about missing ns imports */
@@ -217,6 +220,7 @@ struct symbol {
bool weak;
bool is_func;
bool is_gpl_only; /* exported by EXPORT_SYMBOL_GPL */
+ bool used; /* there exists a user of this symbol */
char name[];
};
@@ -1772,6 +1776,7 @@ static void check_exports(struct module *mod)
continue;
}
+ exp->used = true;
s->module = exp->module;
s->crc_valid = exp->crc_valid;
s->crc = exp->crc;
@@ -1795,6 +1800,23 @@ static void check_exports(struct module *mod)
}
}
+static void handle_white_list_exports(const char *white_list)
+{
+ char *buf, *p, *name;
+
+ buf = read_text_file(white_list);
+ p = buf;
+
+ while ((name = strsep(&p, "\n"))) {
+ struct symbol *sym = find_symbol(name);
+
+ if (sym)
+ sym->used = true;
+ }
+
+ free(buf);
+}
+
static void check_modname_len(struct module *mod)
{
const char *mod_name;
@@ -1865,10 +1887,14 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod)
/* generate struct for exported symbols */
buf_printf(buf, "\n");
- list_for_each_entry(sym, &mod->exported_symbols, list)
+ list_for_each_entry(sym, &mod->exported_symbols, list) {
+ if (trim_unused_exports && !sym->used)
+ continue;
+
buf_printf(buf, "KSYMTAB_%s(%s, \"%s\", \"%s\");\n",
sym->is_func ? "FUNC" : "DATA", sym->name,
sym->is_gpl_only ? "_gpl" : "", sym->namespace);
+ }
if (!modversions)
return;
@@ -1876,6 +1902,9 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod)
/* record CRCs for exported symbols */
buf_printf(buf, "\n");
list_for_each_entry(sym, &mod->exported_symbols, list) {
+ if (trim_unused_exports && !sym->used)
+ continue;
+
if (!sym->crc_valid)
warn("EXPORT symbol \"%s\" [%s%s] version generation failed, symbol will not be versioned.\n"
"Is \"%s\" prototyped in <asm/asm-prototypes.h>?\n",
@@ -2039,9 +2068,6 @@ static void write_mod_c_file(struct module *mod)
char fname[PATH_MAX];
int ret;
- check_modname_len(mod);
- check_exports(mod);
-
add_header(&buf, mod);
add_exported_symbols(&buf, mod);
add_versions(&buf, mod);
@@ -2175,12 +2201,13 @@ int main(int argc, char **argv)
{
struct module *mod;
char *missing_namespace_deps = NULL;
+ char *unused_exports_white_list = NULL;
char *dump_write = NULL, *files_source = NULL;
int opt;
LIST_HEAD(dump_lists);
struct dump_list *dl, *dl2;
- while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) {
+ while ((opt = getopt(argc, argv, "ei:mntT:tW:o:awENd:")) != -1) {
switch (opt) {
case 'e':
external_module = true;
@@ -2205,6 +2232,12 @@ int main(int argc, char **argv)
case 'T':
files_source = optarg;
break;
+ case 't':
+ trim_unused_exports = true;
+ break;
+ case 'W':
+ unused_exports_white_list = optarg;
+ break;
case 'w':
warn_unresolved = true;
break;
@@ -2234,6 +2267,17 @@ int main(int argc, char **argv)
if (files_source)
read_symbols_from_files(files_source);
+ list_for_each_entry(mod, &modules, list) {
+ if (mod->from_dump || mod->is_vmlinux)
+ continue;
+
+ check_modname_len(mod);
+ check_exports(mod);
+ }
+
+ if (unused_exports_white_list)
+ handle_white_list_exports(unused_exports_white_list);
+
list_for_each_entry(mod, &modules, list) {
if (mod->from_dump)
continue;
diff --git a/scripts/remove-stale-files b/scripts/remove-stale-files
index 7f432900671a..8502a17d47df 100755
--- a/scripts/remove-stale-files
+++ b/scripts/remove-stale-files
@@ -33,3 +33,5 @@ rm -f rust/target.json
rm -f scripts/bin2c
rm -f .scmversion
+
+rm -rf include/ksym
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 15/20] kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion
2023-05-21 16:04 ` [PATCH v6 15/20] kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion Masahiro Yamada
@ 2023-05-25 18:14 ` Nick Desaulniers
2023-05-28 7:40 ` Masahiro Yamada
0 siblings, 1 reply; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-25 18:14 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier,
Nicolas Pitre
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> When CONFIG_TRIM_UNUSED_KSYMS is enabled, Kbuild recursively traverses
> the directory tree to determine which EXPORT_SYMBOL to trim. If an
> EXPORT_SYMBOL turns out to be unused by anyone, Kbuild begins the
> second traverse, where some source files are recompiled with their
> EXPORT_SYMBOL() tuned into a no-op.
>
> Linus stated negative opinions about this slowness in commits:
>
> - 5cf0fd591f2e ("Kbuild: disable TRIM_UNUSED_KSYMS option")
> - a555bdd0c58c ("Kbuild: enable TRIM_UNUSED_KSYMS again, with some guarding")
>
> We can do this better now. The final data structures of EXPORT_SYMBOL
> are generated by the modpost stage, so modpost can selectively emit
> KSYMTAB entries that are really used by modules.
>
> Commit f73edc8951b2 ("kbuild: unify two modpost invocations") is another
> ground-work to do this in a one-pass algorithm. With the list of modules,
> modpost sets sym->used if it is used by a module. modpost emits KSYMTAB
> only for symbols with sym->used==true.
>
> BTW, Nicolas explained why the trimming was implemented with recursion:
>
> https://lore.kernel.org/all/2o2rpn97-79nq-p7s2-nq5-8p83391473r@syhkavp.arg/
>
> Actually, we never achieved that level of optimization where the chain
> reaction of trimming comes into play because:
>
> - CONFIG_LTO_CLANG cannot remove any unused symbols
> - CONFIG_LD_DEAD_CODE_DATA_ELIMINATION is enabled only for vmlinux,
> but not modules
>
> If deeper trimming is required, we need to revisit this, but I guess
> that is unlikely to happen.
I think this patch removes the only 2 references to
scripts/gen_autoksyms.sh in the tree. Can or should that be removed as
well?
The rest of the patch LGTM.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> Changes in v5:
> - Clean up more
>
> .gitignore | 1 -
> Makefile | 19 +---------
> include/linux/export.h | 65 +++++----------------------------
> scripts/Makefile.build | 7 ----
> scripts/Makefile.modpost | 7 ++++
> scripts/adjust_autoksyms.sh | 73 -------------------------------------
> scripts/basic/fixdep.c | 3 +-
> scripts/gen_ksymdeps.sh | 30 ---------------
> scripts/mod/modpost.c | 54 ++++++++++++++++++++++++---
> scripts/remove-stale-files | 2 +
> 10 files changed, 70 insertions(+), 191 deletions(-)
> delete mode 100755 scripts/adjust_autoksyms.sh
> delete mode 100755 scripts/gen_ksymdeps.sh
>
> diff --git a/.gitignore b/.gitignore
> index 7f86e0837909..172e3874adfd 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -112,7 +112,6 @@ modules.order
> #
> /include/config/
> /include/generated/
> -/include/ksym/
> /arch/*/include/generated/
>
> # stgit generated dirs
> diff --git a/Makefile b/Makefile
> index f836936fb4d8..ffc2c9b632fd 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1193,28 +1193,13 @@ endif
> export KBUILD_VMLINUX_LIBS
> export KBUILD_LDS := arch/$(SRCARCH)/kernel/vmlinux.lds
>
> -# Recurse until adjust_autoksyms.sh is satisfied
> -PHONY += autoksyms_recursive
> ifdef CONFIG_TRIM_UNUSED_KSYMS
> # For the kernel to actually contain only the needed exported symbols,
> # we have to build modules as well to determine what those symbols are.
> # (this can be evaluated only once include/config/auto.conf has been included)
> KBUILD_MODULES := 1
> -
> -autoksyms_recursive: $(build-dir) modules.order
> - $(Q)$(CONFIG_SHELL) $(srctree)/scripts/adjust_autoksyms.sh \
> - "$(MAKE) -f $(srctree)/Makefile autoksyms_recursive"
> endif
>
> -autoksyms_h := $(if $(CONFIG_TRIM_UNUSED_KSYMS), include/generated/autoksyms.h)
> -
> -quiet_cmd_autoksyms_h = GEN $@
> - cmd_autoksyms_h = mkdir -p $(dir $@); \
> - $(CONFIG_SHELL) $(srctree)/scripts/gen_autoksyms.sh $@
> -
> -$(autoksyms_h):
> - $(call cmd,autoksyms_h)
> -
> # '$(AR) mPi' needs 'T' to workaround the bug of llvm-ar <= 14
> quiet_cmd_ar_vmlinux.a = AR $@
> cmd_ar_vmlinux.a = \
> @@ -1223,7 +1208,7 @@ quiet_cmd_ar_vmlinux.a = AR $@
> $(AR) mPiT $$($(AR) t $@ | sed -n 1p) $@ $$($(AR) t $@ | grep -F -f $(srctree)/scripts/head-object-list.txt)
>
> targets += vmlinux.a
> -vmlinux.a: $(KBUILD_VMLINUX_OBJS) scripts/head-object-list.txt autoksyms_recursive FORCE
> +vmlinux.a: $(KBUILD_VMLINUX_OBJS) scripts/head-object-list.txt FORCE
> $(call if_changed,ar_vmlinux.a)
>
> PHONY += vmlinux_o
> @@ -1279,7 +1264,7 @@ scripts: scripts_basic scripts_dtc
> PHONY += prepare archprepare
>
> archprepare: outputmakefile archheaders archscripts scripts include/config/kernel.release \
> - asm-generic $(version_h) $(autoksyms_h) include/generated/utsrelease.h \
> + asm-generic $(version_h) include/generated/utsrelease.h \
> include/generated/compile.h include/generated/autoconf.h remove-stale-files
>
> prepare0: archprepare
> diff --git a/include/linux/export.h b/include/linux/export.h
> index 32461a01608c..9bf081ff9903 100644
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -37,30 +37,13 @@ extern struct module __this_module;
> #define __EXPORT_SYMBOL_REF(sym) .balign 4; .long sym
> #endif
>
> -#define ____EXPORT_SYMBOL(sym, license, ns) \
> +#define ___EXPORT_SYMBOL(sym, license, ns) \
> .section ".export_symbol","a" ; \
> __export_symbol_##license##_##sym: ; \
> .asciz ns ; \
> __EXPORT_SYMBOL_REF(sym) ; \
> .previous
>
> -#ifdef __GENKSYMS__
> -
> -#define ___EXPORT_SYMBOL(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
> -
> -#elif defined(__ASSEMBLY__)
> -
> -#define ___EXPORT_SYMBOL(sym, license, ns) \
> - ____EXPORT_SYMBOL(sym, license, ns)
> -
> -#else
> -
> -#define ___EXPORT_SYMBOL(sym, license, ns) \
> - __ADDRESSABLE(sym) \
> - asm(__stringify(____EXPORT_SYMBOL(sym, license, ns)))
> -
> -#endif
> -
> #if !defined(CONFIG_MODULES) || defined(__DISABLE_EXPORTS)
>
> /*
> @@ -70,50 +53,20 @@ extern struct module __this_module;
> */
> #define __EXPORT_SYMBOL(sym, sec, ns)
>
> -#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
> +#elif defined(__GENKSYMS__)
>
> -#include <generated/autoksyms.h>
> +#define __EXPORT_SYMBOL(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
>
> -/*
> - * For fine grained build dependencies, we want to tell the build system
> - * about each possible exported symbol even if they're not actually exported.
> - * We use a symbol pattern __ksym_marker_<symbol> that the build system filters
> - * from the $(NM) output (see scripts/gen_ksymdeps.sh). These symbols are
> - * discarded in the final link stage.
> - */
> +#elif defined(__ASSEMBLY__)
>
> -#ifdef __ASSEMBLY__
> -
> -#define __ksym_marker(sym) \
> - .section ".discard.ksym","a" ; \
> -__ksym_marker_##sym: ; \
> - .previous
> +#define __EXPORT_SYMBOL(sym, license, ns) \
> + ___EXPORT_SYMBOL(sym, license, ns)
>
> #else
>
> -#define __ksym_marker(sym) \
> - static int __ksym_marker_##sym[0] __section(".discard.ksym") __used
> -
> -#endif
> -
> -#define __EXPORT_SYMBOL(sym, sec, ns) \
> - __ksym_marker(sym); \
> - __cond_export_sym(sym, sec, ns, __is_defined(__KSYM_##sym))
> -#define __cond_export_sym(sym, sec, ns, conf) \
> - ___cond_export_sym(sym, sec, ns, conf)
> -#define ___cond_export_sym(sym, sec, ns, enabled) \
> - __cond_export_sym_##enabled(sym, sec, ns)
> -#define __cond_export_sym_1(sym, sec, ns) ___EXPORT_SYMBOL(sym, sec, ns)
> -
> -#ifdef __GENKSYMS__
> -#define __cond_export_sym_0(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
> -#else
> -#define __cond_export_sym_0(sym, sec, ns) /* nothing */
> -#endif
> -
> -#else
> -
> -#define __EXPORT_SYMBOL(sym, sec, ns) ___EXPORT_SYMBOL(sym, sec, ns)
> +#define __EXPORT_SYMBOL(sym, license, ns) \
> + __ADDRESSABLE(sym) \
> + asm(__stringify(___EXPORT_SYMBOL(sym, license, ns)))
>
> #endif /* CONFIG_MODULES */
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index bd4123795299..8154bd962eea 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -215,18 +215,12 @@ is-standard-object = $(if $(filter-out y%, $(OBJECT_FILES_NON_STANDARD_$(basetar
>
> $(obj)/%.o: objtool-enabled = $(if $(is-standard-object),$(if $(delay-objtool),$(is-single-obj-m),y))
>
> -ifdef CONFIG_TRIM_UNUSED_KSYMS
> -cmd_gen_ksymdeps = \
> - $(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ >> $(dot-target).cmd
> -endif
> -
> ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
> cmd_warn_shared_object = $(if $(word 2, $(modname-multi)),$(warning $(kbuild-file): $*.o is added to multiple modules: $(modname-multi)))
> endif
>
> define rule_cc_o_c
> $(call cmd_and_fixdep,cc_o_c)
> - $(call cmd,gen_ksymdeps)
> $(call cmd,checksrc)
> $(call cmd,checkdoc)
> $(call cmd,gen_objtooldep)
> @@ -237,7 +231,6 @@ endef
>
> define rule_as_o_S
> $(call cmd_and_fixdep,as_o_S)
> - $(call cmd,gen_ksymdeps)
> $(call cmd,gen_objtooldep)
> $(call cmd,gen_symversions_S)
> $(call cmd,warn_shared_object)
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index 0980c58d8afc..1e0b47cbabd9 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -90,6 +90,13 @@ targets += .vmlinux.objs
> .vmlinux.objs: vmlinux.a $(KBUILD_VMLINUX_LIBS) FORCE
> $(call if_changed,vmlinux_objs)
>
> +ifdef CONFIG_TRIM_UNUSED_KSYMS
> +ksym-wl := $(CONFIG_UNUSED_KSYMS_WHITELIST)
> +ksym-wl := $(if $(filter-out /%, $(ksym-wl)),$(srctree)/)$(ksym-wl)
> +modpost-args += -t $(addprefix -W, $(ksym-wl))
> +modpost-deps += $(ksym-wl)
> +endif
> +
> ifeq ($(wildcard vmlinux.o),)
> missing-input := vmlinux.o
> output-symdump := modules-only.symvers
> diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
> deleted file mode 100755
> index f1b5ac818411..000000000000
> --- a/scripts/adjust_autoksyms.sh
> +++ /dev/null
> @@ -1,73 +0,0 @@
> -#!/bin/sh
> -# SPDX-License-Identifier: GPL-2.0-only
> -
> -# Script to update include/generated/autoksyms.h and dependency files
> -#
> -# Copyright: (C) 2016 Linaro Limited
> -# Created by: Nicolas Pitre, January 2016
> -#
> -
> -# Update the include/generated/autoksyms.h file.
> -#
> -# For each symbol being added or removed, the corresponding dependency
> -# file's timestamp is updated to force a rebuild of the affected source
> -# file. All arguments passed to this script are assumed to be a command
> -# to be exec'd to trigger a rebuild of those files.
> -
> -set -e
> -
> -cur_ksyms_file="include/generated/autoksyms.h"
> -new_ksyms_file="include/generated/autoksyms.h.tmpnew"
> -
> -info() {
> - if [ "$quiet" != "silent_" ]; then
> - printf " %-7s %s\n" "$1" "$2"
> - fi
> -}
> -
> -info "CHK" "$cur_ksyms_file"
> -
> -# Use "make V=1" to debug this script.
> -case "$KBUILD_VERBOSE" in
> -*1*)
> - set -x
> - ;;
> -esac
> -
> -# Generate a new symbol list file
> -$CONFIG_SHELL $srctree/scripts/gen_autoksyms.sh --modorder "$new_ksyms_file"
> -
> -# Extract changes between old and new list and touch corresponding
> -# dependency files.
> -changed=$(
> -count=0
> -sort "$cur_ksyms_file" "$new_ksyms_file" | uniq -u |
> -sed -n 's/^#define __KSYM_\(.*\) 1/\1/p' |
> -while read sympath; do
> - if [ -z "$sympath" ]; then continue; fi
> - depfile="include/ksym/${sympath}"
> - mkdir -p "$(dirname "$depfile")"
> - touch "$depfile"
> - # Filesystems with coarse time precision may create timestamps
> - # equal to the one from a file that was very recently built and that
> - # needs to be rebuild. Let's guard against that by making sure our
> - # dep files are always newer than the first file we created here.
> - while [ ! "$depfile" -nt "$new_ksyms_file" ]; do
> - touch "$depfile"
> - done
> - echo $((count += 1))
> -done | tail -1 )
> -changed=${changed:-0}
> -
> -if [ $changed -gt 0 ]; then
> - # Replace the old list with tne new one
> - old=$(grep -c "^#define __KSYM_" "$cur_ksyms_file" || true)
> - new=$(grep -c "^#define __KSYM_" "$new_ksyms_file" || true)
> - info "KSYMS" "symbols: before=$old, after=$new, changed=$changed"
> - info "UPD" "$cur_ksyms_file"
> - mv -f "$new_ksyms_file" "$cur_ksyms_file"
> - # Then trigger a rebuild of affected source files
> - exec $@
> -else
> - rm -f "$new_ksyms_file"
> -fi
> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> index fa562806c2be..84b6efa849f4 100644
> --- a/scripts/basic/fixdep.c
> +++ b/scripts/basic/fixdep.c
> @@ -246,8 +246,7 @@ static void *read_file(const char *filename)
> /* Ignore certain dependencies */
> static int is_ignored_file(const char *s, int len)
> {
> - return str_ends_with(s, len, "include/generated/autoconf.h") ||
> - str_ends_with(s, len, "include/generated/autoksyms.h");
> + return str_ends_with(s, len, "include/generated/autoconf.h");
> }
>
> /* Do not parse these files */
> diff --git a/scripts/gen_ksymdeps.sh b/scripts/gen_ksymdeps.sh
> deleted file mode 100755
> index 8ee533f33659..000000000000
> --- a/scripts/gen_ksymdeps.sh
> +++ /dev/null
> @@ -1,30 +0,0 @@
> -#!/bin/sh
> -# SPDX-License-Identifier: GPL-2.0
> -
> -set -e
> -
> -# List of exported symbols
> -#
> -# If the object has no symbol, $NM warns 'no symbols'.
> -# Suppress the stderr.
> -# TODO:
> -# Use -q instead of 2>/dev/null when we upgrade the minimum version of
> -# binutils to 2.37, llvm to 13.0.0.
> -ksyms=$($NM $1 2>/dev/null | sed -n 's/.*__ksym_marker_\(.*\)/\1/p')
> -
> -if [ -z "$ksyms" ]; then
> - exit 0
> -fi
> -
> -echo
> -echo "ksymdeps_$1 := \\"
> -
> -for s in $ksyms
> -do
> - printf ' $(wildcard include/ksym/%s) \\\n' "$s"
> -done
> -
> -echo
> -echo "$1: \$(ksymdeps_$1)"
> -echo
> -echo "\$(ksymdeps_$1):"
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index f14fe9301ae6..516323c3910a 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -35,6 +35,9 @@ static bool warn_unresolved;
>
> static int sec_mismatch_count;
> static bool sec_mismatch_warn_only = true;
> +/* Trim EXPORT_SYMBOLs that are unused by in-tree modules */
> +static bool trim_unused_exports;
> +
> /* ignore missing files */
> static bool ignore_missing_files;
> /* If set to 1, only warn (instead of error) about missing ns imports */
> @@ -217,6 +220,7 @@ struct symbol {
> bool weak;
> bool is_func;
> bool is_gpl_only; /* exported by EXPORT_SYMBOL_GPL */
> + bool used; /* there exists a user of this symbol */
> char name[];
> };
>
> @@ -1772,6 +1776,7 @@ static void check_exports(struct module *mod)
> continue;
> }
>
> + exp->used = true;
> s->module = exp->module;
> s->crc_valid = exp->crc_valid;
> s->crc = exp->crc;
> @@ -1795,6 +1800,23 @@ static void check_exports(struct module *mod)
> }
> }
>
> +static void handle_white_list_exports(const char *white_list)
> +{
> + char *buf, *p, *name;
> +
> + buf = read_text_file(white_list);
> + p = buf;
> +
> + while ((name = strsep(&p, "\n"))) {
> + struct symbol *sym = find_symbol(name);
> +
> + if (sym)
> + sym->used = true;
> + }
> +
> + free(buf);
> +}
> +
> static void check_modname_len(struct module *mod)
> {
> const char *mod_name;
> @@ -1865,10 +1887,14 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod)
>
> /* generate struct for exported symbols */
> buf_printf(buf, "\n");
> - list_for_each_entry(sym, &mod->exported_symbols, list)
> + list_for_each_entry(sym, &mod->exported_symbols, list) {
> + if (trim_unused_exports && !sym->used)
> + continue;
> +
> buf_printf(buf, "KSYMTAB_%s(%s, \"%s\", \"%s\");\n",
> sym->is_func ? "FUNC" : "DATA", sym->name,
> sym->is_gpl_only ? "_gpl" : "", sym->namespace);
> + }
>
> if (!modversions)
> return;
> @@ -1876,6 +1902,9 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod)
> /* record CRCs for exported symbols */
> buf_printf(buf, "\n");
> list_for_each_entry(sym, &mod->exported_symbols, list) {
> + if (trim_unused_exports && !sym->used)
> + continue;
> +
> if (!sym->crc_valid)
> warn("EXPORT symbol \"%s\" [%s%s] version generation failed, symbol will not be versioned.\n"
> "Is \"%s\" prototyped in <asm/asm-prototypes.h>?\n",
> @@ -2039,9 +2068,6 @@ static void write_mod_c_file(struct module *mod)
> char fname[PATH_MAX];
> int ret;
>
> - check_modname_len(mod);
> - check_exports(mod);
> -
> add_header(&buf, mod);
> add_exported_symbols(&buf, mod);
> add_versions(&buf, mod);
> @@ -2175,12 +2201,13 @@ int main(int argc, char **argv)
> {
> struct module *mod;
> char *missing_namespace_deps = NULL;
> + char *unused_exports_white_list = NULL;
> char *dump_write = NULL, *files_source = NULL;
> int opt;
> LIST_HEAD(dump_lists);
> struct dump_list *dl, *dl2;
>
> - while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) {
> + while ((opt = getopt(argc, argv, "ei:mntT:tW:o:awENd:")) != -1) {
> switch (opt) {
> case 'e':
> external_module = true;
> @@ -2205,6 +2232,12 @@ int main(int argc, char **argv)
> case 'T':
> files_source = optarg;
> break;
> + case 't':
> + trim_unused_exports = true;
> + break;
> + case 'W':
> + unused_exports_white_list = optarg;
> + break;
> case 'w':
> warn_unresolved = true;
> break;
> @@ -2234,6 +2267,17 @@ int main(int argc, char **argv)
> if (files_source)
> read_symbols_from_files(files_source);
>
> + list_for_each_entry(mod, &modules, list) {
> + if (mod->from_dump || mod->is_vmlinux)
> + continue;
> +
> + check_modname_len(mod);
> + check_exports(mod);
> + }
> +
> + if (unused_exports_white_list)
> + handle_white_list_exports(unused_exports_white_list);
> +
> list_for_each_entry(mod, &modules, list) {
> if (mod->from_dump)
> continue;
> diff --git a/scripts/remove-stale-files b/scripts/remove-stale-files
> index 7f432900671a..8502a17d47df 100755
> --- a/scripts/remove-stale-files
> +++ b/scripts/remove-stale-files
> @@ -33,3 +33,5 @@ rm -f rust/target.json
> rm -f scripts/bin2c
>
> rm -f .scmversion
> +
> +rm -rf include/ksym
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 15/20] kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion
2023-05-25 18:14 ` Nick Desaulniers
@ 2023-05-28 7:40 ` Masahiro Yamada
0 siblings, 0 replies; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-28 7:40 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier,
Nicolas Pitre
On Fri, May 26, 2023 at 3:15 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > When CONFIG_TRIM_UNUSED_KSYMS is enabled, Kbuild recursively traverses
> > the directory tree to determine which EXPORT_SYMBOL to trim. If an
> > EXPORT_SYMBOL turns out to be unused by anyone, Kbuild begins the
> > second traverse, where some source files are recompiled with their
> > EXPORT_SYMBOL() tuned into a no-op.
> >
> > Linus stated negative opinions about this slowness in commits:
> >
> > - 5cf0fd591f2e ("Kbuild: disable TRIM_UNUSED_KSYMS option")
> > - a555bdd0c58c ("Kbuild: enable TRIM_UNUSED_KSYMS again, with some guarding")
> >
> > We can do this better now. The final data structures of EXPORT_SYMBOL
> > are generated by the modpost stage, so modpost can selectively emit
> > KSYMTAB entries that are really used by modules.
> >
> > Commit f73edc8951b2 ("kbuild: unify two modpost invocations") is another
> > ground-work to do this in a one-pass algorithm. With the list of modules,
> > modpost sets sym->used if it is used by a module. modpost emits KSYMTAB
> > only for symbols with sym->used==true.
> >
> > BTW, Nicolas explained why the trimming was implemented with recursion:
> >
> > https://lore.kernel.org/all/2o2rpn97-79nq-p7s2-nq5-8p83391473r@syhkavp.arg/
> >
> > Actually, we never achieved that level of optimization where the chain
> > reaction of trimming comes into play because:
> >
> > - CONFIG_LTO_CLANG cannot remove any unused symbols
> > - CONFIG_LD_DEAD_CODE_DATA_ELIMINATION is enabled only for vmlinux,
> > but not modules
> >
> > If deeper trimming is required, we need to revisit this, but I guess
> > that is unlikely to happen.
>
> I think this patch removes the only 2 references to
> scripts/gen_autoksyms.sh in the tree. Can or should that be removed as
> well?
Good catch.
That script is no longer needed.
I will remove it too.
> The rest of the patch LGTM.
>
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> > Changes in v5:
> > - Clean up more
> >
> > .gitignore | 1 -
> > Makefile | 19 +---------
> > include/linux/export.h | 65 +++++----------------------------
> > scripts/Makefile.build | 7 ----
> > scripts/Makefile.modpost | 7 ++++
> > scripts/adjust_autoksyms.sh | 73 -------------------------------------
> > scripts/basic/fixdep.c | 3 +-
> > scripts/gen_ksymdeps.sh | 30 ---------------
> > scripts/mod/modpost.c | 54 ++++++++++++++++++++++++---
> > scripts/remove-stale-files | 2 +
> > 10 files changed, 70 insertions(+), 191 deletions(-)
> > delete mode 100755 scripts/adjust_autoksyms.sh
> > delete mode 100755 scripts/gen_ksymdeps.sh
> >
> > diff --git a/.gitignore b/.gitignore
> > index 7f86e0837909..172e3874adfd 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -112,7 +112,6 @@ modules.order
> > #
> > /include/config/
> > /include/generated/
> > -/include/ksym/
> > /arch/*/include/generated/
> >
> > # stgit generated dirs
> > diff --git a/Makefile b/Makefile
> > index f836936fb4d8..ffc2c9b632fd 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1193,28 +1193,13 @@ endif
> > export KBUILD_VMLINUX_LIBS
> > export KBUILD_LDS := arch/$(SRCARCH)/kernel/vmlinux.lds
> >
> > -# Recurse until adjust_autoksyms.sh is satisfied
> > -PHONY += autoksyms_recursive
> > ifdef CONFIG_TRIM_UNUSED_KSYMS
> > # For the kernel to actually contain only the needed exported symbols,
> > # we have to build modules as well to determine what those symbols are.
> > # (this can be evaluated only once include/config/auto.conf has been included)
> > KBUILD_MODULES := 1
> > -
> > -autoksyms_recursive: $(build-dir) modules.order
> > - $(Q)$(CONFIG_SHELL) $(srctree)/scripts/adjust_autoksyms.sh \
> > - "$(MAKE) -f $(srctree)/Makefile autoksyms_recursive"
> > endif
> >
> > -autoksyms_h := $(if $(CONFIG_TRIM_UNUSED_KSYMS), include/generated/autoksyms.h)
> > -
> > -quiet_cmd_autoksyms_h = GEN $@
> > - cmd_autoksyms_h = mkdir -p $(dir $@); \
> > - $(CONFIG_SHELL) $(srctree)/scripts/gen_autoksyms.sh $@
> > -
> > -$(autoksyms_h):
> > - $(call cmd,autoksyms_h)
> > -
> > # '$(AR) mPi' needs 'T' to workaround the bug of llvm-ar <= 14
> > quiet_cmd_ar_vmlinux.a = AR $@
> > cmd_ar_vmlinux.a = \
> > @@ -1223,7 +1208,7 @@ quiet_cmd_ar_vmlinux.a = AR $@
> > $(AR) mPiT $$($(AR) t $@ | sed -n 1p) $@ $$($(AR) t $@ | grep -F -f $(srctree)/scripts/head-object-list.txt)
> >
> > targets += vmlinux.a
> > -vmlinux.a: $(KBUILD_VMLINUX_OBJS) scripts/head-object-list.txt autoksyms_recursive FORCE
> > +vmlinux.a: $(KBUILD_VMLINUX_OBJS) scripts/head-object-list.txt FORCE
> > $(call if_changed,ar_vmlinux.a)
> >
> > PHONY += vmlinux_o
> > @@ -1279,7 +1264,7 @@ scripts: scripts_basic scripts_dtc
> > PHONY += prepare archprepare
> >
> > archprepare: outputmakefile archheaders archscripts scripts include/config/kernel.release \
> > - asm-generic $(version_h) $(autoksyms_h) include/generated/utsrelease.h \
> > + asm-generic $(version_h) include/generated/utsrelease.h \
> > include/generated/compile.h include/generated/autoconf.h remove-stale-files
> >
> > prepare0: archprepare
> > diff --git a/include/linux/export.h b/include/linux/export.h
> > index 32461a01608c..9bf081ff9903 100644
> > --- a/include/linux/export.h
> > +++ b/include/linux/export.h
> > @@ -37,30 +37,13 @@ extern struct module __this_module;
> > #define __EXPORT_SYMBOL_REF(sym) .balign 4; .long sym
> > #endif
> >
> > -#define ____EXPORT_SYMBOL(sym, license, ns) \
> > +#define ___EXPORT_SYMBOL(sym, license, ns) \
> > .section ".export_symbol","a" ; \
> > __export_symbol_##license##_##sym: ; \
> > .asciz ns ; \
> > __EXPORT_SYMBOL_REF(sym) ; \
> > .previous
> >
> > -#ifdef __GENKSYMS__
> > -
> > -#define ___EXPORT_SYMBOL(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
> > -
> > -#elif defined(__ASSEMBLY__)
> > -
> > -#define ___EXPORT_SYMBOL(sym, license, ns) \
> > - ____EXPORT_SYMBOL(sym, license, ns)
> > -
> > -#else
> > -
> > -#define ___EXPORT_SYMBOL(sym, license, ns) \
> > - __ADDRESSABLE(sym) \
> > - asm(__stringify(____EXPORT_SYMBOL(sym, license, ns)))
> > -
> > -#endif
> > -
> > #if !defined(CONFIG_MODULES) || defined(__DISABLE_EXPORTS)
> >
> > /*
> > @@ -70,50 +53,20 @@ extern struct module __this_module;
> > */
> > #define __EXPORT_SYMBOL(sym, sec, ns)
> >
> > -#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
> > +#elif defined(__GENKSYMS__)
> >
> > -#include <generated/autoksyms.h>
> > +#define __EXPORT_SYMBOL(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
> >
> > -/*
> > - * For fine grained build dependencies, we want to tell the build system
> > - * about each possible exported symbol even if they're not actually exported.
> > - * We use a symbol pattern __ksym_marker_<symbol> that the build system filters
> > - * from the $(NM) output (see scripts/gen_ksymdeps.sh). These symbols are
> > - * discarded in the final link stage.
> > - */
> > +#elif defined(__ASSEMBLY__)
> >
> > -#ifdef __ASSEMBLY__
> > -
> > -#define __ksym_marker(sym) \
> > - .section ".discard.ksym","a" ; \
> > -__ksym_marker_##sym: ; \
> > - .previous
> > +#define __EXPORT_SYMBOL(sym, license, ns) \
> > + ___EXPORT_SYMBOL(sym, license, ns)
> >
> > #else
> >
> > -#define __ksym_marker(sym) \
> > - static int __ksym_marker_##sym[0] __section(".discard.ksym") __used
> > -
> > -#endif
> > -
> > -#define __EXPORT_SYMBOL(sym, sec, ns) \
> > - __ksym_marker(sym); \
> > - __cond_export_sym(sym, sec, ns, __is_defined(__KSYM_##sym))
> > -#define __cond_export_sym(sym, sec, ns, conf) \
> > - ___cond_export_sym(sym, sec, ns, conf)
> > -#define ___cond_export_sym(sym, sec, ns, enabled) \
> > - __cond_export_sym_##enabled(sym, sec, ns)
> > -#define __cond_export_sym_1(sym, sec, ns) ___EXPORT_SYMBOL(sym, sec, ns)
> > -
> > -#ifdef __GENKSYMS__
> > -#define __cond_export_sym_0(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
> > -#else
> > -#define __cond_export_sym_0(sym, sec, ns) /* nothing */
> > -#endif
> > -
> > -#else
> > -
> > -#define __EXPORT_SYMBOL(sym, sec, ns) ___EXPORT_SYMBOL(sym, sec, ns)
> > +#define __EXPORT_SYMBOL(sym, license, ns) \
> > + __ADDRESSABLE(sym) \
> > + asm(__stringify(___EXPORT_SYMBOL(sym, license, ns)))
> >
> > #endif /* CONFIG_MODULES */
> >
> > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > index bd4123795299..8154bd962eea 100644
> > --- a/scripts/Makefile.build
> > +++ b/scripts/Makefile.build
> > @@ -215,18 +215,12 @@ is-standard-object = $(if $(filter-out y%, $(OBJECT_FILES_NON_STANDARD_$(basetar
> >
> > $(obj)/%.o: objtool-enabled = $(if $(is-standard-object),$(if $(delay-objtool),$(is-single-obj-m),y))
> >
> > -ifdef CONFIG_TRIM_UNUSED_KSYMS
> > -cmd_gen_ksymdeps = \
> > - $(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ >> $(dot-target).cmd
> > -endif
> > -
> > ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
> > cmd_warn_shared_object = $(if $(word 2, $(modname-multi)),$(warning $(kbuild-file): $*.o is added to multiple modules: $(modname-multi)))
> > endif
> >
> > define rule_cc_o_c
> > $(call cmd_and_fixdep,cc_o_c)
> > - $(call cmd,gen_ksymdeps)
> > $(call cmd,checksrc)
> > $(call cmd,checkdoc)
> > $(call cmd,gen_objtooldep)
> > @@ -237,7 +231,6 @@ endef
> >
> > define rule_as_o_S
> > $(call cmd_and_fixdep,as_o_S)
> > - $(call cmd,gen_ksymdeps)
> > $(call cmd,gen_objtooldep)
> > $(call cmd,gen_symversions_S)
> > $(call cmd,warn_shared_object)
> > diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> > index 0980c58d8afc..1e0b47cbabd9 100644
> > --- a/scripts/Makefile.modpost
> > +++ b/scripts/Makefile.modpost
> > @@ -90,6 +90,13 @@ targets += .vmlinux.objs
> > .vmlinux.objs: vmlinux.a $(KBUILD_VMLINUX_LIBS) FORCE
> > $(call if_changed,vmlinux_objs)
> >
> > +ifdef CONFIG_TRIM_UNUSED_KSYMS
> > +ksym-wl := $(CONFIG_UNUSED_KSYMS_WHITELIST)
> > +ksym-wl := $(if $(filter-out /%, $(ksym-wl)),$(srctree)/)$(ksym-wl)
> > +modpost-args += -t $(addprefix -W, $(ksym-wl))
> > +modpost-deps += $(ksym-wl)
> > +endif
> > +
> > ifeq ($(wildcard vmlinux.o),)
> > missing-input := vmlinux.o
> > output-symdump := modules-only.symvers
> > diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
> > deleted file mode 100755
> > index f1b5ac818411..000000000000
> > --- a/scripts/adjust_autoksyms.sh
> > +++ /dev/null
> > @@ -1,73 +0,0 @@
> > -#!/bin/sh
> > -# SPDX-License-Identifier: GPL-2.0-only
> > -
> > -# Script to update include/generated/autoksyms.h and dependency files
> > -#
> > -# Copyright: (C) 2016 Linaro Limited
> > -# Created by: Nicolas Pitre, January 2016
> > -#
> > -
> > -# Update the include/generated/autoksyms.h file.
> > -#
> > -# For each symbol being added or removed, the corresponding dependency
> > -# file's timestamp is updated to force a rebuild of the affected source
> > -# file. All arguments passed to this script are assumed to be a command
> > -# to be exec'd to trigger a rebuild of those files.
> > -
> > -set -e
> > -
> > -cur_ksyms_file="include/generated/autoksyms.h"
> > -new_ksyms_file="include/generated/autoksyms.h.tmpnew"
> > -
> > -info() {
> > - if [ "$quiet" != "silent_" ]; then
> > - printf " %-7s %s\n" "$1" "$2"
> > - fi
> > -}
> > -
> > -info "CHK" "$cur_ksyms_file"
> > -
> > -# Use "make V=1" to debug this script.
> > -case "$KBUILD_VERBOSE" in
> > -*1*)
> > - set -x
> > - ;;
> > -esac
> > -
> > -# Generate a new symbol list file
> > -$CONFIG_SHELL $srctree/scripts/gen_autoksyms.sh --modorder "$new_ksyms_file"
> > -
> > -# Extract changes between old and new list and touch corresponding
> > -# dependency files.
> > -changed=$(
> > -count=0
> > -sort "$cur_ksyms_file" "$new_ksyms_file" | uniq -u |
> > -sed -n 's/^#define __KSYM_\(.*\) 1/\1/p' |
> > -while read sympath; do
> > - if [ -z "$sympath" ]; then continue; fi
> > - depfile="include/ksym/${sympath}"
> > - mkdir -p "$(dirname "$depfile")"
> > - touch "$depfile"
> > - # Filesystems with coarse time precision may create timestamps
> > - # equal to the one from a file that was very recently built and that
> > - # needs to be rebuild. Let's guard against that by making sure our
> > - # dep files are always newer than the first file we created here.
> > - while [ ! "$depfile" -nt "$new_ksyms_file" ]; do
> > - touch "$depfile"
> > - done
> > - echo $((count += 1))
> > -done | tail -1 )
> > -changed=${changed:-0}
> > -
> > -if [ $changed -gt 0 ]; then
> > - # Replace the old list with tne new one
> > - old=$(grep -c "^#define __KSYM_" "$cur_ksyms_file" || true)
> > - new=$(grep -c "^#define __KSYM_" "$new_ksyms_file" || true)
> > - info "KSYMS" "symbols: before=$old, after=$new, changed=$changed"
> > - info "UPD" "$cur_ksyms_file"
> > - mv -f "$new_ksyms_file" "$cur_ksyms_file"
> > - # Then trigger a rebuild of affected source files
> > - exec $@
> > -else
> > - rm -f "$new_ksyms_file"
> > -fi
> > diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> > index fa562806c2be..84b6efa849f4 100644
> > --- a/scripts/basic/fixdep.c
> > +++ b/scripts/basic/fixdep.c
> > @@ -246,8 +246,7 @@ static void *read_file(const char *filename)
> > /* Ignore certain dependencies */
> > static int is_ignored_file(const char *s, int len)
> > {
> > - return str_ends_with(s, len, "include/generated/autoconf.h") ||
> > - str_ends_with(s, len, "include/generated/autoksyms.h");
> > + return str_ends_with(s, len, "include/generated/autoconf.h");
> > }
> >
> > /* Do not parse these files */
> > diff --git a/scripts/gen_ksymdeps.sh b/scripts/gen_ksymdeps.sh
> > deleted file mode 100755
> > index 8ee533f33659..000000000000
> > --- a/scripts/gen_ksymdeps.sh
> > +++ /dev/null
> > @@ -1,30 +0,0 @@
> > -#!/bin/sh
> > -# SPDX-License-Identifier: GPL-2.0
> > -
> > -set -e
> > -
> > -# List of exported symbols
> > -#
> > -# If the object has no symbol, $NM warns 'no symbols'.
> > -# Suppress the stderr.
> > -# TODO:
> > -# Use -q instead of 2>/dev/null when we upgrade the minimum version of
> > -# binutils to 2.37, llvm to 13.0.0.
> > -ksyms=$($NM $1 2>/dev/null | sed -n 's/.*__ksym_marker_\(.*\)/\1/p')
> > -
> > -if [ -z "$ksyms" ]; then
> > - exit 0
> > -fi
> > -
> > -echo
> > -echo "ksymdeps_$1 := \\"
> > -
> > -for s in $ksyms
> > -do
> > - printf ' $(wildcard include/ksym/%s) \\\n' "$s"
> > -done
> > -
> > -echo
> > -echo "$1: \$(ksymdeps_$1)"
> > -echo
> > -echo "\$(ksymdeps_$1):"
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index f14fe9301ae6..516323c3910a 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -35,6 +35,9 @@ static bool warn_unresolved;
> >
> > static int sec_mismatch_count;
> > static bool sec_mismatch_warn_only = true;
> > +/* Trim EXPORT_SYMBOLs that are unused by in-tree modules */
> > +static bool trim_unused_exports;
> > +
> > /* ignore missing files */
> > static bool ignore_missing_files;
> > /* If set to 1, only warn (instead of error) about missing ns imports */
> > @@ -217,6 +220,7 @@ struct symbol {
> > bool weak;
> > bool is_func;
> > bool is_gpl_only; /* exported by EXPORT_SYMBOL_GPL */
> > + bool used; /* there exists a user of this symbol */
> > char name[];
> > };
> >
> > @@ -1772,6 +1776,7 @@ static void check_exports(struct module *mod)
> > continue;
> > }
> >
> > + exp->used = true;
> > s->module = exp->module;
> > s->crc_valid = exp->crc_valid;
> > s->crc = exp->crc;
> > @@ -1795,6 +1800,23 @@ static void check_exports(struct module *mod)
> > }
> > }
> >
> > +static void handle_white_list_exports(const char *white_list)
> > +{
> > + char *buf, *p, *name;
> > +
> > + buf = read_text_file(white_list);
> > + p = buf;
> > +
> > + while ((name = strsep(&p, "\n"))) {
> > + struct symbol *sym = find_symbol(name);
> > +
> > + if (sym)
> > + sym->used = true;
> > + }
> > +
> > + free(buf);
> > +}
> > +
> > static void check_modname_len(struct module *mod)
> > {
> > const char *mod_name;
> > @@ -1865,10 +1887,14 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod)
> >
> > /* generate struct for exported symbols */
> > buf_printf(buf, "\n");
> > - list_for_each_entry(sym, &mod->exported_symbols, list)
> > + list_for_each_entry(sym, &mod->exported_symbols, list) {
> > + if (trim_unused_exports && !sym->used)
> > + continue;
> > +
> > buf_printf(buf, "KSYMTAB_%s(%s, \"%s\", \"%s\");\n",
> > sym->is_func ? "FUNC" : "DATA", sym->name,
> > sym->is_gpl_only ? "_gpl" : "", sym->namespace);
> > + }
> >
> > if (!modversions)
> > return;
> > @@ -1876,6 +1902,9 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod)
> > /* record CRCs for exported symbols */
> > buf_printf(buf, "\n");
> > list_for_each_entry(sym, &mod->exported_symbols, list) {
> > + if (trim_unused_exports && !sym->used)
> > + continue;
> > +
> > if (!sym->crc_valid)
> > warn("EXPORT symbol \"%s\" [%s%s] version generation failed, symbol will not be versioned.\n"
> > "Is \"%s\" prototyped in <asm/asm-prototypes.h>?\n",
> > @@ -2039,9 +2068,6 @@ static void write_mod_c_file(struct module *mod)
> > char fname[PATH_MAX];
> > int ret;
> >
> > - check_modname_len(mod);
> > - check_exports(mod);
> > -
> > add_header(&buf, mod);
> > add_exported_symbols(&buf, mod);
> > add_versions(&buf, mod);
> > @@ -2175,12 +2201,13 @@ int main(int argc, char **argv)
> > {
> > struct module *mod;
> > char *missing_namespace_deps = NULL;
> > + char *unused_exports_white_list = NULL;
> > char *dump_write = NULL, *files_source = NULL;
> > int opt;
> > LIST_HEAD(dump_lists);
> > struct dump_list *dl, *dl2;
> >
> > - while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) {
> > + while ((opt = getopt(argc, argv, "ei:mntT:tW:o:awENd:")) != -1) {
> > switch (opt) {
> > case 'e':
> > external_module = true;
> > @@ -2205,6 +2232,12 @@ int main(int argc, char **argv)
> > case 'T':
> > files_source = optarg;
> > break;
> > + case 't':
> > + trim_unused_exports = true;
> > + break;
> > + case 'W':
> > + unused_exports_white_list = optarg;
> > + break;
> > case 'w':
> > warn_unresolved = true;
> > break;
> > @@ -2234,6 +2267,17 @@ int main(int argc, char **argv)
> > if (files_source)
> > read_symbols_from_files(files_source);
> >
> > + list_for_each_entry(mod, &modules, list) {
> > + if (mod->from_dump || mod->is_vmlinux)
> > + continue;
> > +
> > + check_modname_len(mod);
> > + check_exports(mod);
> > + }
> > +
> > + if (unused_exports_white_list)
> > + handle_white_list_exports(unused_exports_white_list);
> > +
> > list_for_each_entry(mod, &modules, list) {
> > if (mod->from_dump)
> > continue;
> > diff --git a/scripts/remove-stale-files b/scripts/remove-stale-files
> > index 7f432900671a..8502a17d47df 100755
> > --- a/scripts/remove-stale-files
> > +++ b/scripts/remove-stale-files
> > @@ -33,3 +33,5 @@ rm -f rust/target.json
> > rm -f scripts/bin2c
> >
> > rm -f .scmversion
> > +
> > +rm -rf include/ksym
> > --
> > 2.39.2
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 16/20] modpost: merge fromsec=DATA_SECTIONS entries in sectioncheck table
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (14 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 15/20] kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-25 18:30 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 17/20] modpost: merge bad_tosec=ALL_EXIT_SECTIONS " Masahiro Yamada
` (4 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
You can merge these entries.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 516323c3910a..aea9d6cd243d 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -884,12 +884,7 @@ static const struct sectioncheck sectioncheck[] = {
},
{
.fromsec = { DATA_SECTIONS, NULL },
- .bad_tosec = { ALL_XXXINIT_SECTIONS, NULL },
- .mismatch = DATA_TO_ANY_INIT,
-},
-{
- .fromsec = { DATA_SECTIONS, NULL },
- .bad_tosec = { INIT_SECTIONS, NULL },
+ .bad_tosec = { ALL_XXXINIT_SECTIONS, INIT_SECTIONS, NULL },
.mismatch = DATA_TO_ANY_INIT,
},
{
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 16/20] modpost: merge fromsec=DATA_SECTIONS entries in sectioncheck table
2023-05-21 16:04 ` [PATCH v6 16/20] modpost: merge fromsec=DATA_SECTIONS entries in sectioncheck table Masahiro Yamada
@ 2023-05-25 18:30 ` Nick Desaulniers
0 siblings, 0 replies; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-25 18:30 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> You can merge these entries.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>
> scripts/mod/modpost.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 516323c3910a..aea9d6cd243d 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -884,12 +884,7 @@ static const struct sectioncheck sectioncheck[] = {
> },
> {
> .fromsec = { DATA_SECTIONS, NULL },
> - .bad_tosec = { ALL_XXXINIT_SECTIONS, NULL },
> - .mismatch = DATA_TO_ANY_INIT,
> -},
> -{
> - .fromsec = { DATA_SECTIONS, NULL },
> - .bad_tosec = { INIT_SECTIONS, NULL },
> + .bad_tosec = { ALL_XXXINIT_SECTIONS, INIT_SECTIONS, NULL },
> .mismatch = DATA_TO_ANY_INIT,
> },
> {
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 17/20] modpost: merge bad_tosec=ALL_EXIT_SECTIONS entries in sectioncheck table
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (15 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 16/20] modpost: merge fromsec=DATA_SECTIONS entries in sectioncheck table Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-25 18:36 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 18/20] modpost: remove *_sections[] arrays Masahiro Yamada
` (3 subsequent siblings)
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
There is no distinction between TEXT_TO_ANY_EXIT and DATA_TO_ANY_EXIT.
Just merge them.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index aea9d6cd243d..b5f7f4811c39 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -844,8 +844,7 @@ static const char *const optim_symbols[] = { "*.constprop.*", NULL };
enum mismatch {
TEXT_TO_ANY_INIT,
DATA_TO_ANY_INIT,
- TEXT_TO_ANY_EXIT,
- DATA_TO_ANY_EXIT,
+ TEXTDATA_TO_ANY_EXIT,
XXXINIT_TO_SOME_INIT,
XXXEXIT_TO_SOME_EXIT,
ANY_INIT_TO_ANY_EXIT,
@@ -888,14 +887,9 @@ static const struct sectioncheck sectioncheck[] = {
.mismatch = DATA_TO_ANY_INIT,
},
{
- .fromsec = { TEXT_SECTIONS, NULL },
+ .fromsec = { TEXT_SECTIONS, DATA_SECTIONS, NULL },
.bad_tosec = { ALL_EXIT_SECTIONS, NULL },
- .mismatch = TEXT_TO_ANY_EXIT,
-},
-{
- .fromsec = { DATA_SECTIONS, NULL },
- .bad_tosec = { ALL_EXIT_SECTIONS, NULL },
- .mismatch = DATA_TO_ANY_EXIT,
+ .mismatch = TEXTDATA_TO_ANY_EXIT,
},
/* Do not reference init code/data from meminit code/data */
{
@@ -1162,8 +1156,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
switch (mismatch->mismatch) {
case TEXT_TO_ANY_INIT:
case DATA_TO_ANY_INIT:
- case TEXT_TO_ANY_EXIT:
- case DATA_TO_ANY_EXIT:
+ case TEXTDATA_TO_ANY_EXIT:
case XXXINIT_TO_SOME_INIT:
case XXXEXIT_TO_SOME_EXIT:
case ANY_INIT_TO_ANY_EXIT:
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 17/20] modpost: merge bad_tosec=ALL_EXIT_SECTIONS entries in sectioncheck table
2023-05-21 16:04 ` [PATCH v6 17/20] modpost: merge bad_tosec=ALL_EXIT_SECTIONS " Masahiro Yamada
@ 2023-05-25 18:36 ` Nick Desaulniers
2023-05-28 16:43 ` Masahiro Yamada
0 siblings, 1 reply; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-25 18:36 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> There is no distinction between TEXT_TO_ANY_EXIT and DATA_TO_ANY_EXIT.
> Just merge them.
Can something similar be done for ALL_PCI_INIT_SECTIONS and
ALL_XXXINIT_SECTIONS? (as a follow up)
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> scripts/mod/modpost.c | 15 ++++-----------
> 1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index aea9d6cd243d..b5f7f4811c39 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -844,8 +844,7 @@ static const char *const optim_symbols[] = { "*.constprop.*", NULL };
> enum mismatch {
> TEXT_TO_ANY_INIT,
> DATA_TO_ANY_INIT,
> - TEXT_TO_ANY_EXIT,
> - DATA_TO_ANY_EXIT,
> + TEXTDATA_TO_ANY_EXIT,
> XXXINIT_TO_SOME_INIT,
> XXXEXIT_TO_SOME_EXIT,
> ANY_INIT_TO_ANY_EXIT,
> @@ -888,14 +887,9 @@ static const struct sectioncheck sectioncheck[] = {
> .mismatch = DATA_TO_ANY_INIT,
> },
> {
> - .fromsec = { TEXT_SECTIONS, NULL },
> + .fromsec = { TEXT_SECTIONS, DATA_SECTIONS, NULL },
> .bad_tosec = { ALL_EXIT_SECTIONS, NULL },
> - .mismatch = TEXT_TO_ANY_EXIT,
> -},
> -{
> - .fromsec = { DATA_SECTIONS, NULL },
> - .bad_tosec = { ALL_EXIT_SECTIONS, NULL },
> - .mismatch = DATA_TO_ANY_EXIT,
> + .mismatch = TEXTDATA_TO_ANY_EXIT,
> },
> /* Do not reference init code/data from meminit code/data */
> {
> @@ -1162,8 +1156,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> switch (mismatch->mismatch) {
> case TEXT_TO_ANY_INIT:
> case DATA_TO_ANY_INIT:
> - case TEXT_TO_ANY_EXIT:
> - case DATA_TO_ANY_EXIT:
> + case TEXTDATA_TO_ANY_EXIT:
> case XXXINIT_TO_SOME_INIT:
> case XXXEXIT_TO_SOME_EXIT:
> case ANY_INIT_TO_ANY_EXIT:
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 17/20] modpost: merge bad_tosec=ALL_EXIT_SECTIONS entries in sectioncheck table
2023-05-25 18:36 ` Nick Desaulniers
@ 2023-05-28 16:43 ` Masahiro Yamada
0 siblings, 0 replies; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-28 16:43 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Fri, May 26, 2023 at 3:36 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > There is no distinction between TEXT_TO_ANY_EXIT and DATA_TO_ANY_EXIT.
> > Just merge them.
>
> Can something similar be done for ALL_PCI_INIT_SECTIONS and
> ALL_XXXINIT_SECTIONS? (as a follow up)
Yes, I think so.
BTW, the enum does not have a sensible name.
Commit bb15d8db7cce7 should have added
PCI_INIT_TO_ANY_INIT instead of
ANY_INIT_TO_ANY_EXIT.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 18/20] modpost: remove *_sections[] arrays
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (16 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 17/20] modpost: merge bad_tosec=ALL_EXIT_SECTIONS " Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-21 16:04 ` [PATCH v6 19/20] modpost: merge two similar section mismatch warnings Masahiro Yamada
` (2 subsequent siblings)
20 siblings, 0 replies; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
Use PATTERNS() macros to remove unneeded array definitions.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 36 +++++++++---------------------------
1 file changed, 9 insertions(+), 27 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index b5f7f4811c39..852cc164c77e 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -823,24 +823,6 @@ static void check_section(const char *modname, struct elf_info *elf,
#define ALL_TEXT_SECTIONS ALL_INIT_TEXT_SECTIONS, ALL_EXIT_TEXT_SECTIONS, \
TEXT_SECTIONS, OTHER_TEXT_SECTIONS
-/* init data sections */
-static const char *const init_data_sections[] =
- { ALL_INIT_DATA_SECTIONS, NULL };
-
-/* all init sections */
-static const char *const init_sections[] = { ALL_INIT_SECTIONS, NULL };
-
-/* all text sections */
-static const char *const text_sections[] = { ALL_TEXT_SECTIONS, NULL };
-
-/* data section */
-static const char *const data_sections[] = { DATA_SECTIONS, NULL };
-
-static const char *const head_sections[] = { ".head.text*", NULL };
-static const char *const linker_symbols[] =
- { "__init_begin", "_sinittext", "_einittext", NULL };
-static const char *const optim_symbols[] = { "*.constprop.*", NULL };
-
enum mismatch {
TEXT_TO_ANY_INIT,
DATA_TO_ANY_INIT,
@@ -1006,14 +988,14 @@ static int secref_whitelist(const char *fromsec, const char *fromsym,
const char *tosec, const char *tosym)
{
/* Check for pattern 1 */
- if (match(tosec, init_data_sections) &&
- match(fromsec, data_sections) &&
+ if (match(tosec, PATTERNS(ALL_INIT_DATA_SECTIONS)) &&
+ match(fromsec, PATTERNS(DATA_SECTIONS)) &&
strstarts(fromsym, "__param"))
return 0;
/* Check for pattern 1a */
if (strcmp(tosec, ".init.text") == 0 &&
- match(fromsec, data_sections) &&
+ match(fromsec, PATTERNS(DATA_SECTIONS)) &&
strstarts(fromsym, "__param_ops_"))
return 0;
@@ -1036,18 +1018,18 @@ static int secref_whitelist(const char *fromsec, const char *fromsym,
return 0;
/* Check for pattern 3 */
- if (match(fromsec, head_sections) &&
- match(tosec, init_sections))
+ if (strstarts(fromsec, ".head.text") &&
+ match(tosec, PATTERNS(ALL_INIT_SECTIONS)))
return 0;
/* Check for pattern 4 */
- if (match(tosym, linker_symbols))
+ if (match(tosym, PATTERNS("__init_begin", "_sinittext", "_einittext")))
return 0;
/* Check for pattern 5 */
- if (match(fromsec, text_sections) &&
- match(tosec, init_sections) &&
- match(fromsym, optim_symbols))
+ if (match(fromsec, PATTERNS(ALL_TEXT_SECTIONS)) &&
+ match(tosec, PATTERNS(ALL_INIT_SECTIONS)) &&
+ match(fromsym, PATTERNS("*.constprop.*")))
return 0;
return 1;
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH v6 19/20] modpost: merge two similar section mismatch warnings
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (17 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 18/20] modpost: remove *_sections[] arrays Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-25 18:20 ` Nick Desaulniers
2023-05-21 16:04 ` [PATCH v6 20/20] modpost: show offset from symbol for " Masahiro Yamada
2023-05-22 1:47 ` [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
In case of section mismatch, modpost shows slightly different messages.
For extable section mismatch:
"%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n"
For the other cases:
"%s: section mismatch in reference: %s (section: %s) -> %s (section: %s)\n"
They are similar. Merge them.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 18 +++---------------
1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 852cc164c77e..e7561fa57478 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1135,21 +1135,10 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
sec_mismatch_count++;
- switch (mismatch->mismatch) {
- case TEXT_TO_ANY_INIT:
- case DATA_TO_ANY_INIT:
- case TEXTDATA_TO_ANY_EXIT:
- case XXXINIT_TO_SOME_INIT:
- case XXXEXIT_TO_SOME_EXIT:
- case ANY_INIT_TO_ANY_EXIT:
- case ANY_EXIT_TO_ANY_INIT:
- warn("%s: section mismatch in reference: %s (section: %s) -> %s (section: %s)\n",
- modname, fromsym, fromsec, tosym, tosec);
- break;
- case EXTABLE_TO_NON_TEXT:
- warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
- modname, fromsec, (long)faddr, tosec, tosym);
+ warn("%s: section mismatch in reference: %s (section: %s) -> %s (section: %s)\n",
+ modname, fromsym, fromsec, tosym, tosec);
+ if (mismatch->mismatch == EXTABLE_TO_NON_TEXT) {
if (match(tosec, mismatch->bad_tosec))
fatal("The relocation at %s+0x%lx references\n"
"section \"%s\" which is black-listed.\n"
@@ -1169,7 +1158,6 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
else
error("%s+0x%lx references non-executable section '%s'\n",
fromsec, (long)faddr, tosec);
- break;
}
}
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 19/20] modpost: merge two similar section mismatch warnings
2023-05-21 16:04 ` [PATCH v6 19/20] modpost: merge two similar section mismatch warnings Masahiro Yamada
@ 2023-05-25 18:20 ` Nick Desaulniers
0 siblings, 0 replies; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-25 18:20 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> In case of section mismatch, modpost shows slightly different messages.
>
> For extable section mismatch:
>
> "%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n"
>
> For the other cases:
>
> "%s: section mismatch in reference: %s (section: %s) -> %s (section: %s)\n"
>
> They are similar. Merge them.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>
> scripts/mod/modpost.c | 18 +++---------------
> 1 file changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 852cc164c77e..e7561fa57478 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1135,21 +1135,10 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
>
> sec_mismatch_count++;
>
> - switch (mismatch->mismatch) {
> - case TEXT_TO_ANY_INIT:
> - case DATA_TO_ANY_INIT:
> - case TEXTDATA_TO_ANY_EXIT:
> - case XXXINIT_TO_SOME_INIT:
> - case XXXEXIT_TO_SOME_EXIT:
> - case ANY_INIT_TO_ANY_EXIT:
> - case ANY_EXIT_TO_ANY_INIT:
> - warn("%s: section mismatch in reference: %s (section: %s) -> %s (section: %s)\n",
> - modname, fromsym, fromsec, tosym, tosec);
> - break;
> - case EXTABLE_TO_NON_TEXT:
> - warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
> - modname, fromsec, (long)faddr, tosec, tosym);
> + warn("%s: section mismatch in reference: %s (section: %s) -> %s (section: %s)\n",
> + modname, fromsym, fromsec, tosym, tosec);
>
> + if (mismatch->mismatch == EXTABLE_TO_NON_TEXT) {
> if (match(tosec, mismatch->bad_tosec))
> fatal("The relocation at %s+0x%lx references\n"
> "section \"%s\" which is black-listed.\n"
> @@ -1169,7 +1158,6 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> else
> error("%s+0x%lx references non-executable section '%s'\n",
> fromsec, (long)faddr, tosec);
> - break;
> }
> }
>
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v6 20/20] modpost: show offset from symbol for section mismatch warnings
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (18 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 19/20] modpost: merge two similar section mismatch warnings Masahiro Yamada
@ 2023-05-21 16:04 ` Masahiro Yamada
2023-05-25 18:26 ` Nick Desaulniers
2023-05-22 1:47 ` [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
20 siblings, 1 reply; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-21 16:04 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Masahiro Yamada
Currently, modpost only shows the symbol names and section names, so it
repeats the same message if there are multiple relocations in the same
symbol. It is common the relocation spans across multiple instructions.
It is better to show the offset from the symbol.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
scripts/mod/modpost.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index e7561fa57478..4da96746a03b 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1135,8 +1135,8 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
sec_mismatch_count++;
- warn("%s: section mismatch in reference: %s (section: %s) -> %s (section: %s)\n",
- modname, fromsym, fromsec, tosym, tosec);
+ warn("%s: section mismatch in reference: %s+0x%x (section: %s) -> %s (section: %s)\n",
+ modname, fromsym, (unsigned int)(faddr - from->st_value), fromsec, tosym, tosec);
if (mismatch->mismatch == EXTABLE_TO_NON_TEXT) {
if (match(tosec, mismatch->bad_tosec))
--
2.39.2
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 20/20] modpost: show offset from symbol for section mismatch warnings
2023-05-21 16:04 ` [PATCH v6 20/20] modpost: show offset from symbol for " Masahiro Yamada
@ 2023-05-25 18:26 ` Nick Desaulniers
2023-05-28 7:29 ` Masahiro Yamada
0 siblings, 1 reply; 51+ messages in thread
From: Nick Desaulniers @ 2023-05-25 18:26 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Currently, modpost only shows the symbol names and section names, so it
> repeats the same message if there are multiple relocations in the same
> symbol. It is common the relocation spans across multiple instructions.
>
> It is better to show the offset from the symbol.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> scripts/mod/modpost.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index e7561fa57478..4da96746a03b 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1135,8 +1135,8 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
>
> sec_mismatch_count++;
>
> - warn("%s: section mismatch in reference: %s (section: %s) -> %s (section: %s)\n",
> - modname, fromsym, fromsec, tosym, tosec);
> + warn("%s: section mismatch in reference: %s+0x%x (section: %s) -> %s (section: %s)\n",
> + modname, fromsym, (unsigned int)(faddr - from->st_value), fromsec, tosym, tosec);
Is the cast necessary if you use the %p format specifier instead of 0x%x?
>
> if (mismatch->mismatch == EXTABLE_TO_NON_TEXT) {
> if (match(tosec, mismatch->bad_tosec))
> --
> 2.39.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 20/20] modpost: show offset from symbol for section mismatch warnings
2023-05-25 18:26 ` Nick Desaulniers
@ 2023-05-28 7:29 ` Masahiro Yamada
0 siblings, 0 replies; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-28 7:29 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-kbuild, linux-kernel, Nathan Chancellor, Nicolas Schier
On Fri, May 26, 2023 at 3:27 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > Currently, modpost only shows the symbol names and section names, so it
> > repeats the same message if there are multiple relocations in the same
> > symbol. It is common the relocation spans across multiple instructions.
> >
> > It is better to show the offset from the symbol.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> > scripts/mod/modpost.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index e7561fa57478..4da96746a03b 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1135,8 +1135,8 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> >
> > sec_mismatch_count++;
> >
> > - warn("%s: section mismatch in reference: %s (section: %s) -> %s (section: %s)\n",
> > - modname, fromsym, fromsec, tosym, tosec);
> > + warn("%s: section mismatch in reference: %s+0x%x (section: %s) -> %s (section: %s)\n",
> > + modname, fromsym, (unsigned int)(faddr - from->st_value), fromsec, tosym, tosec);
>
> Is the cast necessary if you use the %p format specifier instead of 0x%x?
No.
faddr and from->st_value are offsets from
the start of the section. They are not pointers.
%p does not make sense.
>
> >
> > if (mismatch->mismatch == EXTABLE_TO_NON_TEXT) {
> > if (match(tosec, mismatch->bad_tosec))
> > --
> > 2.39.2
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS
2023-05-21 16:04 [PATCH v6 00/20] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS Masahiro Yamada
` (19 preceding siblings ...)
2023-05-21 16:04 ` [PATCH v6 20/20] modpost: show offset from symbol for " Masahiro Yamada
@ 2023-05-22 1:47 ` Masahiro Yamada
20 siblings, 0 replies; 51+ messages in thread
From: Masahiro Yamada @ 2023-05-22 1:47 UTC (permalink / raw)
To: linux-kbuild
Cc: linux-kernel, Nathan Chancellor, Nick Desaulniers, Nicolas Schier
On Mon, May 22, 2023 at 1:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
>
> This patch set refactors modpost first to make it easier to
> add new code.
>
> My goals:
>
> - Refactors EXPORT_SYMBOL, <linux/export.h> and <asm/export.h>.
> You can still put EXPORT_SYMBOL() in *.S file, very close to the definition,
> but you do not need to care about whether it is a function or a data.
> This removes EXPORT_DATA_SYMBOL().
>
> - Re-implement TRIM_UNUSED_KSYMS in one-pass.
> This makes the building faster.
>
> - Move the static EXPORT_SYMBOL check to modpost.
> This also makes the building faster.
>
> This patch set is applicable to
> git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git kbuild
I pushed this series to
git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
modpost-v6-testing
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 51+ messages in thread