[PATCH 0/2] modpost: skip section mismatch warnings on ELF local symbols by default

[PATCH 0/2] modpost: skip section mismatch warnings on ELF local symbols by default

[Paul Walmsley]

modpost: skip section mismatch warnings on ELF local symbols by default

modpost, by default, reports section mismatch warnings on ELF local
symbols.  This caused false positive warnings to be reported for a
local symbol name that would otherwise be elided by matching against a
name pattern.  This was observed using a RISC-V toolchain that generates
section anchors.

To avoid this noise in the common case, this patch series disables
section mismatch warnings on ELF local symbols by default.  It also
adds a modpost command line switch to re-enable these warnings, since
I wasn't able to convince myself that section mismatch warnings on ELF
local symbols were completely useless; just mostly useless :-)

I'm neither a modpost nor an ELF expert, so careful review of this
series is appreciated.

This modpost series can also be found at:

https://github.com/sifive/riscv-linux/tree/dev/paulw/modpost-elf-local-symbols-v4.19-rc7

The series was tested on a RISC-V build of the SiFive UART serial
driver, at:

https://github.com/sifive/riscv-linux/tree/dev/paulw/serial-v4.19-rc7

Paul Walmsley (2):
  modpost: add switch to skip symbol exclusions likely to generate false
    positives
  modpost: skip ELF local symbols by default during section mismatch
    check

 scripts/mod/modpost.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

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

-- 
2.19.1

[PATCH 1/2] modpost: add switch to skip symbol exclusions likely to generate false positives

[Paul Walmsley]

modpost uses symbol name whitelist patterns to determine whether
symbols should be excluded from section mismatch tests.  Since ELF
local symbols, empty symbol names, and ARM toolchain "magic" symbols
have autogenerated symbol names, they can trigger false positive
warnings for section mismatches, and are thus excluded from the
modpost section mismatch scan.  However, it seems useful to have the
option to include these autogenerated symbols in some section mismatch
scans, as an indicator that a patch set could require more attention
during review.  To that end, this patch adds a '-P' flag to modpost
that disables the filters that exclude autogenerated and empty symbol
names.

Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: Jim Wilson <jimw@sifive.com>
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 | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 0d998c54564d..38fc1bd47926 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -39,6 +39,8 @@ static int sec_mismatch_verbose = 1;
 static int sec_mismatch_fatal = 0;
 /* ignore missing files */
 static int ignore_missing_files;
+/* accept checks which are more likely to generate false positives if set to 1 */
+static int accept_falsepos_risk;
 
 enum export {
 	export_plain,      export_unused,     export_gpl,
@@ -1292,7 +1294,7 @@ static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
 		symsec = sec_name(elf, get_secindex(elf, sym));
 		if (strcmp(symsec, sec) != 0)
 			continue;
-		if (!is_valid_name(elf, sym))
+		if (!accept_falsepos_risk && !is_valid_name(elf, sym))
 			continue;
 		if (sym->st_value <= addr) {
 			if ((addr - sym->st_value) < distance) {
@@ -2416,7 +2418,7 @@ int main(int argc, char **argv)
 	struct ext_sym_list *extsym_iter;
 	struct ext_sym_list *extsym_start = NULL;
 
-	while ((opt = getopt(argc, argv, "i:I:e:mnsST:o:awM:K:E")) != -1) {
+	while ((opt = getopt(argc, argv, "i:I:e:mnPsST:o:awM:K:E")) != -1) {
 		switch (opt) {
 		case 'i':
 			kernel_read = optarg;
@@ -2442,6 +2444,9 @@ int main(int argc, char **argv)
 		case 'o':
 			dump_write = optarg;
 			break;
+		case 'P':
+			accept_falsepos_risk = 1;
+			break;
 		case 'a':
 			all_versions = 1;
 			break;
-- 
2.19.1

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

[Paul Walmsley]

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(+)

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

Re: [PATCH 0/2] modpost: skip section mismatch warnings on ELF local symbols by default

[Sam Ravnborg]

Hi Paul.

> modpost: skip section mismatch warnings on ELF local symbols by default
> 
> modpost, by default, reports section mismatch warnings on ELF local
> symbols.  This caused false positive warnings to be reported for a
> local symbol name that would otherwise be elided by matching against a
> name pattern.  This was observed using a RISC-V toolchain that generates
> section anchors.
> 
> To avoid this noise in the common case, this patch series disables
> section mismatch warnings on ELF local symbols by default.
This part is fine.

> It also
> adds a modpost command line switch to re-enable these warnings, since
> I wasn't able to convince myself that section mismatch warnings on ELF
> local symbols were completely useless; just mostly useless :-)
modpost is not supposed to be used outside the kernel build.
And therefore if we introduce a new option then the infrastructure
to enable that option should also be in place.
In this particular case I cannot see why we should add the possibility
to include local symbols, in other words do not add the option.
Wait a few days before you kill it, maybe others see the usefulness of it.

I checked if there were any options supported by modpost that
was not configurable in makefile.modpost.
And I could see that the -M and -K options in getopt() was leftovers.
The code that used these option was was dropped in:
a8773769d1a1e08d0ca15f890515401ab3860637 ("Kbuild: clear marker out of modpost")

Could you add a patch that delete these on top of what you already have.

	Sam

Re: [PATCH 2/2] modpost: skip ELF local symbols by default during section mismatch check

[Masahiro Yamada]

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
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] modpost: skip ELF local symbols by default during section mismatch check

[Paul Walmsley]

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

Re: [PATCH 0/2] modpost: skip section mismatch warnings on ELF local symbols by default

[Paul Walmsley]

Hi Sam,

On Sat, 20 Oct 2018, Sam Ravnborg wrote:

> modpost is not supposed to be used outside the kernel build.
> And therefore if we introduce a new option then the infrastructure
> to enable that option should also be in place.
> In this particular case I cannot see why we should add the possibility
> to include local symbols, in other words do not add the option.
> Wait a few days before you kill it, maybe others see the usefulness of it.

I've dropped the optionality from the patch set.

> I checked if there were any options supported by modpost that
> was not configurable in makefile.modpost.
> And I could see that the -M and -K options in getopt() was leftovers.
> The code that used these option was was dropped in:
> a8773769d1a1e08d0ca15f890515401ab3860637 ("Kbuild: clear marker out of modpost")
>
> Could you add a patch that delete these on top of what you already have.

Done.  This patch is now in the v2 set which should appear shortly.

- Paul