[2/2] modpost: skip ELF local symbols by default during section mismatch check
diff mbox series

Message ID 20181020131911.22443-3-paul.walmsley@sifive.com
State Superseded
Headers show
Series
  • modpost: skip section mismatch warnings on ELF local symbols by default
Related show

Commit Message

Paul Walmsley Oct. 20, 2018, 1:19 p.m. UTC
During development of a serial console driver with a RISC-V toolchain,
the following modpost warning appeared:

----
WARNING: vmlinux.o(.data+0x19b10): Section mismatch in reference from the variable .LANCHOR1 to the function .init.text:sifive_serial_console_setup()
The variable .LANCHOR1 references
the function __init sifive_serial_console_setup()
If the reference is valid then annotate the
variable with __init* or __refdata (see linux/init.h) or name the variable:
*_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console
----

".LANCHOR1" is an ELF local symbol, automatically created by gcc's section
anchor generation code:

https://gcc.gnu.org/onlinedocs/gccint/Anchored-Addresses.html

https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/varasm.c;h=cd9591a45617464946dcf9a126dde277d9de9804;hb=9fb89fa845c1b2e0a18d85ada0b077c84508ab78#l7473

This was verified by compiling the kernel with -fno-section-anchors.
The serial driver code idiom triggering the warning is standard serial
driver practice, and one that has a specific whitelist inclusion in
modpost.c.

I'm neither a modpost nor an ELF expert, but naively, it doesn't seem
useful for modpost to report section mismatch warnings caused by ELF
local symbols by default.  Local symbols have compiler-generated
names, and thus bypass modpost's whitelisting algorithm, which relies
on the presence of a non-autogenerated symbol name.  This increases
the likelihood that false positive warnings will be generated (as in
the above case).

