[PATCH 0/7] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL()

[PATCH 0/7] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL()

[Masahiro Yamada]

This patch set refactors EXPORT_SYMBOL, <linux/export.h> and <asm/export.h>.

You can still put EXPORT_SYMBOL() in *.S file, very close to the definition,
but you do not need to care about whether it is a function or a data.
Remove EXPORT_DATA_SYMBOL().



Masahiro Yamada (7):
  modpost: fix section mismatch check for exported init/exit sections
  modpost: put get_secindex() call inside sec_name()
  kbuild: generate struct kernel_symbol by modpost
  ia64,export.h: replace EXPORT_DATA_SYMBOL* with EXPORT_SYMBOL*
  checkpatch: warn if <asm/export.h> is included
  modpost: merge sym_update_namespace() into sym_add_exported()
  modpost: use null string instead of NULL pointer for default namespace

 arch/ia64/include/asm/Kbuild    |   1 +
 arch/ia64/include/asm/export.h  |   3 -
 arch/ia64/kernel/head.S         |   2 +-
 arch/ia64/kernel/ivt.S          |   2 +-
 include/asm-generic/export.h    |  83 +------------------
 include/linux/export-internal.h |  44 ++++++++++
 include/linux/export.h          |  97 ++++++++--------------
 kernel/module/internal.h        |  12 +++
 kernel/module/main.c            |   1 -
 scripts/Makefile.build          |   8 +-
 scripts/check-local-export      |   4 +-
 scripts/checkpatch.pl           |   7 ++
 scripts/mod/modpost.c           | 139 +++++++++++++++++---------------
 scripts/mod/modpost.h           |   1 +
 14 files changed, 182 insertions(+), 222 deletions(-)
 delete mode 100644 arch/ia64/include/asm/export.h

-- 
2.32.0

[PATCH 1/7] modpost: fix section mismatch check for exported init/exit sections

[Masahiro Yamada]

Since commit f02e8a6596b7 ("module: Sort exported symbols"),
EXPORT_SYMBOL* is placed in the individual section ___ksymtab(_gpl)+<sym>
(3 leading underscores instead of 2).

Since then, modpost cannot detect the bad combination of EXPORT_SYMBOL
and __init/__exit.

Fix the .fromsec field.

Fixes: f02e8a6596b7 ("module: Sort exported symbols")
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---

 scripts/mod/modpost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 29d5a841e215..620dc8c4c814 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -980,7 +980,7 @@ static const struct sectioncheck sectioncheck[] = {
 },
 /* Do not export init/exit functions or data */
 {
-	.fromsec = { "__ksymtab*", NULL },
+	.fromsec = { "___ksymtab*", NULL },
 	.bad_tosec = { INIT_SECTIONS, EXIT_SECTIONS, NULL },
 	.mismatch = EXPORT_TO_INIT_EXIT,
 	.symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, NULL },
-- 
2.32.0

[PATCH 2/7] modpost: put get_secindex() call inside sec_name()

[Masahiro Yamada]

There are 5 callsites of sec_name(). In all the places, sec_name() is
used together with get_secindex().

So, it is simpler to merge two function calls

    sec_name(elf, get_secindex(elf, sym))

into one call:

    sec_name_of_symbol(elf, sym)

While I was here, I also inserted this array range check:

    if (secindex >= info->num_sections)
            return "";

This will make the code robust against info->sechdrs[] overrun.

sym->st_shndx is 2 bytes (for both 32 and 64 bit systems), and the
range 0xff00..0xffff is reserved for special sections.

For example, a symbol specifies an absolute value, sym->st_shndx==0xfff1.
get_secindex() remaps it to 0xfffffff1.

There is no corresponding section header for such special sections.

The existing code does not hit this issue, but it is better to check
the array range.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 620dc8c4c814..b9f2a040f185 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -339,8 +339,19 @@ static const char *sech_name(const struct elf_info *info, Elf_Shdr *sechdr)
 				      sechdr->sh_name);
 }
 
-static const char *sec_name(const struct elf_info *info, int secindex)
+static const char *sec_name_of_symbol(const struct elf_info *info,
+				      const Elf_Sym *sym)
 {
+	unsigned int secindex = get_secindex(info, sym);
+
+	/*
+	 * If sym->st_shndx is within the special section range, get_secindex()
+	 * will remapit to a big number.
+	 * Bail out here, otherwise info->sechdrs[secindex] would overrun.
+	 */
+	if (secindex >= info->num_sections)
+		return "";
+
 	return sech_name(info, &info->sechdrs[secindex]);
 }
 
@@ -649,7 +660,7 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
 			const char *name, *secname;
 
 			name = symname + strlen("__ksymtab_");
-			secname = sec_name(info, get_secindex(info, sym));
+			secname = sec_name_of_symbol(info, sym);
 
 			if (strstarts(secname, "___ksymtab_gpl+"))
 				sym_add_exported(name, mod, true);
@@ -1217,7 +1228,7 @@ static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
 
 		if (is_shndx_special(sym->st_shndx))
 			continue;
-		symsec = sec_name(elf, get_secindex(elf, sym));
+		symsec = sec_name_of_symbol(elf, sym);
 		if (strcmp(symsec, sec) != 0)
 			continue;
 		if (!is_valid_name(elf, sym))
@@ -1457,7 +1468,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 	if (strstarts(fromsym, "reference___initcall"))
 		return;
 
-	tosec = sec_name(elf, get_secindex(elf, sym));
+	tosec = sec_name_of_symbol(elf, sym);
 	to = find_elf_symbol(elf, r->r_addend, sym);
 	tosym = sym_name(elf, to);
 
