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
