| Message ID | 20230521160426.1881124-3-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS | expand |
+ 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 >
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
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
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.
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:
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(-)