Thus, disable section mismatch reporting on ELF local symbols by
default.  The rationale here is similar to that of commit
2e3a10a1551d6ceea005e6a62ca58183b8976217 ("ARM: avoid ARM binutils
leaking ELF local symbols") and of similar code already present in
modpost.c:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/mod/modpost.c?h=v4.19-rc4&id=7876320f88802b22d4e2daf7eb027dd14175a0f8#n1256

Since it might be useful for some static analysis runs to detect
whether any new symbols have been added that result in any section
mismatches, even from ELF local symbols, this change can be disabled
(along with other tests that can often generate false positives) by
passing the '-P' switch on the modpost command line.

Cc: Russell King <linux@armlinux.org.uk>
Cc: Jim Wilson <jimw@sifive.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: linux-kbuild@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
 scripts/mod/modpost.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Masahiro Yamada Nov. 13, 2018, 2:19 a.m. UTC | #1
Hi Paul,


On Sat, Oct 20, 2018 at 10:21 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>
> During development of a serial console driver with a RISC-V toolchain,
> the following modpost warning appeared:
>
> ----
> WARNING: vmlinux.o(.data+0x19b10): Section mismatch in reference from the variable .LANCHOR1 to the function .init.text:sifive_serial_console_setup()
> The variable .LANCHOR1 references
> the function __init sifive_serial_console_setup()
> If the reference is valid then annotate the
> variable with __init* or __refdata (see linux/init.h) or name the variable:
> *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console
> ----

Could you provide me a little more information to reproduce it?

I tried your dev/paulw/serial-v4.19-rc7,
but I could not get that warning.

I used risc64-linux-gcc (GCC 7.3, 8.1) from kernel.org





> ".LANCHOR1" is an ELF local symbol, automatically created by gcc's section
> anchor generation code:
>
> https://gcc.gnu.org/onlinedocs/gccint/Anchored-Addresses.html
>
> https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/varasm.c;h=cd9591a45617464946dcf9a126dde277d9de9804;hb=9fb89fa845c1b2e0a18d85ada0b077c84508ab78#l7473
>
> This was verified by compiling the kernel with -fno-section-anchors.
> The serial driver code idiom triggering the warning is standard serial
> driver practice, and one that has a specific whitelist inclusion in
> modpost.c.
>
> I'm neither a modpost nor an ELF expert, but naively, it doesn't seem
> useful for modpost to report section mismatch warnings caused by ELF
> local symbols by default.  Local symbols have compiler-generated
> names, and thus bypass modpost's whitelisting algorithm, which relies
> on the presence of a non-autogenerated symbol name.  This increases
> the likelihood that false positive warnings will be generated (as in
> the above case).
>
> Thus, disable section mismatch reporting on ELF local symbols by
> default.  The rationale here is similar to that of commit
> 2e3a10a1551d6ceea005e6a62ca58183b8976217 ("ARM: avoid ARM binutils
> leaking ELF local symbols") and of similar code already present in
> modpost.c:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/mod/modpost.c?h=v4.19-rc4&id=7876320f88802b22d4e2daf7eb027dd14175a0f8#n1256
>
> Since it might be useful for some static analysis runs to detect
> whether any new symbols have been added that result in any section
> mismatches, even from ELF local symbols, this change can be disabled
> (along with other tests that can often generate false positives) by
> passing the '-P' switch on the modpost command line.
>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Jim Wilson <jimw@sifive.com>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Michal Marek <michal.lkml@markovi.net>
> Cc: linux-kbuild@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> ---
>  scripts/mod/modpost.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 38fc1bd47926..2b8c960ece66 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1255,6 +1255,17 @@ static inline int is_arm_mapping_symbol(const char *str)
>                && (str[2] == '\0' || str[2] == '.');
>  }
>
> +/*
> + * If a symbol name follows the convention for ELF-local symbols (i.e., the
> + * name begins with a ".L"), return true; otherwise false.  This is used to
> + * skip section mismatch reporting on ELF-local symbols, due to the risk of
> + * false positives.
> + */
> +static inline int is_local_symbol(const char *str)
> +{
> +       return str[0] == '.' && str[1] == 'L';
> +}
> +
>  /*
>   * If there's no name there, ignore it; likewise, ignore it if it's
>   * one of the magic symbols emitted used by current ARM tools.
> @@ -1537,6 +1548,9 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
>         if (strstarts(fromsym, "reference___initcall"))
>                 return;
>
> +       if (!accept_falsepos_risk && is_local_symbol(fromsym))
> +               return;
> +
>         tosec = sec_name(elf, get_secindex(elf, sym));
>         to = find_elf_symbol(elf, r->r_addend, sym);
>         tosym = sym_name(elf, to);
> --
> 2.19.1
>
Paul Walmsley Nov. 14, 2018, 7:30 p.m. UTC | #2
Hello Masahiro,

On Tue, 13 Nov 2018, Masahiro Yamada wrote:

> Hi Paul,
>
> On Sat, Oct 20, 2018 at 10:21 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>>
>> During development of a serial console driver with a RISC-V toolchain,
>> the following modpost warning appeared:
>>
>> ----
>> WARNING: vmlinux.o(.data+0x19b10): Section mismatch in reference from the variable .LANCHOR1 to the function .init.text:sifive_serial_console_setup()
>> The variable .LANCHOR1 references
>> the function __init sifive_serial_console_setup()
>> If the reference is valid then annotate the
>> variable with __init* or __refdata (see linux/init.h) or name the variable:
>> *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console
>> ----
>
> Could you provide me a little more information to reproduce it?
>
> I tried your dev/paulw/serial-v4.19-rc7,
> but I could not get that warning.
>
> I used risc64-linux-gcc (GCC 7.3, 8.1) from kernel.org

I observed this issue with gcc 8.2:

riscv64-unknown-linux-gnu-gcc (crosstool-NG 1.23.0.534-710c8) 8.2.0

This toolchain was built with mainline crosstool-NG:

https://github.com/crosstool-ng/crosstool-ng

using the "riscv64-unknown-linux-gnu" sample configuration.


- Paul

Patch
diff mbox series

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 38fc1bd47926..2b8c960ece66 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1255,6 +1255,17 @@  static inline int is_arm_mapping_symbol(const char *str)
 	       && (str[2] == '\0' || str[2] == '.');
 }
 
+/*
+ * If a symbol name follows the convention for ELF-local symbols (i.e., the
+ * name begins with a ".L"), return true; otherwise false.  This is used to
+ * skip section mismatch reporting on ELF-local symbols, due to the risk of
+ * false positives.
+ */
+static inline int is_local_symbol(const char *str)
+{
+	return str[0] == '.' && str[1] == 'L';
+}
+
 /*
  * If there's no name there, ignore it; likewise, ignore it if it's
  * one of the magic symbols emitted used by current ARM tools.
@@ -1537,6 +1548,9 @@  static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 	if (strstarts(fromsym, "reference___initcall"))
 		return;
 
+	if (!accept_falsepos_risk && is_local_symbol(fromsym))
+		return;
+
 	tosec = sec_name(elf, get_secindex(elf, sym));
 	to = find_elf_symbol(elf, r->r_addend, sym);
 	tosym = sym_name(elf, to);