@@ -1559,7 +1570,7 @@ static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
 				     Elf_Rela* r, Elf_Sym* sym,
 				     const char *fromsec)
 {
-	const char* tosec = sec_name(elf, get_secindex(elf, sym));
+	const char *tosec = sec_name_of_symbol(elf, sym);
 
 	sec_mismatch_count++;
 
@@ -1593,7 +1604,7 @@ static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
 static void check_section_mismatch(const char *modname, struct elf_info *elf,
 				   Elf_Rela *r, Elf_Sym *sym, const char *fromsec)
 {
-	const char *tosec = sec_name(elf, get_secindex(elf, sym));
+	const char *tosec = sec_name_of_symbol(elf, sym);
 	const struct sectioncheck *mismatch = section_mismatch(fromsec, tosec);
 
 	if (mismatch) {
-- 
2.32.0

Re: [PATCH 2/7] modpost: put get_secindex() call inside sec_name()

[Nick Desaulniers]

On Fri, Jun 10, 2022 at 11:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> There are 5 callsites of sec_name(). In all the places, sec_name() is
> used together with get_secindex().
>
> So, it is simpler to merge two function calls
>
>     sec_name(elf, get_secindex(elf, sym))
>
> into one call:
>
>     sec_name_of_symbol(elf, sym)
>
> While I was here, I also inserted this array range check:
>
>     if (secindex >= info->num_sections)
>             return "";
>
> This will make the code robust against info->sechdrs[] overrun.
>
> sym->st_shndx is 2 bytes (for both 32 and 64 bit systems), and the
> range 0xff00..0xffff is reserved for special sections.
>
> For example, a symbol specifies an absolute value, sym->st_shndx==0xfff1.
> get_secindex() remaps it to 0xfffffff1.
>
> There is no corresponding section header for such special sections.
>
> The existing code does not hit this issue, but it is better to check
> the array range.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>
>  scripts/mod/modpost.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 620dc8c4c814..b9f2a040f185 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -339,8 +339,19 @@ static const char *sech_name(const struct elf_info *info, Elf_Shdr *sechdr)
>                                       sechdr->sh_name);
>  }
>
> -static const char *sec_name(const struct elf_info *info, int secindex)
> +static const char *sec_name_of_symbol(const struct elf_info *info,
> +                                     const Elf_Sym *sym)
>  {
> +       unsigned int secindex = get_secindex(info, sym);
> +
> +       /*
> +        * If sym->st_shndx is within the special section range, get_secindex()
> +        * will remapit to a big number.
> +        * Bail out here, otherwise info->sechdrs[secindex] would overrun.
> +        */
> +       if (secindex >= info->num_sections)
> +               return "";
> +
>         return sech_name(info, &info->sechdrs[secindex]);
>  }
>
> @@ -649,7 +660,7 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
>                         const char *name, *secname;
>
>                         name = symname + strlen("__ksymtab_");
> -                       secname = sec_name(info, get_secindex(info, sym));
> +                       secname = sec_name_of_symbol(info, sym);
>
>                         if (strstarts(secname, "___ksymtab_gpl+"))
>                                 sym_add_exported(name, mod, true);
> @@ -1217,7 +1228,7 @@ static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
>
>                 if (is_shndx_special(sym->st_shndx))
>                         continue;
> -               symsec = sec_name(elf, get_secindex(elf, sym));
> +               symsec = sec_name_of_symbol(elf, sym);
>                 if (strcmp(symsec, sec) != 0)
>                         continue;
>                 if (!is_valid_name(elf, sym))
> @@ -1457,7 +1468,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
>         if (strstarts(fromsym, "reference___initcall"))
>                 return;
>
> -       tosec = sec_name(elf, get_secindex(elf, sym));
> +       tosec = sec_name_of_symbol(elf, sym);
>         to = find_elf_symbol(elf, r->r_addend, sym);
>         tosym = sym_name(elf, to);
>
> @@ -1559,7 +1570,7 @@ static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
>                                      Elf_Rela* r, Elf_Sym* sym,
>                                      const char *fromsec)
>  {
> -       const char* tosec = sec_name(elf, get_secindex(elf, sym));
> +       const char *tosec = sec_name_of_symbol(elf, sym);
>
>         sec_mismatch_count++;
>
> @@ -1593,7 +1604,7 @@ static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
>  static void check_section_mismatch(const char *modname, struct elf_info *elf,
>                                    Elf_Rela *r, Elf_Sym *sym, const char *fromsec)
>  {
> -       const char *tosec = sec_name(elf, get_secindex(elf, sym));
> +       const char *tosec = sec_name_of_symbol(elf, sym);
>         const struct sectioncheck *mismatch = section_mismatch(fromsec, tosec);
>
>         if (mismatch) {
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

[PATCH 3/7] kbuild: generate struct kernel_symbol by modpost

[Masahiro Yamada]

Commit 7b4537199a4a ("kbuild: link symbol CRCs at final link, removing
CONFIG_MODULE_REL_CRCS") made the module versioning implementation
arch-agnostic; now all the version CRCs are generated by modpost as C
code whether the EXPORT_SYMBOL() is placed in *.c or *.S.

Doing similar for the entire data structure of EXPORT_SYMBOL() makes
further cleanups possible.

This commit splits EXPORT_SYMBOL() compilation into two stages.

When a source file is compiled, EXPORT_SYMBOL() is converted into a
dummy symbol in the .discard.export_symbol section.

For example,

    EXPORT_SYMBOL(foo);
    EXPORT_SYMBOL_NS_GPL(bar, BAR_NAMESPACE);

will be expanded into the following assembly code:

    .section .discard.export_symbol
    __export_symbol.foo:
	.asciz ""
    .previous

    .section .discard.export_symbol
    __export_symbol_gpl.bar:
	.asciz "BAR_NAMESPACE"
    .previous

They are just markers to tell modpost the name, license, and namespace
of the symbols. They will be dropped from the final vmlinux and modules
because the section name starts with ".discard.".

Then, modpost extracts all the information of EXPORT_SYMBOL() from the
.discard.export_symbol section, then generates C code:

    KSYMTAB_ENTRY(foo, "", "");
    KSYMTAB_ENTRY(bar, "_gpl", "BAR_NAMESPACE");

KSYMTAB_ENTRY() is expanded to struct kernel_symbol that will be linked
to the vmlinux or a module.

With this change, EXPORT_SYMBOL() works in the same way for *.c and *.S
files, providing the following benefits.

[1] Deprecate EXPORT_DATA_SYMBOL()

In the old days, EXPORT_SYMBOL() was only available in C files.
To export assembly code, EXPORT_SYMBOL() was placed in a separate file.
arch/arm/kernel/armksyms.c is one example written in the old fashion.

Commit 22823ab419d8 ("EXPORT_SYMBOL() for asm") removed this limitation.
Since then, EXPORT_SYMBOL() can be placed close to the symbol definition
in *.S files. It was a nice improvement.

However, as that commit mentioned, you need to use EXPORT_DATA_SYMBOL()
for data objects on some architectures.

For example, commit e007c53397ac ("ia64: move exports to definitions")
moved EXPORT_SYMBOL(ia64_ivt) to a .S file, but it resulted in
EXPORT_DATA_SYMBOL().

In the new approach, struct kernel_symbol is populated in C.
You can always use EXPORT_SYMBOL().

EXPORT_DATA_SYMBOL() is now deprecated.

[2] merge <linux/export.h> and <asm-generic/export.h>

There are two similar header implementations:

  include/linux/export.h        for .c files
  include/asm-generic/export.h  for .S files

Ideally, the functionality should be consistent between them, but they
tend to diverge.

Commit 8651ec01daed ("module: add support for symbol namespaces.") did
not support the namespace for *.S files.

This commit shifts the essential part to C. EXPORT_SYMBOL_NS() is now
supported for .S files.

<asm/export.h> and <asm-generic/export.h> will remain as a wrapper of
<linux/export.h> for a while.

They will be removed after #include <asm/export.h> directives are all
replaced.

[3] Implement CONFIG_TRIM_UNUSED_KSYMS in one-pass (planned)

CONFIG_TRIM_UNUSED_KSYMS works by recursively traversing the directory
tree. If an EXPORT_SYMBOL turns out to be unused by anyone, the source
file is recompiled, making the EXPORT_SYMBOL() a no-op.

Linus did not like this slowness.

See:

 - 5cf0fd591f2e ("Kbuild: disable TRIM_UNUSED_KSYMS option")
 - a555bdd0c58c ("Kbuild: enable TRIM_UNUSED_KSYMS again, with some guarding")

We can do this better now; modpost can selectively emit KSYMTAB_ENTRY's
that are really referenced by modules.

Nicolas explained why the trimming was implemented with recursion:

  https://lore.kernel.org/all/2o2rpn97-79nq-p7s2-nq5-8p83391473r@syhkavp.arg/

Actually, we never achieved that level of optimization where the chain
reaction of trimming comes into play because:

 - CONFIG_LTO_CLANG cannot remove any unused symbols
 - CONFIG_LD_DEAD_CODE_DATA_ELIMINATION is enabled only for vmlinux,
   but not modules

If deeper trimming is required, we need to revisit this, but making the
directory traverse one-pass is presumably the right way to go.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 arch/ia64/include/asm/Kbuild    |  1 +
 arch/ia64/include/asm/export.h  |  3 -
 include/asm-generic/export.h    | 84 ++--------------------------
 include/linux/export-internal.h | 44 +++++++++++++++
 include/linux/export.h          | 97 +++++++++++----------------------
 kernel/module/internal.h        | 12 ++++
 kernel/module/main.c            |  1 -
 scripts/Makefile.build          |  8 +--
 scripts/check-local-export      |  4 +-
 scripts/mod/modpost.c           | 85 +++++++++++++++++------------
 scripts/mod/modpost.h           |  1 +
 11 files changed, 151 insertions(+), 189 deletions(-)
 delete mode 100644 arch/ia64/include/asm/export.h

diff --git a/arch/ia64/include/asm/Kbuild b/arch/ia64/include/asm/Kbuild
index f994c1daf9d4..fc998339c405 100644
--- a/arch/ia64/include/asm/Kbuild
+++ b/arch/ia64/include/asm/Kbuild
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 generated-y += syscall_table.h
+generic-y += export.h
 generic-y += kvm_para.h
 generic-y += mcs_spinlock.h
 generic-y += vtime.h
diff --git a/arch/ia64/include/asm/export.h b/arch/ia64/include/asm/export.h
deleted file mode 100644
index ad18c6583252..000000000000
--- a/arch/ia64/include/asm/export.h
+++ /dev/null
@@ -1,3 +0,0 @@
-/* EXPORT_DATA_SYMBOL != EXPORT_SYMBOL here */
-#define KSYM_FUNC(name) @fptr(name)
-#include <asm-generic/export.h>
diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
index 5e4b1f2369d2..0ae9f38a904c 100644
--- a/include/asm-generic/export.h
+++ b/include/asm-generic/export.h
@@ -3,86 +3,12 @@
 #define __ASM_GENERIC_EXPORT_H
 
 /*
- * This comment block is used by fixdep. Please do not remove.
- *
- * When CONFIG_MODVERSIONS is changed from n to y, all source files having
- * EXPORT_SYMBOL variants must be re-compiled because genksyms is run as a
- * side effect of the *.o build rule.
+ * <asm/export.h> and <asm-generic/export.h> are deprecated.
+ * Please include <linux/export.h> directly.
  */
+#include <linux/export.h>
 
-#ifndef KSYM_FUNC
-#define KSYM_FUNC(x) x
-#endif
-#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
-#define KSYM_ALIGN 4
-#elif defined(CONFIG_64BIT)
-#define KSYM_ALIGN 8
-#else
-#define KSYM_ALIGN 4
-#endif
-
-.macro __put, val, name
-#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
-	.long	\val - ., \name - ., 0
-#elif defined(CONFIG_64BIT)
-	.quad	\val, \name, 0
-#else
-	.long	\val, \name, 0
-#endif
-.endm
-
-/*
- * note on .section use: we specify progbits since usage of the "M" (SHF_MERGE)
- * section flag requires it. Use '%progbits' instead of '@progbits' since the
- * former apparently works on all arches according to the binutils source.
- */
-
-.macro ___EXPORT_SYMBOL name,val,sec
-#if defined(CONFIG_MODULES) && !defined(__DISABLE_EXPORTS)
-	.section ___ksymtab\sec+\name,"a"
-	.balign KSYM_ALIGN
-__ksymtab_\name:
-	__put \val, __kstrtab_\name
-	.previous
-	.section __ksymtab_strings,"aMS",%progbits,1
-__kstrtab_\name:
-	.asciz "\name"
-	.previous
-#endif
-.endm
-
-#if defined(CONFIG_TRIM_UNUSED_KSYMS)
-
-#include <linux/kconfig.h>
-#include <generated/autoksyms.h>
-
-.macro __ksym_marker sym
-	.section ".discard.ksym","a"
-__ksym_marker_\sym:
-	 .previous
-.endm
-
-#define __EXPORT_SYMBOL(sym, val, sec)				\
-	__ksym_marker sym;					\
-	__cond_export_sym(sym, val, sec, __is_defined(__KSYM_##sym))
-#define __cond_export_sym(sym, val, sec, conf)			\
-	___cond_export_sym(sym, val, sec, conf)
-#define ___cond_export_sym(sym, val, sec, enabled)		\
-	__cond_export_sym_##enabled(sym, val, sec)
-#define __cond_export_sym_1(sym, val, sec) ___EXPORT_SYMBOL sym, val, sec
-#define __cond_export_sym_0(sym, val, sec) /* nothing */
-
-#else
-#define __EXPORT_SYMBOL(sym, val, sec) ___EXPORT_SYMBOL sym, val, sec
-#endif
-
-#define EXPORT_SYMBOL(name)					\
-	__EXPORT_SYMBOL(name, KSYM_FUNC(name),)
-#define EXPORT_SYMBOL_GPL(name) 				\
-	__EXPORT_SYMBOL(name, KSYM_FUNC(name), _gpl)
-#define EXPORT_DATA_SYMBOL(name)				\
-	__EXPORT_SYMBOL(name, name,)
-#define EXPORT_DATA_SYMBOL_GPL(name)				\
-	__EXPORT_SYMBOL(name, name,_gpl)
+#define EXPORT_DATA_SYMBOL(name)	EXPORT_SYMBOL(name)
+#define EXPORT_DATA_SYMBOL_GPL(name)	EXPORT_SYMBOL_GPL(name)
 
 #endif
diff --git a/include/linux/export-internal.h b/include/linux/export-internal.h
index c2b1d4fd5987..b491ee482947 100644
--- a/include/linux/export-internal.h
+++ b/include/linux/export-internal.h
@@ -14,4 +14,48 @@
 #define SYMBOL_CRC(sym, crc, sec)   \
 	u32 __section("___kcrctab" sec "+" #sym) __used __crc_##sym = crc
 
+#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
+/*
+ * Emit the ksymtab entry as a pair of relative references: this reduces the
+ * size by half on 64-bit architectures, and eliminates the need for absolute
+ * relocations that require runtime processing on relocatable kernels.
+ */
+#define __KSYMTAB_ENTRY(sym, sec)					\
+	asm("	.section \"___ksymtab" sec "+" #sym "\", \"a\"	\n"	\
+	    "	.balign	4					\n"	\
+	    "__ksymtab_" #sym ":				\n"	\
+	    "	.long	" #sym "- .				\n"	\
+	    "	.long	__kstrtab_" #sym "- .			\n"	\
+	    "	.long	__kstrtabns_" #sym "- .			\n"	\
+	    "	.previous					\n")
+#else
+#define __KSYMTAB_ENTRY(sym, sec)					\
+	asm("	.section \"___ksymtab" sec "+" #sym "\", \"a\"	\n"	\
+	    "__ksymtab_" #sym ":				\n"	\
+	    "	.long	" #sym "				\n"	\
+	    "	.long	__kstrtab_" #sym "			\n"	\
+	    "	.long	__kstrtabns_" #sym "			\n"	\
+	    "	.previous					\n")
+#endif
+
+/*
+ * For every exported symbol, do the following:
+ *
+ * - Put the name of the symbol and namespace (empty string "" for none) in
+ *   __ksymtab_strings.
+ * - Place a struct kernel_symbol entry in the __ksymtab section.
+ *
+ * Note on .section use: we specify progbits since usage of the "M" (SHF_MERGE)
+ * section flag requires it. Use '%progbits' instead of '@progbits' since the
+ * former apparently works on all arches according to the binutils source.
+ */
+#define KSYMTAB_ENTRY(sym, sec, ns)						\
+	asm("	.section \"__ksymtab_strings\",\"aMS\",%progbits,1	\n"	\
+	    "__kstrtab_" #sym ":					\n"	\
+	    "	.asciz 	\"" #sym "\"					\n"	\
+	    "__kstrtabns_" #sym ":					\n"	\
+	    "	.asciz 	\"" ns "\"					\n"	\
+	    "	.previous						\n");	\
+	__KSYMTAB_ENTRY(sym, sec)
+
 #endif /* __LINUX_EXPORT_INTERNAL_H__ */
diff --git a/include/linux/export.h b/include/linux/export.h
index 3f31ced0d977..0c71577cf8bb 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -2,6 +2,7 @@
 #ifndef _LINUX_EXPORT_H
 #define _LINUX_EXPORT_H
 
+#include <linux/compiler.h>
 #include <linux/stringify.h>
 
 /*
@@ -28,72 +29,28 @@ extern struct module __this_module;
 #else
 #define THIS_MODULE ((struct module *)0)
 #endif
-
-#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
-#include <linux/compiler.h>
-/*
- * Emit the ksymtab entry as a pair of relative references: this reduces
- * the size by half on 64-bit architectures, and eliminates the need for
- * absolute relocations that require runtime processing on relocatable
- * kernels.
- */
-#define __KSYMTAB_ENTRY(sym, sec)					\
-	__ADDRESSABLE(sym)						\
-	asm("	.section \"___ksymtab" sec "+" #sym "\", \"a\"	\n"	\
-	    "	.balign	4					\n"	\
-	    "__ksymtab_" #sym ":				\n"	\
-	    "	.long	" #sym "- .				\n"	\
-	    "	.long	__kstrtab_" #sym "- .			\n"	\
-	    "	.long	__kstrtabns_" #sym "- .			\n"	\
-	    "	.previous					\n")
-
-struct kernel_symbol {
-	int value_offset;
-	int name_offset;
-	int namespace_offset;
-};
-#else
-#define __KSYMTAB_ENTRY(sym, sec)					\
-	static const struct kernel_symbol __ksymtab_##sym		\
-	__attribute__((section("___ksymtab" sec "+" #sym), used))	\
-	__aligned(sizeof(void *))					\
-	= { (unsigned long)&sym, __kstrtab_##sym, __kstrtabns_##sym }
-
-struct kernel_symbol {
-	unsigned long value;
-	const char *name;
-	const char *namespace;
-};
-#endif
+#endif /* __ASSEMBLY__ */
 
 #ifdef __GENKSYMS__
 
 #define ___EXPORT_SYMBOL(sym, sec, ns)	__GENKSYMS_EXPORT_SYMBOL(sym)
 
+#elif defined(__ASSEMBLY__)
+
+#define ___EXPORT_SYMBOL(sym, sec, ns)				\
+	.section .discard.export_symbol ;			\
+	__export_symbol##sec##.sym: ;				\
+	.asciz ns ;						\
+	.previous
+
 #else
 
-/*
- * For every exported symbol, do the following:
- *
- * - Put the name of the symbol and namespace (empty string "" for none) in
- *   __ksymtab_strings.
- * - Place a struct kernel_symbol entry in the __ksymtab section.
- *
- * note on .section use: we specify progbits since usage of the "M" (SHF_MERGE)
- * section flag requires it. Use '%progbits' instead of '@progbits' since the
- * former apparently works on all arches according to the binutils source.
- */
-#define ___EXPORT_SYMBOL(sym, sec, ns)						\
-	extern typeof(sym) sym;							\
-	extern const char __kstrtab_##sym[];					\
-	extern const char __kstrtabns_##sym[];					\
-	asm("	.section \"__ksymtab_strings\",\"aMS\",%progbits,1	\n"	\
-	    "__kstrtab_" #sym ":					\n"	\
-	    "	.asciz 	\"" #sym "\"					\n"	\
-	    "__kstrtabns_" #sym ":					\n"	\
-	    "	.asciz 	\"" ns "\"					\n"	\
-	    "	.previous						\n");	\
-	__KSYMTAB_ENTRY(sym, sec)
+#define ___EXPORT_SYMBOL(sym, sec, ns)				\
+	__ADDRESSABLE(sym)					\
+	asm(".section .discard.export_symbol		\n"	\
+	    "__export_symbol" #sec "." #sym ":		\n"	\
+	    ".asciz " "\"" ns "\"" "			\n"	\
+	    ".previous					\n")
 
 #endif
 
@@ -117,9 +74,21 @@ struct kernel_symbol {
  * from the $(NM) output (see scripts/gen_ksymdeps.sh). These symbols are
  * discarded in the final link stage.
  */
+
+#ifdef __ASSEMBLY__
+
+#define __ksym_marker(sym)					\
+	.section ".discard.ksym","a" ;				\
+__ksym_marker_##sym: ;						\
+	.previous
+
+#else
+
 #define __ksym_marker(sym)	\
 	static int __ksym_marker_##sym[0] __section(".discard.ksym") __used
 
+#endif
+
 #define __EXPORT_SYMBOL(sym, sec, ns)					\
 	__ksym_marker(sym);						\
 	__cond_export_sym(sym, sec, ns, __is_defined(__KSYM_##sym))
@@ -147,11 +116,9 @@ struct kernel_symbol {
 #define _EXPORT_SYMBOL(sym, sec)	__EXPORT_SYMBOL(sym, sec, "")
 #endif
 
-#define EXPORT_SYMBOL(sym)		_EXPORT_SYMBOL(sym, "")
-#define EXPORT_SYMBOL_GPL(sym)		_EXPORT_SYMBOL(sym, "_gpl")
-#define EXPORT_SYMBOL_NS(sym, ns)	__EXPORT_SYMBOL(sym, "", __stringify(ns))
-#define EXPORT_SYMBOL_NS_GPL(sym, ns)	__EXPORT_SYMBOL(sym, "_gpl", __stringify(ns))
-
-#endif /* !__ASSEMBLY__ */
+#define EXPORT_SYMBOL(sym)		_EXPORT_SYMBOL(sym,)
+#define EXPORT_SYMBOL_GPL(sym)		_EXPORT_SYMBOL(sym,_gpl)
+#define EXPORT_SYMBOL_NS(sym, ns)	__EXPORT_SYMBOL(sym,, __stringify(ns))
+#define EXPORT_SYMBOL_NS_GPL(sym, ns)	__EXPORT_SYMBOL(sym,_gpl, __stringify(ns))
 
 #endif /* _LINUX_EXPORT_H */
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index bc5507ab8450..a362905ed24c 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -36,6 +36,18 @@
 # define strict_align(X) (X)
 #endif
 
+struct kernel_symbol {
+#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
+	int value_offset;
+	int name_offset;
+	int namespace_offset;
+#else
+	unsigned long value;
+	const char *name;
+	const char *namespace;
+#endif
+};
+
 extern struct mutex module_mutex;
 extern struct list_head modules;
 
diff --git a/kernel/module/main.c b/kernel/module/main.c
index fed58d30725d..e2ed87de6977 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -6,7 +6,6 @@
 
 #define INCLUDE_VERMAGIC
 
-#include <linux/export.h>
 #include <linux/extable.h>
 #include <linux/moduleloader.h>
 #include <linux/module_signature.h>
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 1f01ac65c0cd..8848e465ba25 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -164,7 +164,7 @@ quiet_cmd_cc_o_c = CC $(quiet_modtag)  $@
 ifdef CONFIG_MODVERSIONS
 # When module versioning is enabled the following steps are executed:
 # o compile a <file>.o from <file>.c
-# o if <file>.o doesn't contain a __ksymtab version, i.e. does
+# o if <file>.o doesn't contain a __export_symbol*, i.e. does
 #   not export symbols, it's done.
 # o otherwise, we calculate symbol versions using the good old
 #   genksyms on the preprocessed source and dump them into the .cmd file.
@@ -172,7 +172,7 @@ ifdef CONFIG_MODVERSIONS
 #   be compiled and linked to the kernel and/or modules.
 
 gen_symversions =								\
-	if $(NM) $@ 2>/dev/null | grep -q __ksymtab; then			\
+	if $(NM) $@ 2>/dev/null | grep -q '__export_symbol.*\.'; then		\
 		$(call cmd_gensymtypes_$(1),$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \
 			>> $(dot-target).cmd;					\
 	fi
@@ -288,9 +288,7 @@ $(obj)/%.lst: $(src)/%.c FORCE
 cmd_gensymtypes_S =                                                         \
    { echo "\#include <linux/kernel.h>" ;                                    \
      echo "\#include <asm/asm-prototypes.h>" ;                              \
-    $(CPP) $(a_flags) $< |                                                  \
-     grep "\<___EXPORT_SYMBOL\>" |                                          \
-     sed 's/.*___EXPORT_SYMBOL[[:space:]]*\([a-zA-Z0-9_]*\)[[:space:]]*,.*/EXPORT_SYMBOL(\1);/' ; } | \
+     $(NM) $@ | sed -n 's/.*__export_symbol.*\.\(.*\)/EXPORT_SYMBOL(\1);/p' ; } | \
     $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | $(genksyms)
 
 quiet_cmd_cc_symtypes_S = SYM $(quiet_modtag) $@
diff --git a/scripts/check-local-export b/scripts/check-local-export
index da745e2743b7..2e83e0498d4f 100755
--- a/scripts/check-local-export
+++ b/scripts/check-local-export
@@ -34,8 +34,8 @@ do
 	symbol_types[${name}]=${type}
 
 	# append the exported symbol to the array
-	if [[ ${name} == __ksymtab_* ]]; then
-		export_symbols+=(${name#__ksymtab_})
+	if [[ ${name} == __export_symbol*.* ]]; then
+		export_symbols+=(${name#*.})
 	fi
 
 	# If there is no symbol in the object, ${NM} (both GNU nm and llvm-nm)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index b9f2a040f185..f738dddde7b8 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -533,6 +533,8 @@ static int parse_elf(struct elf_info *info, const char *filename)
 				fatal("%s has NOBITS .modinfo\n", filename);
 			info->modinfo = (void *)hdr + sechdrs[i].sh_offset;
 			info->modinfo_len = sechdrs[i].sh_size;
+		} else if (!strcmp(secname, ".discard.export_symbol")) {
+			info->export_symbol_sec = i;
 		}
 
 		if (sechdrs[i].sh_type == SHT_SYMTAB) {
@@ -655,17 +657,20 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
 				   ELF_ST_BIND(sym->st_info) == STB_WEAK);
 		break;
 	default:
-		/* All exported symbols */
-		if (strstarts(symname, "__ksymtab_")) {
-			const char *name, *secname;
+		if (sym->st_shndx == info->export_symbol_sec) {
+			const char *name;
 
-			name = symname + strlen("__ksymtab_");
-			secname = sec_name_of_symbol(info, sym);
-
-			if (strstarts(secname, "___ksymtab_gpl+"))
+			if (strstarts(symname, "__export_symbol_gpl.")) {
+				name = symname + strlen("__export_symbol_gpl.");
 				sym_add_exported(name, mod, true);
-			else if (strstarts(secname, "___ksymtab+"))
+				sym_update_namespace(name, sym_get_data(info, sym));
+			} else if (strstarts(symname, "__export_symbol.")) {
+				name = symname + strlen("__export_symbol.");
 				sym_add_exported(name, mod, false);
+				sym_update_namespace(name, sym_get_data(info, sym));
+			}
+
+			break;
 		}
 		if (strcmp(symname, "init_module") == 0)
 			mod->has_init = true;
@@ -877,7 +882,6 @@ enum mismatch {
 	XXXEXIT_TO_SOME_EXIT,
 	ANY_INIT_TO_ANY_EXIT,
 	ANY_EXIT_TO_ANY_INIT,
-	EXPORT_TO_INIT_EXIT,
 	EXTABLE_TO_NON_TEXT,
 };
 
@@ -989,13 +993,6 @@ static const struct sectioncheck sectioncheck[] = {
 	.mismatch = ANY_INIT_TO_ANY_EXIT,
 	.symbol_white_list = { NULL },
 },
-/* Do not export init/exit functions or data */
-{
-	.fromsec = { "___ksymtab*", NULL },
-	.bad_tosec = { INIT_SECTIONS, EXIT_SECTIONS, NULL },
-	.mismatch = EXPORT_TO_INIT_EXIT,
-	.symbol_white_list = { DEFAULT_SYMBOL_WHITE_LIST, NULL },
-},
 {
 	.fromsec = { "__ex_table", NULL },
 	/* If you're adding any new black-listed sections in here, consider
@@ -1435,15 +1432,6 @@ static void report_sec_mismatch(const char *modname,
 		free(prl_from);
 		free(prl_to);
 		break;
-	case EXPORT_TO_INIT_EXIT:
-		prl_to = sec2annotation(tosec);
-		fprintf(stderr,
-		"The symbol %s is exported and annotated %s\n"
-		"Fix this by removing the %sannotation of %s "
-		"or drop the export.\n",
-		tosym, prl_to, prl_to, tosym);
-		free(prl_to);
-		break;
 	case EXTABLE_TO_NON_TEXT:
 		fatal("There's a special handler for this mismatch type, "
 		      "we should never get here.");
@@ -1830,6 +1818,36 @@ static void section_rel(const char *modname, struct elf_info *elf,
 	}
 }
 
+static void check_sec_for_export_symbols(struct module *mod, struct elf_info *elf)
+{
+	static const char *const init_or_exit_sections[] = {
+		INIT_SECTIONS,
+		EXIT_SECTIONS,
+		NULL
+	};
+	Elf_Sym *sym;
+
+	for (sym = elf->symtab_start; sym < elf->symtab_stop; sym++) {
+		const char *symname = sym_name(elf, sym);
+		const char *secname;
+		char *annotate;
+
+		if (!sym_find_with_module(symname, mod))
+			continue;	/* This is not an exported symbol */
+
+		secname = sec_name_of_symbol(elf, sym);
+
+		if (!match(secname, init_or_exit_sections))
+			continue;	/* OK, this is not __init or __exit */
+
+		annotate = sec2annotation(secname);
+
+		error("%s: exported and annotated %s. Remove %s or EXPORT_SYMBOL.\n",
+		      symname, annotate, annotate);
+		free(annotate);
+	}
+}
+
 /**
  * A module includes a number of sections that are discarded
  * either when loaded or when used as built-in.
@@ -2016,16 +2034,8 @@ static void read_symbols(const char *modname)
 		handle_moddevtable(mod, &info, sym, symname);
 	}
 
-	for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
-		symname = remove_dot(info.strtab + sym->st_name);
-
-		/* Apply symbol namespaces from __kstrtabns_<symbol> entries. */
-		if (strstarts(symname, "__kstrtabns_"))
-			sym_update_namespace(symname + strlen("__kstrtabns_"),
-					     sym_get_data(&info, sym));
-	}
-
 	check_sec_ref(modname, &info);
+	check_sec_for_export_symbols(mod, &info);
 
 	if (!mod->is_vmlinux) {
 		version = get_modinfo(&info, "version");
@@ -2208,6 +2218,13 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod)
 {
 	struct symbol *sym;
 
+	/* generate struct for exported symbols */
+	buf_printf(buf, "\n");
+	list_for_each_entry(sym, &mod->exported_symbols, list)
+		buf_printf(buf, "KSYMTAB_ENTRY(%s, \"%s\", \"%s\");\n",
+			   sym->name, sym->is_gpl_only ? "_gpl" : "",
+			   sym->namespace ?: "");
+
 	if (!modversions)
 		return;
 
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 044bdfb894b7..ae59417a9df9 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -139,6 +139,7 @@ struct elf_info {
 	Elf_Shdr     *sechdrs;
 	Elf_Sym      *symtab_start;
 	Elf_Sym      *symtab_stop;
+	unsigned int export_symbol_sec;	/* .discard.export_symbol section */
 	char         *strtab;
 	char	     *modinfo;
 	unsigned int modinfo_len;
-- 
2.32.0

Re: [PATCH 3/7] kbuild: generate struct kernel_symbol by modpost

[Masahiro Yamada]

On Sat, Jun 11, 2022 at 3:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Commit 7b4537199a4a ("kbuild: link symbol CRCs at final link, removing
> CONFIG_MODULE_REL_CRCS") made the module versioning implementation
> arch-agnostic; now all the version CRCs are generated by modpost as C
> code whether the EXPORT_SYMBOL() is placed in *.c or *.S.
>
> Doing similar for the entire data structure of EXPORT_SYMBOL() makes
> further cleanups possible.
>
> This commit splits EXPORT_SYMBOL() compilation into two stages.
>
> When a source file is compiled, EXPORT_SYMBOL() is converted into a
> dummy symbol in the .discard.export_symbol section.
>
> For example,
>
>     EXPORT_SYMBOL(foo);
>     EXPORT_SYMBOL_NS_GPL(bar, BAR_NAMESPACE);
>
> will be expanded into the following assembly code:
>
>     .section .discard.export_symbol
>     __export_symbol.foo:
>         .asciz ""
>     .previous
>
>     .section .discard.export_symbol
>     __export_symbol_gpl.bar:
>         .asciz "BAR_NAMESPACE"
>     .previous
>
> They are just markers to tell modpost the name, license, and namespace
> of the symbols. They will be dropped from the final vmlinux and modules
> because the section name starts with ".discard.".
>
> Then, modpost extracts all the information of EXPORT_SYMBOL() from the
> .discard.export_symbol section, then generates C code:
>
>     KSYMTAB_ENTRY(foo, "", "");
>     KSYMTAB_ENTRY(bar, "_gpl", "BAR_NAMESPACE");
>
> KSYMTAB_ENTRY() is expanded to struct kernel_symbol that will be linked
> to the vmlinux or a module.
>
> With this change, EXPORT_SYMBOL() works in the same way for *.c and *.S
> files, providing the following benefits.
>
> [1] Deprecate EXPORT_DATA_SYMBOL()



Sorry, please let me take back this.

I completely missed how linkage for ia64 works.
I used inline assembly in C, so it would break ia64.

Please ignore this patch.








-- 
Best Regards
Masahiro Yamada

[PATCH 4/7] ia64,export.h: replace EXPORT_DATA_SYMBOL* with EXPORT_SYMBOL*

[Masahiro Yamada]

With the previous refactoring, you no longer need to know whether the
exported symbol is a function or a data.

Replace two instances in ia64, then remove EXPORT_DATA_SYMBOL*.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 arch/ia64/kernel/head.S      | 2 +-
 arch/ia64/kernel/ivt.S       | 2 +-
 include/asm-generic/export.h | 3 ---
 3 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/ia64/kernel/head.S b/arch/ia64/kernel/head.S
index f22469f1c1fc..c096500590e9 100644
--- a/arch/ia64/kernel/head.S
+++ b/arch/ia64/kernel/head.S
@@ -170,7 +170,7 @@ RestRR:											\
 	__PAGE_ALIGNED_DATA
 
 	.global empty_zero_page
-EXPORT_DATA_SYMBOL_GPL(empty_zero_page)
+EXPORT_SYMBOL_GPL(empty_zero_page)
 empty_zero_page:
 	.skip PAGE_SIZE
 
diff --git a/arch/ia64/kernel/ivt.S b/arch/ia64/kernel/ivt.S
index d6d4229b28db..7a418e324d30 100644
--- a/arch/ia64/kernel/ivt.S
+++ b/arch/ia64/kernel/ivt.S
@@ -87,7 +87,7 @@
 
 	.align 32768	// align on 32KB boundary
 	.global ia64_ivt
-	EXPORT_DATA_SYMBOL(ia64_ivt)
+	EXPORT_SYMBOL(ia64_ivt)
 ia64_ivt:
 /////////////////////////////////////////////////////////////////////////////////////////
 // 0x0000 Entry 0 (size 64 bundles) VHPT Translation (8,20,47)
diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
index 0ae9f38a904c..570cd4da7210 100644
--- a/include/asm-generic/export.h
+++ b/include/asm-generic/export.h
@@ -8,7 +8,4 @@
  */
 #include <linux/export.h>
 
-#define EXPORT_DATA_SYMBOL(name)	EXPORT_SYMBOL(name)
-#define EXPORT_DATA_SYMBOL_GPL(name)	EXPORT_SYMBOL_GPL(name)
-
 #endif
-- 
2.32.0

Re: [PATCH 4/7] ia64,export.h: replace EXPORT_DATA_SYMBOL* with EXPORT_SYMBOL*

[Masahiro Yamada]

On Sat, Jun 11, 2022 at 3:33 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> With the previous refactoring, you no longer need to know whether the
> exported symbol is a function or a data.
>
> Replace two instances in ia64, then remove EXPORT_DATA_SYMBOL*.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Please ignore this patch.

The previous refactoring is not working correctly.





-- 
Best Regards
Masahiro Yamada

[PATCH 5/7] checkpatch: warn if <asm/export.h> is included

[Masahiro Yamada]

With the previous refactoring,

 - <asm/export.h> is a wrapper of <asm-generic/export.h>
 - <asm-generic/export.h> is a wrapper of <linux/export.h>

My hope is to replace

   #include <asm/export.h>  -->  #include <linux/export.h>

for all *.S files.

For now, adding a warning in the checkpatch.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/checkpatch.pl | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 503e8abbb2c1..f824d690a565 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3753,6 +3753,13 @@ sub process {
 			     "Avoid using '.L' prefixed local symbol names for denoting a range of code via 'SYM_*_START/END' annotations; see Documentation/asm-annotations.rst\n" . $herecurr);
 		}
 
+# warn if <asm/export.h> is included.
+# <asm/export.h> is a wrapper of <linux/export.h>. Please include <linux/export.h> directly.
+		if ($tree && $rawline =~ m{^.\s*\#\s*include\s*\<asm\/export\.h\>}) {
+			WARN("INCLUDE_LINUX_EXPORT",
+			    "Please include <linux/export.h> instead of <asm/export.h>\n" . $herecurr);
+		}
+
 # check we are in a valid source file C or perl if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
 
-- 
2.32.0

Re: [PATCH 5/7] checkpatch: warn if <asm/export.h> is included

[Joe Perches]

On Sat, 2022-06-11 at 03:32 +0900, Masahiro Yamada wrote:
> With the previous refactoring,
> 
>  - <asm/export.h> is a wrapper of <asm-generic/export.h>
>  - <asm-generic/export.h> is a wrapper of <linux/export.h>
> 
> My hope is to replace
> 
>    #include <asm/export.h>  -->  #include <linux/export.h>
> 
> for all *.S files.
> 
> For now, adding a warning in the checkpatch.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  scripts/checkpatch.pl | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3753,6 +3753,13 @@ sub process {
>  			     "Avoid using '.L' prefixed local symbol names for denoting a range of code via 'SYM_*_START/END' annotations; see Documentation/asm-annotations.rst\n" . $herecurr);
>  		}
>  
> +# warn if <asm/export.h> is included.
> +# <asm/export.h> is a wrapper of <linux/export.h>. Please include <linux/export.h> directly.
> +		if ($tree && $rawline =~ m{^.\s*\#\s*include\s*\<asm\/export\.h\>}) {
> +			WARN("INCLUDE_LINUX_EXPORT",
> +			    "Please include <linux/export.h> instead of <asm/export.h>\n" . $herecurr);
> +		}

This warns on patch context lines.
That's not something checkpatch generally does.

Likely this should use /^\+ rather than /^.

And it's nice to have --fix capability

			if (WARN("etc...") &&
			    $fix) {
				$fixed[$fixlinenr] =~ s/\s*#\s*include\s*\<asm\/export\.h\>/#include <linux/export.h>/;
			}

cheers, Joe

Re: [PATCH 5/7] checkpatch: warn if <asm/export.h> is included

[Masahiro Yamada]

On Sat, Jun 11, 2022 at 10:33 AM Joe Perches <joe@perches.com> wrote:
>
> On Sat, 2022-06-11 at 03:32 +0900, Masahiro Yamada wrote:
> > With the previous refactoring,
> >
> >  - <asm/export.h> is a wrapper of <asm-generic/export.h>
> >  - <asm-generic/export.h> is a wrapper of <linux/export.h>
> >
> > My hope is to replace
> >
> >    #include <asm/export.h>  -->  #include <linux/export.h>
> >
> > for all *.S files.
> >
> > For now, adding a warning in the checkpatch.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  scripts/checkpatch.pl | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -3753,6 +3753,13 @@ sub process {
> >                            "Avoid using '.L' prefixed local symbol names for denoting a range of code via 'SYM_*_START/END' annotations; see Documentation/asm-annotations.rst\n" . $herecurr);
> >               }
> >
> > +# warn if <asm/export.h> is included.
> > +# <asm/export.h> is a wrapper of <linux/export.h>. Please include <linux/export.h> directly.
> > +             if ($tree && $rawline =~ m{^.\s*\#\s*include\s*\<asm\/export\.h\>}) {
> > +                     WARN("INCLUDE_LINUX_EXPORT",
> > +                         "Please include <linux/export.h> instead of <asm/export.h>\n" . $herecurr);
> > +             }
>
> This warns on patch context lines.
> That's not something checkpatch generally does.
>
> Likely this should use /^\+ rather than /^.
>
> And it's nice to have --fix capability
>
>                         if (WARN("etc...") &&
>                             $fix) {
>                                 $fixed[$fixlinenr] =~ s/\s*#\s*include\s*\<asm\/export\.h\>/#include <linux/export.h>/;
>                         }
>
> cheers, Joe



I retract this patch series because I realized it was breaking ia64.
Please ignore this patch.

Anyway, your expert feedback about checkpatch.pl
was very appreciated. Thank you.


-- 
Best Regards
Masahiro Yamada

[PATCH 6/7] modpost: merge sym_update_namespace() into sym_add_exported()

[Masahiro Yamada]

Pass a set of the name, license, and namespace to sym_add_exported().

sym_update_namespace() is unneeded.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/mod/modpost.c | 41 +++++++++--------------------------------
 1 file changed, 9 insertions(+), 32 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f738dddde7b8..0db2cbb74a2a 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -357,26 +357,8 @@ static const char *sec_name_of_symbol(const struct elf_info *info,
 
 #define strstarts(str, prefix) (strncmp(str, prefix, strlen(prefix)) == 0)
 
-static void sym_update_namespace(const char *symname, const char *namespace)
-{
-	struct symbol *s = find_symbol(symname);
-
-	/*
-	 * That symbol should have been created earlier and thus this is
-	 * actually an assertion.
-	 */
-	if (!s) {
-		error("Could not update namespace(%s) for symbol %s\n",
-		      namespace, symname);
-		return;
-	}
-
-	free(s->namespace);
-	s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
-}
-
 static struct symbol *sym_add_exported(const char *name, struct module *mod,
-				       bool gpl_only)
+				       bool gpl_only, const char *namespace)
 {
 	struct symbol *s = find_symbol(name);
 
@@ -389,6 +371,7 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
 	s = alloc_symbol(name);
 	s->module = mod;
 	s->is_gpl_only = gpl_only;
+	s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
 	list_add_tail(&s->list, &mod->exported_symbols);
 	hash_add_symbol(s);
 
@@ -658,17 +641,12 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
 		break;
 	default:
 		if (sym->st_shndx == info->export_symbol_sec) {
-			const char *name;
-
-			if (strstarts(symname, "__export_symbol_gpl.")) {
-				name = symname + strlen("__export_symbol_gpl.");
-				sym_add_exported(name, mod, true);
-				sym_update_namespace(name, sym_get_data(info, sym));
-			} else if (strstarts(symname, "__export_symbol.")) {
-				name = symname + strlen("__export_symbol.");
-				sym_add_exported(name, mod, false);
-				sym_update_namespace(name, sym_get_data(info, sym));
-			}
+			if (strstarts(symname, "__export_symbol_gpl."))
+				sym_add_exported(symname + strlen("__export_symbol_gpl."),
+						 mod, true, sym_get_data(info, sym));
+			else if (strstarts(symname, "__export_symbol."))
+				sym_add_exported(symname + strlen("__export_symbol."),
+						 mod, false, sym_get_data(info, sym));
 
 			break;
 		}
@@ -2470,9 +2448,8 @@ static void read_dump(const char *fname)
 			mod = new_module(modname, strlen(modname));
 			mod->from_dump = true;
 		}
-		s = sym_add_exported(symname, mod, gpl_only);
+		s = sym_add_exported(symname, mod, gpl_only, namespace);
 		sym_set_crc(s, crc);
-		sym_update_namespace(symname, namespace);
 	}
 	free(buf);
 	return;
-- 
2.32.0

Re: [PATCH 6/7] modpost: merge sym_update_namespace() into sym_add_exported()

[Nick Desaulniers]

On Fri, Jun 10, 2022 at 11:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Pass a set of the name, license, and namespace to sym_add_exported().
>
> sym_update_namespace() is unneeded.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>
>  scripts/mod/modpost.c | 41 +++++++++--------------------------------
>  1 file changed, 9 insertions(+), 32 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index f738dddde7b8..0db2cbb74a2a 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -357,26 +357,8 @@ static const char *sec_name_of_symbol(const struct elf_info *info,
>
>  #define strstarts(str, prefix) (strncmp(str, prefix, strlen(prefix)) == 0)
>
> -static void sym_update_namespace(const char *symname, const char *namespace)
> -{
> -       struct symbol *s = find_symbol(symname);
> -
> -       /*
> -        * That symbol should have been created earlier and thus this is
> -        * actually an assertion.
> -        */
> -       if (!s) {
> -               error("Could not update namespace(%s) for symbol %s\n",
> -                     namespace, symname);
> -               return;
> -       }
> -
> -       free(s->namespace);
> -       s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
> -}
> -
>  static struct symbol *sym_add_exported(const char *name, struct module *mod,
> -                                      bool gpl_only)
> +                                      bool gpl_only, const char *namespace)
>  {
>         struct symbol *s = find_symbol(name);
>
> @@ -389,6 +371,7 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
>         s = alloc_symbol(name);
>         s->module = mod;
>         s->is_gpl_only = gpl_only;
> +       s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
>         list_add_tail(&s->list, &mod->exported_symbols);
>         hash_add_symbol(s);
>
> @@ -658,17 +641,12 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
>                 break;
>         default:
>                 if (sym->st_shndx == info->export_symbol_sec) {
> -                       const char *name;
> -
> -                       if (strstarts(symname, "__export_symbol_gpl.")) {
> -                               name = symname + strlen("__export_symbol_gpl.");
> -                               sym_add_exported(name, mod, true);
> -                               sym_update_namespace(name, sym_get_data(info, sym));
> -                       } else if (strstarts(symname, "__export_symbol.")) {
> -                               name = symname + strlen("__export_symbol.");
> -                               sym_add_exported(name, mod, false);
> -                               sym_update_namespace(name, sym_get_data(info, sym));
> -                       }
> +                       if (strstarts(symname, "__export_symbol_gpl."))
> +                               sym_add_exported(symname + strlen("__export_symbol_gpl."),
> +                                                mod, true, sym_get_data(info, sym));
> +                       else if (strstarts(symname, "__export_symbol."))
> +                               sym_add_exported(symname + strlen("__export_symbol."),
> +                                                mod, false, sym_get_data(info, sym));
>
>                         break;
>                 }
> @@ -2470,9 +2448,8 @@ static void read_dump(const char *fname)
>                         mod = new_module(modname, strlen(modname));
>                         mod->from_dump = true;
>                 }
> -               s = sym_add_exported(symname, mod, gpl_only);
> +               s = sym_add_exported(symname, mod, gpl_only, namespace);
>                 sym_set_crc(s, crc);
> -               sym_update_namespace(symname, namespace);
>         }
>         free(buf);
>         return;
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

[PATCH 7/7] modpost: use null string instead of NULL pointer for default namespace

[Masahiro Yamada]

The default namespace is the null string, "".

When set, the null string "" is converted to NULL:

  s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;

When printed, the NULL pointer is get back to the null string:

  sym->namespace ?: ""

This saves 1 byte memory allocated for "", but loses the readability.

In kernel-space, we strive to save memory, but modpost is a userspace
tool used to build the kernel. On modern systems, such small piece of
memory is not a big deal.

Handle the namespace string as is.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/mod/modpost.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 0db2cbb74a2a..5a1785645943 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -296,6 +296,13 @@ static bool contains_namespace(struct list_head *head, const char *namespace)
 {
 	struct namespace_list *list;
 
+	/*
+	 * The default namespace is null string "", which is always implicitly
+	 * contained.
+	 */
+	if (!namespace[0])
+		return true;
+
 	list_for_each_entry(list, head, list) {
 		if (!strcmp(list->namespace, namespace))
 			return true;
@@ -371,7 +378,7 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
 	s = alloc_symbol(name);
 	s->module = mod;
 	s->is_gpl_only = gpl_only;
-	s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
+	s->namespace = NOFAIL(strdup(namespace));
 	list_add_tail(&s->list, &mod->exported_symbols);
 	hash_add_symbol(s);
 
@@ -2117,8 +2124,7 @@ static void check_exports(struct module *mod)
 		else
 			basename = mod->name;
 
-		if (exp->namespace &&
-		    !contains_namespace(&mod->imported_namespaces, exp->namespace)) {
+		if (!contains_namespace(&mod->imported_namespaces, exp->namespace)) {
 			modpost_log(allow_missing_ns_imports ? LOG_WARN : LOG_ERROR,
 				    "module %s uses symbol %s from namespace %s, but does not import it.\n",
 				    basename, exp->name, exp->namespace);
@@ -2201,7 +2207,7 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod)
 	list_for_each_entry(sym, &mod->exported_symbols, list)
 		buf_printf(buf, "KSYMTAB_ENTRY(%s, \"%s\", \"%s\");\n",
 			   sym->name, sym->is_gpl_only ? "_gpl" : "",
-			   sym->namespace ?: "");
+			   sym->namespace);
 
 	if (!modversions)
 		return;
@@ -2471,7 +2477,7 @@ static void write_dump(const char *fname)
 			buf_printf(&buf, "0x%08x\t%s\t%s\tEXPORT_SYMBOL%s\t%s\n",
 				   sym->crc, sym->name, mod->name,
 				   sym->is_gpl_only ? "_GPL" : "",
-				   sym->namespace ?: "");
+				   sym->namespace);
 		}
 	}
 	write_buf(&buf, fname);
-- 
2.32.0

Re: [PATCH 7/7] modpost: use null string instead of NULL pointer for default namespace

[Nick Desaulniers]

On Fri, Jun 10, 2022 at 11:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> The default namespace is the null string, "".
>
> When set, the null string "" is converted to NULL:
>
>   s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
>
> When printed, the NULL pointer is get back to the null string:
>
>   sym->namespace ?: ""
>
> This saves 1 byte memory allocated for "", but loses the readability.
>
> In kernel-space, we strive to save memory, but modpost is a userspace
> tool used to build the kernel. On modern systems, such small piece of
> memory is not a big deal.
>
> Handle the namespace string as is.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Sorry for the late review.  If this is still useful:
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Feel free to ping me via mail if I'm falling behind. Otherwise you
should join us on IRC. (#clangbuiltlinux on libera)

> ---
>
>  scripts/mod/modpost.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 0db2cbb74a2a..5a1785645943 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -296,6 +296,13 @@ static bool contains_namespace(struct list_head *head, const char *namespace)
>  {
>         struct namespace_list *list;
>
> +       /*
> +        * The default namespace is null string "", which is always implicitly
> +        * contained.
> +        */
> +       if (!namespace[0])
> +               return true;
> +
>         list_for_each_entry(list, head, list) {
>                 if (!strcmp(list->namespace, namespace))
>                         return true;
> @@ -371,7 +378,7 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
>         s = alloc_symbol(name);
>         s->module = mod;
>         s->is_gpl_only = gpl_only;
> -       s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
> +       s->namespace = NOFAIL(strdup(namespace));
>         list_add_tail(&s->list, &mod->exported_symbols);
>         hash_add_symbol(s);
>
> @@ -2117,8 +2124,7 @@ static void check_exports(struct module *mod)
>                 else
>                         basename = mod->name;
>
> -               if (exp->namespace &&
> -                   !contains_namespace(&mod->imported_namespaces, exp->namespace)) {
> +               if (!contains_namespace(&mod->imported_namespaces, exp->namespace)) {
>                         modpost_log(allow_missing_ns_imports ? LOG_WARN : LOG_ERROR,
>                                     "module %s uses symbol %s from namespace %s, but does not import it.\n",
>                                     basename, exp->name, exp->namespace);
> @@ -2201,7 +2207,7 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod)
>         list_for_each_entry(sym, &mod->exported_symbols, list)
>                 buf_printf(buf, "KSYMTAB_ENTRY(%s, \"%s\", \"%s\");\n",
>                            sym->name, sym->is_gpl_only ? "_gpl" : "",
> -                          sym->namespace ?: "");
> +                          sym->namespace);
>
>         if (!modversions)
>                 return;
> @@ -2471,7 +2477,7 @@ static void write_dump(const char *fname)
>                         buf_printf(&buf, "0x%08x\t%s\t%s\tEXPORT_SYMBOL%s\t%s\n",
>                                    sym->crc, sym->name, mod->name,
>                                    sym->is_gpl_only ? "_GPL" : "",
> -                                  sym->namespace ?: "");
> +                                  sym->namespace);
>                 }
>         }
>         write_buf(&buf, fname);
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 0/7] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL()

[Masahiro Yamada]

On Sat, Jun 11, 2022 at 3:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
>
> This patch set refactors EXPORT_SYMBOL, <linux/export.h> and <asm/export.h>.
>
> You can still put EXPORT_SYMBOL() in *.S file, very close to the definition,
> but you do not need to care about whether it is a function or a data.
> Remove EXPORT_DATA_SYMBOL().
>


Sorry, please ignore this patch set.

With further testing for ia64,
I realized it was not working.





>
> Masahiro Yamada (7):
>   modpost: fix section mismatch check for exported init/exit sections
>   modpost: put get_secindex() call inside sec_name()
>   kbuild: generate struct kernel_symbol by modpost
>   ia64,export.h: replace EXPORT_DATA_SYMBOL* with EXPORT_SYMBOL*
>   checkpatch: warn if <asm/export.h> is included
>   modpost: merge sym_update_namespace() into sym_add_exported()
>   modpost: use null string instead of NULL pointer for default namespace
>
>  arch/ia64/include/asm/Kbuild    |   1 +
>  arch/ia64/include/asm/export.h  |   3 -
>  arch/ia64/kernel/head.S         |   2 +-
>  arch/ia64/kernel/ivt.S          |   2 +-
>  include/asm-generic/export.h    |  83 +------------------
>  include/linux/export-internal.h |  44 ++++++++++
>  include/linux/export.h          |  97 ++++++++--------------
>  kernel/module/internal.h        |  12 +++
>  kernel/module/main.c            |   1 -
>  scripts/Makefile.build          |   8 +-
>  scripts/check-local-export      |   4 +-
>  scripts/checkpatch.pl           |   7 ++
>  scripts/mod/modpost.c           | 139 +++++++++++++++++---------------
>  scripts/mod/modpost.h           |   1 +
>  14 files changed, 182 insertions(+), 222 deletions(-)
>  delete mode 100644 arch/ia64/include/asm/export.h
>
> --
> 2.32.0
>


-- 
Best Regards
Masahiro Yamada