Re: [PATCH v6 12/20] modpost: check static EXPORT_SYMBOL* by modpost again

[Nick Desaulniers]

On Sun, May 21, 2023 at 9:05 AM Masahiro Yamada <masahiroy@xxxxxxxxxx> 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@xxxxxxxxxx>

Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>

> ---
>
> 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@xxxxxxxxxx>
> -# Copyright (C) 2022 Owen Rafferty <owen@xxxxxxxxxxxxxxxx>
> -#
> -# 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