[PATCH v5 00/21] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS

[PATCH v5 00/21] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS

[Masahiro Yamada]

This patch set refactors modpost first to make it easier to
add new code.

My goals:

 - 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.
   This removes EXPORT_DATA_SYMBOL().

 - Re-implement TRIM_UNUSED_KSYMS in one-pass.
   This makes the building faster.

 - Move the static EXPORT_SYMBOL check to modpost.
   This also makes the building faster.

Previous version
v4: https://lore.kernel.org/linux-kbuild/CAK7LNASDzy9RERN6+q6WgR4ROYZQue=SBqgbcoYuVePByHtk6Q@mail.gmail.com/T/#t
v3: https://lore.kernel.org/all/20220928063947.299333-1-masahiroy@kernel.org/



Masahiro Yamada (21):
  modpost: remove broken calculation of exception_table_entry size
  modpost: remove fromsym info in __ex_table section mismatch warning
  modpost: remove get_prettyname()
  modpost: squash report_extable_warnings() into
    extable_mismatch_handler()
  modpost: squash report_sec_mismatch() into default_mismatch_handler()
  modpost: clean up is_executable_section()
  modpost: squash extable_mismatch_handler() into
    default_mismatch_handler()
  modpost: pass 'tosec' down to default_mismatch_handler()
  modpost: pass section index to find_elf_symbol2()
  modpost: rename find_elf_symbol() and find_elf_symbol2()
  modpost: modpost: refactor find_fromsym() and find_tosym()
  modpost: unify 'sym' and 'to' in default_mismatch_handler()
  modpost: replace r->r_offset, r->r_addend with faddr, taddr
  modpost: remove is_shndx_special() check from section_rel(a)
  modpost: pass struct module pointer to check_section_mismatch()
  kbuild: generate KSYMTAB entries by modpost
  ia64,export.h: replace EXPORT_DATA_SYMBOL* with EXPORT_SYMBOL*
  modpost: check static EXPORT_SYMBOL* by modpost again
  modpost: squash sym_update_namespace() into sym_add_exported()
  modpost: use null string instead of NULL pointer for default namespace
  kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion

 .gitignore                        |   1 -
 Makefile                          |  19 +-
 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/asm-generic/vmlinux.lds.h |   1 +
 include/linux/export-internal.h   |  49 +++
 include/linux/export.h            | 119 ++-----
 include/linux/pm.h                |   8 +-
 kernel/module/internal.h          |  12 +
 scripts/Makefile.build            |  19 +-
 scripts/Makefile.modpost          |   7 +
 scripts/adjust_autoksyms.sh       |  73 ----
 scripts/basic/fixdep.c            |   3 +-
 scripts/check-local-export        |  70 ----
 scripts/gen_ksymdeps.sh           |  30 --
 scripts/mod/modpost.c             | 561 ++++++++++++------------------
 scripts/mod/modpost.h             |   1 +
 scripts/remove-stale-files        |   2 +
 21 files changed, 343 insertions(+), 723 deletions(-)
 delete mode 100644 arch/ia64/include/asm/export.h
 delete mode 100755 scripts/adjust_autoksyms.sh
 delete mode 100755 scripts/check-local-export
 delete mode 100755 scripts/gen_ksymdeps.sh

-- 
2.39.2

[PATCH v5 01/21] modpost: remove broken calculation of exception_table_entry size

[Masahiro Yamada]

find_extable_entry_size() is completely broken. It has awesome comments
about how to calculate sizeof(struct exception_table_entry).

It was based on these assumptions:

  - struct exception_table_entry has two fields
  - both of the fields have the same size

Then, we came up with this equation:

  (offset of the second field) * 2 == (size of struct)

It was true for all architectures when commit 52dc0595d540 ("modpost:
handle relocations mismatch in __ex_table.") was applied.

Our mathematics broke when commit 548acf19234d ("x86/mm: Expand the
exception table logic to allow new handling options") introduced the
third field.

Now, the definition of exception_table_entry is highly arch-dependent.

For x86, sizeof(struct exception_table_entry) is apparently 12, but
find_extable_entry_size() sets extable_entry_size to 8.

I could fix it, but I do not see much value in this code.

extable_entry_size is used just for selecting a slightly different
error message.

If the first field ("insn") references to a non-executable section,

    The relocation at %s+0x%lx references
    section "%s" which is not executable, IOW
    it is not possible for the kernel to fault
    at that address.  Something is seriously wrong
    and should be fixed.

If the second field ("fixup") references to a non-executable section,

    The relocation at %s+0x%lx references
    section "%s" which is not executable, IOW
    the kernel will fault if it ever tries to
    jump to it.  Something is seriously wrong
    and should be fixed.

Merge the two error messages rather than adding even more complexity.

Change fatal() to error() to make it continue running and catch more
possible errors.

Fixes: 548acf19234d ("x86/mm: Expand the exception table logic to allow new handling options")
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/mod/modpost.c | 60 +++----------------------------------------
 1 file changed, 3 insertions(+), 57 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index c1c523adb139..ba4577aa4f1d 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1292,43 +1292,6 @@ static int is_executable_section(struct elf_info* elf, unsigned int section_inde
 	return ((elf->sechdrs[section_index].sh_flags & SHF_EXECINSTR) == SHF_EXECINSTR);
 }
 
-/*
- * We rely on a gross hack in section_rel[a]() calling find_extable_entry_size()
- * to know the sizeof(struct exception_table_entry) for the target architecture.
- */
-static unsigned int extable_entry_size = 0;
-static void find_extable_entry_size(const char* const sec, const Elf_Rela* r)
-{
-	/*
-	 * If we're currently checking the second relocation within __ex_table,
-	 * that relocation offset tells us the offsetof(struct
-	 * exception_table_entry, fixup) which is equal to sizeof(struct
-	 * exception_table_entry) divided by two.  We use that to our advantage
-	 * since there's no portable way to get that size as every architecture
-	 * seems to go with different sized types.  Not pretty but better than
-	 * hard-coding the size for every architecture..
-	 */
-	if (!extable_entry_size)
-		extable_entry_size = r->r_offset * 2;
-}
-
-static inline bool is_extable_fault_address(Elf_Rela *r)
-{
-	/*
-	 * extable_entry_size is only discovered after we've handled the
-	 * _second_ relocation in __ex_table, so only abort when we're not
-	 * handling the first reloc and extable_entry_size is zero.
-	 */
-	if (r->r_offset && extable_entry_size == 0)
-		fatal("extable_entry size hasn't been discovered!\n");
-
-	return ((r->r_offset == 0) ||
-		(r->r_offset % extable_entry_size == 0));
-}
-
-#define is_second_extable_reloc(Start, Cur, Sec)			\
-	(((Cur) == (Start) + 1) && (strcmp("__ex_table", (Sec)) == 0))
-
 static void report_extable_warnings(const char* modname, struct elf_info* elf,
 				    const struct sectioncheck* const mismatch,
 				    Elf_Rela* r, Elf_Sym* sym,
@@ -1384,22 +1347,9 @@ static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
 		      "You might get more information about where this is\n"
 		      "coming from by using scripts/check_extable.sh %s\n",
 		      fromsec, (long)r->r_offset, tosec, modname);
-	else if (!is_executable_section(elf, get_secindex(elf, sym))) {
-		if (is_extable_fault_address(r))
-			fatal("The relocation at %s+0x%lx references\n"
-			      "section \"%s\" which is not executable, IOW\n"
-			      "it is not possible for the kernel to fault\n"
-			      "at that address.  Something is seriously wrong\n"
-			      "and should be fixed.\n",
-			      fromsec, (long)r->r_offset, tosec);
-		else
-			fatal("The relocation at %s+0x%lx references\n"
-			      "section \"%s\" which is not executable, IOW\n"
-			      "the kernel will fault if it ever tries to\n"
-			      "jump to it.  Something is seriously wrong\n"
-			      "and should be fixed.\n",
-			      fromsec, (long)r->r_offset, tosec);
-	}
+	else if (!is_executable_section(elf, get_secindex(elf, sym)))
+		error("%s+0x%lx references non-executable section '%s'\n",
+		      fromsec, (long)r->r_offset, tosec);
 }
 
 static void check_section_mismatch(const char *modname, struct elf_info *elf,
@@ -1574,8 +1524,6 @@ static void section_rela(const char *modname, struct elf_info *elf,
 		/* Skip special sections */
 		if (is_shndx_special(sym->st_shndx))
 			continue;
-		if (is_second_extable_reloc(start, rela, fromsec))
-			find_extable_entry_size(fromsec, &r);
 		check_section_mismatch(modname, elf, &r, sym, fromsec);
 	}
 }
@@ -1635,8 +1583,6 @@ static void section_rel(const char *modname, struct elf_info *elf,
 		/* Skip special sections */
 		if (is_shndx_special(sym->st_shndx))
 			continue;
-		if (is_second_extable_reloc(start, rel, fromsec))
-			find_extable_entry_size(fromsec, &r);
 		check_section_mismatch(modname, elf, &r, sym, fromsec);
 	}
 }
-- 
2.39.2

[PATCH v5 02/21] modpost: remove fromsym info in __ex_table section mismatch warning

[Masahiro Yamada]

report_extable_warnings() prints "from" in a pretty form, but we know
it is always located in the __ex_table section, i.e. a collection of
struct exception_table_entry.

It is very likely to fail to get the symbol name and ends up with
meaningless message:

  ... in reference from the (unknown reference) (unknown) to ...

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index ba4577aa4f1d..bbe066f7adbc 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1297,23 +1297,16 @@ static void report_extable_warnings(const char* modname, struct elf_info* elf,
 				    Elf_Rela* r, Elf_Sym* sym,
 				    const char* fromsec, const char* tosec)
 {
-	Elf_Sym* fromsym = find_elf_symbol2(elf, r->r_offset, fromsec);
-	const char* fromsym_name = sym_name(elf, fromsym);
 	Elf_Sym* tosym = find_elf_symbol(elf, r->r_addend, sym);
 	const char* tosym_name = sym_name(elf, tosym);
-	const char* from_pretty_name;
-	const char* from_pretty_name_p;
 	const char* to_pretty_name;
 	const char* to_pretty_name_p;
 
-	get_pretty_name(is_function(fromsym),
-			&from_pretty_name, &from_pretty_name_p);
 	get_pretty_name(is_function(tosym),
 			&to_pretty_name, &to_pretty_name_p);
 
-	warn("%s(%s+0x%lx): Section mismatch in reference from the %s %s%s to the %s %s:%s%s\n",
-	     modname, fromsec, (long)r->r_offset, from_pretty_name,
-	     fromsym_name, from_pretty_name_p,
+	warn("%s(%s+0x%lx): Section mismatch in reference to the %s %s:%s%s\n",
+	     modname, fromsec, (long)r->r_offset,
 	     to_pretty_name, tosec, tosym_name, to_pretty_name_p);
 
 	if (!match(tosec, mismatch->bad_tosec) &&
-- 
2.39.2

Re: [PATCH v5 02/21] modpost: remove fromsym info in __ex_table section mismatch warning

[Nick Desaulniers]

On Sun, May 14, 2023 at 8:27 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> report_extable_warnings() prints "from" in a pretty form, but we know
> it is always located in the __ex_table section, i.e. a collection of
> struct exception_table_entry.

Would it still be helpful to have "from __ex_table" somewhere in the
error string that may be shown to developers?

>
> It is very likely to fail to get the symbol name and ends up with
> meaningless message:
>
>   ... in reference from the (unknown reference) (unknown) to ...
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/modpost.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index ba4577aa4f1d..bbe066f7adbc 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1297,23 +1297,16 @@ static void report_extable_warnings(const char* modname, struct elf_info* elf,
>                                     Elf_Rela* r, Elf_Sym* sym,
>                                     const char* fromsec, const char* tosec)
>  {
> -       Elf_Sym* fromsym = find_elf_symbol2(elf, r->r_offset, fromsec);
> -       const char* fromsym_name = sym_name(elf, fromsym);
>         Elf_Sym* tosym = find_elf_symbol(elf, r->r_addend, sym);
>         const char* tosym_name = sym_name(elf, tosym);
> -       const char* from_pretty_name;
> -       const char* from_pretty_name_p;
>         const char* to_pretty_name;
>         const char* to_pretty_name_p;
>
> -       get_pretty_name(is_function(fromsym),
> -                       &from_pretty_name, &from_pretty_name_p);
>         get_pretty_name(is_function(tosym),
>                         &to_pretty_name, &to_pretty_name_p);
>
> -       warn("%s(%s+0x%lx): Section mismatch in reference from the %s %s%s to the %s %s:%s%s\n",
> -            modname, fromsec, (long)r->r_offset, from_pretty_name,
> -            fromsym_name, from_pretty_name_p,
> +       warn("%s(%s+0x%lx): Section mismatch in reference to the %s %s:%s%s\n",
> +            modname, fromsec, (long)r->r_offset,
>              to_pretty_name, tosec, tosym_name, to_pretty_name_p);
>
>         if (!match(tosec, mismatch->bad_tosec) &&
> --
> 2.39.2
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v5 02/21] modpost: remove fromsym info in __ex_table section mismatch warning

[Masahiro Yamada]

On Thu, May 18, 2023 at 3:53 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, May 14, 2023 at 8:27 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > report_extable_warnings() prints "from" in a pretty form, but we know
> > it is always located in the __ex_table section, i.e. a collection of
> > struct exception_table_entry.
>
> Would it still be helpful to have "from __ex_table" somewhere in the
> error string that may be shown to developers?


See the code.

The variable, 'fromsec' (i.e. "__ex_table") is remaining.

It prints a warning message as you wish.




> >
> > It is very likely to fail to get the symbol name and ends up with
> > meaningless message:
> >
> >   ... in reference from the (unknown reference) (unknown) to ...
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  scripts/mod/modpost.c | 11 ++---------
> >  1 file changed, 2 insertions(+), 9 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index ba4577aa4f1d..bbe066f7adbc 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1297,23 +1297,16 @@ static void report_extable_warnings(const char* modname, struct elf_info* elf,
> >                                     Elf_Rela* r, Elf_Sym* sym,
> >                                     const char* fromsec, const char* tosec)
> >  {
> > -       Elf_Sym* fromsym = find_elf_symbol2(elf, r->r_offset, fromsec);
> > -       const char* fromsym_name = sym_name(elf, fromsym);
> >         Elf_Sym* tosym = find_elf_symbol(elf, r->r_addend, sym);
> >         const char* tosym_name = sym_name(elf, tosym);
> > -       const char* from_pretty_name;
> > -       const char* from_pretty_name_p;
> >         const char* to_pretty_name;
> >         const char* to_pretty_name_p;
> >
> > -       get_pretty_name(is_function(fromsym),
> > -                       &from_pretty_name, &from_pretty_name_p);
> >         get_pretty_name(is_function(tosym),
> >                         &to_pretty_name, &to_pretty_name_p);
> >
> > -       warn("%s(%s+0x%lx): Section mismatch in reference from the %s %s%s to the %s %s:%s%s\n",
> > -            modname, fromsec, (long)r->r_offset, from_pretty_name,
> > -            fromsym_name, from_pretty_name_p,
> > +       warn("%s(%s+0x%lx): Section mismatch in reference to the %s %s:%s%s\n",
> > +            modname, fromsec, (long)r->r_offset,
> >              to_pretty_name, tosec, tosym_name, to_pretty_name_p);
> >
> >         if (!match(tosec, mismatch->bad_tosec) &&
> > --
> > 2.39.2
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Best Regards
Masahiro Yamada

[PATCH v5 03/21] modpost: remove get_prettyname()

[Masahiro Yamada]

This is the last user of get_pretty_name() - it is just used to
distinguish whether the symbol is a function or not. It is not
valuable information.

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index bbe066f7adbc..371891d67175 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1207,23 +1207,6 @@ static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
 	return near;
 }
 
-static int is_function(Elf_Sym *sym)
-{
-	if (sym)
-		return ELF_ST_TYPE(sym->st_info) == STT_FUNC;
-	else
-		return -1;
-}
-
-static inline void get_pretty_name(int is_func, const char** name, const char** name_p)
-{
-	switch (is_func) {
-	case 0:	*name = "variable"; *name_p = ""; break;
-	case 1:	*name = "function"; *name_p = "()"; break;
-	default: *name = "(unknown reference)"; *name_p = ""; break;
-	}
-}
-
 /*
  * Print a warning about a section mismatch.
  * Try to find symbols near it so user can find it.
@@ -1299,15 +1282,9 @@ static void report_extable_warnings(const char* modname, struct elf_info* elf,
 {
 	Elf_Sym* tosym = find_elf_symbol(elf, r->r_addend, sym);
 	const char* tosym_name = sym_name(elf, tosym);
-	const char* to_pretty_name;
-	const char* to_pretty_name_p;
 
-	get_pretty_name(is_function(tosym),
-			&to_pretty_name, &to_pretty_name_p);
-
-	warn("%s(%s+0x%lx): Section mismatch in reference to the %s %s:%s%s\n",
-	     modname, fromsec, (long)r->r_offset,
-	     to_pretty_name, tosec, tosym_name, to_pretty_name_p);
+	warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
+	     modname, fromsec, (long)r->r_offset, tosec, tosym_name);
 
 	if (!match(tosec, mismatch->bad_tosec) &&
 	    is_executable_section(elf, get_secindex(elf, sym)))
-- 
2.39.2

Re: [PATCH v5 03/21] modpost: remove get_prettyname()

[Nick Desaulniers]

On Sun, May 14, 2023 at 8:27 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> This is the last user of get_pretty_name() - it is just used to
> distinguish whether the symbol is a function or not. It is not
> valuable information.

I thought it's a nice touch. Not like it should slow things down
unless there are extable warnings to report. May we keep it?

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/modpost.c | 27 ++-------------------------
>  1 file changed, 2 insertions(+), 25 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index bbe066f7adbc..371891d67175 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1207,23 +1207,6 @@ static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
>         return near;
>  }
>
> -static int is_function(Elf_Sym *sym)
> -{
> -       if (sym)
> -               return ELF_ST_TYPE(sym->st_info) == STT_FUNC;
> -       else
> -               return -1;
> -}
> -
> -static inline void get_pretty_name(int is_func, const char** name, const char** name_p)
> -{
> -       switch (is_func) {
> -       case 0: *name = "variable"; *name_p = ""; break;
> -       case 1: *name = "function"; *name_p = "()"; break;
> -       default: *name = "(unknown reference)"; *name_p = ""; break;
> -       }
> -}
> -
>  /*
>   * Print a warning about a section mismatch.
>   * Try to find symbols near it so user can find it.
> @@ -1299,15 +1282,9 @@ static void report_extable_warnings(const char* modname, struct elf_info* elf,
>  {
>         Elf_Sym* tosym = find_elf_symbol(elf, r->r_addend, sym);
>         const char* tosym_name = sym_name(elf, tosym);
> -       const char* to_pretty_name;
> -       const char* to_pretty_name_p;
>
> -       get_pretty_name(is_function(tosym),
> -                       &to_pretty_name, &to_pretty_name_p);
> -
> -       warn("%s(%s+0x%lx): Section mismatch in reference to the %s %s:%s%s\n",
> -            modname, fromsec, (long)r->r_offset,
> -            to_pretty_name, tosec, tosym_name, to_pretty_name_p);
> +       warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
> +            modname, fromsec, (long)r->r_offset, tosec, tosym_name);
>
>         if (!match(tosec, mismatch->bad_tosec) &&
>             is_executable_section(elf, get_secindex(elf, sym)))
> --
> 2.39.2
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v5 03/21] modpost: remove get_prettyname()

[Masahiro Yamada]

On Thu, May 18, 2023 at 6:44 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, May 14, 2023 at 8:27 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > This is the last user of get_pretty_name() - it is just used to
> > distinguish whether the symbol is a function or not. It is not
> > valuable information.
>
> I thought it's a nice touch. Not like it should slow things down
> unless there are extable warnings to report. May we keep it?


I do not think so.

The ex_table quite often points to assembly code, where
the distinction of function/data is not so important.

The extable has a lot of code just to print annoyingly
long error messages.




-- 
Best Regards
Masahiro Yamada

[PATCH v5 04/21] modpost: squash report_extable_warnings() into extable_mismatch_handler()

[Masahiro Yamada]

Collect relevant code into one place to clarify all the cases are
covered by 'if () ... else if ... else ...'.

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

 scripts/mod/modpost.c | 40 ++++++++++++++--------------------------
 1 file changed, 14 insertions(+), 26 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 371891d67175..7a9a3ef8ca0d 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1275,40 +1275,19 @@ static int is_executable_section(struct elf_info* elf, unsigned int section_inde
 	return ((elf->sechdrs[section_index].sh_flags & SHF_EXECINSTR) == SHF_EXECINSTR);
 }
 
-static void report_extable_warnings(const char* modname, struct elf_info* elf,
-				    const struct sectioncheck* const mismatch,
-				    Elf_Rela* r, Elf_Sym* sym,
-				    const char* fromsec, const char* tosec)
-{
-	Elf_Sym* tosym = find_elf_symbol(elf, r->r_addend, sym);
-	const char* tosym_name = sym_name(elf, tosym);
-
-	warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
-	     modname, fromsec, (long)r->r_offset, tosec, tosym_name);
-
-	if (!match(tosec, mismatch->bad_tosec) &&
-	    is_executable_section(elf, get_secindex(elf, sym)))
-		fprintf(stderr,
-			"The relocation at %s+0x%lx references\n"
-			"section \"%s\" which is not in the list of\n"
-			"authorized sections.  If you're adding a new section\n"
-			"and/or if this reference is valid, add \"%s\" to the\n"
-			"list of authorized sections to jump to on fault.\n"
-			"This can be achieved by adding \"%s\" to \n"
-			"OTHER_TEXT_SECTIONS in scripts/mod/modpost.c.\n",
-			fromsec, (long)r->r_offset, tosec, tosec, tosec);
-}
-
 static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
 				     const struct sectioncheck* const mismatch,
 				     Elf_Rela* r, Elf_Sym* sym,
 				     const char *fromsec)
 {
 	const char* tosec = sec_name(elf, get_secindex(elf, sym));
+	Elf_Sym *tosym = find_elf_symbol(elf, r->r_addend, sym);
+	const char *tosym_name = sym_name(elf, tosym);
 
 	sec_mismatch_count++;
 
-	report_extable_warnings(modname, elf, mismatch, r, sym, fromsec, tosec);
+	warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
+	     modname, fromsec, (long)r->r_offset, tosec, tosym_name);
 
 	if (match(tosec, mismatch->bad_tosec))
 		fatal("The relocation at %s+0x%lx references\n"
@@ -1317,7 +1296,16 @@ static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
 		      "You might get more information about where this is\n"
 		      "coming from by using scripts/check_extable.sh %s\n",
 		      fromsec, (long)r->r_offset, tosec, modname);
-	else if (!is_executable_section(elf, get_secindex(elf, sym)))
+	else if (is_executable_section(elf, get_secindex(elf, sym)))
+		warn("The relocation at %s+0x%lx references\n"
+		     "section \"%s\" which is not in the list of\n"
+		     "authorized sections.  If you're adding a new section\n"
+		     "and/or if this reference is valid, add \"%s\" to the\n"
+		     "list of authorized sections to jump to on fault.\n"
+		     "This can be achieved by adding \"%s\" to\n"
+		     "OTHER_TEXT_SECTIONS in scripts/mod/modpost.c.\n",
+		     fromsec, (long)r->r_offset, tosec, tosec, tosec);
+	else
 		error("%s+0x%lx references non-executable section '%s'\n",
 		      fromsec, (long)r->r_offset, tosec);
 }
-- 
2.39.2

Re: [PATCH v5 04/21] modpost: squash report_extable_warnings() into extable_mismatch_handler()

[Nick Desaulniers]

On Sun, May 14, 2023 at 8:27 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Collect relevant code into one place to clarify all the cases are
> covered by 'if () ... else if ... else ...'.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

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

> ---
>
>  scripts/mod/modpost.c | 40 ++++++++++++++--------------------------
>  1 file changed, 14 insertions(+), 26 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 371891d67175..7a9a3ef8ca0d 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1275,40 +1275,19 @@ static int is_executable_section(struct elf_info* elf, unsigned int section_inde
>         return ((elf->sechdrs[section_index].sh_flags & SHF_EXECINSTR) == SHF_EXECINSTR);
>  }
>
> -static void report_extable_warnings(const char* modname, struct elf_info* elf,
> -                                   const struct sectioncheck* const mismatch,
> -                                   Elf_Rela* r, Elf_Sym* sym,
> -                                   const char* fromsec, const char* tosec)
> -{
> -       Elf_Sym* tosym = find_elf_symbol(elf, r->r_addend, sym);
> -       const char* tosym_name = sym_name(elf, tosym);
> -
> -       warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
> -            modname, fromsec, (long)r->r_offset, tosec, tosym_name);
> -
> -       if (!match(tosec, mismatch->bad_tosec) &&
> -           is_executable_section(elf, get_secindex(elf, sym)))
> -               fprintf(stderr,
> -                       "The relocation at %s+0x%lx references\n"
> -                       "section \"%s\" which is not in the list of\n"
> -                       "authorized sections.  If you're adding a new section\n"
> -                       "and/or if this reference is valid, add \"%s\" to the\n"
> -                       "list of authorized sections to jump to on fault.\n"
> -                       "This can be achieved by adding \"%s\" to \n"
> -                       "OTHER_TEXT_SECTIONS in scripts/mod/modpost.c.\n",
> -                       fromsec, (long)r->r_offset, tosec, tosec, tosec);
> -}
> -
>  static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
>                                      const struct sectioncheck* const mismatch,
>                                      Elf_Rela* r, Elf_Sym* sym,
>                                      const char *fromsec)
>  {
>         const char* tosec = sec_name(elf, get_secindex(elf, sym));
> +       Elf_Sym *tosym = find_elf_symbol(elf, r->r_addend, sym);
> +       const char *tosym_name = sym_name(elf, tosym);
>
>         sec_mismatch_count++;
>
> -       report_extable_warnings(modname, elf, mismatch, r, sym, fromsec, tosec);
> +       warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
> +            modname, fromsec, (long)r->r_offset, tosec, tosym_name);
>
>         if (match(tosec, mismatch->bad_tosec))
>                 fatal("The relocation at %s+0x%lx references\n"
> @@ -1317,7 +1296,16 @@ static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
>                       "You might get more information about where this is\n"
>                       "coming from by using scripts/check_extable.sh %s\n",
>                       fromsec, (long)r->r_offset, tosec, modname);
> -       else if (!is_executable_section(elf, get_secindex(elf, sym)))
> +       else if (is_executable_section(elf, get_secindex(elf, sym)))
> +               warn("The relocation at %s+0x%lx references\n"
> +                    "section \"%s\" which is not in the list of\n"
> +                    "authorized sections.  If you're adding a new section\n"
> +                    "and/or if this reference is valid, add \"%s\" to the\n"
> +                    "list of authorized sections to jump to on fault.\n"
> +                    "This can be achieved by adding \"%s\" to\n"
> +                    "OTHER_TEXT_SECTIONS in scripts/mod/modpost.c.\n",
> +                    fromsec, (long)r->r_offset, tosec, tosec, tosec);
> +       else
>                 error("%s+0x%lx references non-executable section '%s'\n",
>                       fromsec, (long)r->r_offset, tosec);
>  }
> --
> 2.39.2
>


-- 
Thanks,
~Nick Desaulniers

[PATCH v5 05/21] modpost: squash report_sec_mismatch() into default_mismatch_handler()

[Masahiro Yamada]

report_sec_mismatch() and default_mismatch_handler() are small enough
to be merged together.

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

 scripts/mod/modpost.c | 55 ++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 35 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 7a9a3ef8ca0d..bb7d1d87bae7 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1207,17 +1207,27 @@ static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
 	return near;
 }
 
-/*
- * Print a warning about a section mismatch.
- * Try to find symbols near it so user can find it.
- * Check whitelist before warning - it may be a false positive.
- */
-static void report_sec_mismatch(const char *modname,
-				const struct sectioncheck *mismatch,
-				const char *fromsec,
-				const char *fromsym,
-				const char *tosec, const char *tosym)
+static void default_mismatch_handler(const char *modname, struct elf_info *elf,
+				     const struct sectioncheck* const mismatch,
+				     Elf_Rela *r, Elf_Sym *sym, const char *fromsec)
 {
+	const char *tosec;
+	Elf_Sym *to;
+	Elf_Sym *from;
+	const char *tosym;
+	const char *fromsym;
+
+	from = find_elf_symbol2(elf, r->r_offset, fromsec);
+	fromsym = sym_name(elf, from);
+
+	tosec = sec_name(elf, get_secindex(elf, sym));
+	to = find_elf_symbol(elf, r->r_addend, sym);
+	tosym = sym_name(elf, to);
+
+	/* check whitelist - we may ignore it */
+	if (!secref_whitelist(mismatch, fromsec, fromsym, tosec, tosym))
+		return;
+
 	sec_mismatch_count++;
 
 	switch (mismatch->mismatch) {
@@ -1242,31 +1252,6 @@ static void report_sec_mismatch(const char *modname,
 	}
 }
 
-static void default_mismatch_handler(const char *modname, struct elf_info *elf,
-				     const struct sectioncheck* const mismatch,
-				     Elf_Rela *r, Elf_Sym *sym, const char *fromsec)
-{
-	const char *tosec;
-	Elf_Sym *to;
-	Elf_Sym *from;
-	const char *tosym;
-	const char *fromsym;
-
-	from = find_elf_symbol2(elf, r->r_offset, fromsec);
-	fromsym = sym_name(elf, from);
-
-	tosec = sec_name(elf, get_secindex(elf, sym));
-	to = find_elf_symbol(elf, r->r_addend, sym);
-	tosym = sym_name(elf, to);
-
-	/* check whitelist - we may ignore it */
-	if (secref_whitelist(mismatch,
-			     fromsec, fromsym, tosec, tosym)) {
-		report_sec_mismatch(modname, mismatch,
-				    fromsec, fromsym, tosec, tosym);
-	}
-}
-
 static int is_executable_section(struct elf_info* elf, unsigned int section_index)
 {
 	if (section_index > elf->num_sections)
-- 
2.39.2

Re: [PATCH v5 05/21] modpost: squash report_sec_mismatch() into default_mismatch_handler()

[Nick Desaulniers]

On Sun, May 14, 2023 at 8:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> report_sec_mismatch() and default_mismatch_handler() are small enough
> to be merged together.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

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

> ---
>
>  scripts/mod/modpost.c | 55 ++++++++++++++++---------------------------
>  1 file changed, 20 insertions(+), 35 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 7a9a3ef8ca0d..bb7d1d87bae7 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1207,17 +1207,27 @@ static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
>         return near;
>  }
>
> -/*
> - * Print a warning about a section mismatch.
> - * Try to find symbols near it so user can find it.
> - * Check whitelist before warning - it may be a false positive.
> - */
> -static void report_sec_mismatch(const char *modname,
> -                               const struct sectioncheck *mismatch,
> -                               const char *fromsec,
> -                               const char *fromsym,
> -                               const char *tosec, const char *tosym)
> +static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> +                                    const struct sectioncheck* const mismatch,
> +                                    Elf_Rela *r, Elf_Sym *sym, const char *fromsec)
>  {
> +       const char *tosec;
> +       Elf_Sym *to;
> +       Elf_Sym *from;
> +       const char *tosym;
> +       const char *fromsym;
> +
> +       from = find_elf_symbol2(elf, r->r_offset, fromsec);
> +       fromsym = sym_name(elf, from);
> +
> +       tosec = sec_name(elf, get_secindex(elf, sym));
> +       to = find_elf_symbol(elf, r->r_addend, sym);
> +       tosym = sym_name(elf, to);
> +
> +       /* check whitelist - we may ignore it */
> +       if (!secref_whitelist(mismatch, fromsec, fromsym, tosec, tosym))
> +               return;
> +
>         sec_mismatch_count++;
>
>         switch (mismatch->mismatch) {
> @@ -1242,31 +1252,6 @@ static void report_sec_mismatch(const char *modname,
>         }
>  }
>
> -static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> -                                    const struct sectioncheck* const mismatch,
> -                                    Elf_Rela *r, Elf_Sym *sym, const char *fromsec)
> -{
> -       const char *tosec;
> -       Elf_Sym *to;
> -       Elf_Sym *from;
> -       const char *tosym;
> -       const char *fromsym;
> -
> -       from = find_elf_symbol2(elf, r->r_offset, fromsec);
> -       fromsym = sym_name(elf, from);
> -
> -       tosec = sec_name(elf, get_secindex(elf, sym));
> -       to = find_elf_symbol(elf, r->r_addend, sym);
> -       tosym = sym_name(elf, to);
> -
> -       /* check whitelist - we may ignore it */
> -       if (secref_whitelist(mismatch,
> -                            fromsec, fromsym, tosec, tosym)) {
> -               report_sec_mismatch(modname, mismatch,
> -                                   fromsec, fromsym, tosec, tosym);
> -       }
> -}
> -
>  static int is_executable_section(struct elf_info* elf, unsigned int section_index)
>  {
>         if (section_index > elf->num_sections)
> --
> 2.39.2
>


-- 
Thanks,
~Nick Desaulniers

[PATCH v5 06/21] modpost: clean up is_executable_section()

[Masahiro Yamada]

SHF_EXECINSTR is a bit flag (#define SHF_EXECINSTR 0x4).
Compare the masked flag to '!= 0'.

There is no good reason to stop modpost immediately even if a special
section index is given. You will get a section mismatch error anyway.

Also, change the return type to bool.

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index bb7d1d87bae7..0bda2f22c985 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1207,6 +1207,14 @@ static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
 	return near;
 }
 
+static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
+{
+	if (secndx > elf->num_sections)
+		return false;
+
+	return (elf->sechdrs[secndx].sh_flags & SHF_EXECINSTR) != 0;
+}
+
 static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 				     const struct sectioncheck* const mismatch,
 				     Elf_Rela *r, Elf_Sym *sym, const char *fromsec)
@@ -1252,14 +1260,6 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 	}
 }
 
-static int is_executable_section(struct elf_info* elf, unsigned int section_index)
-{
-	if (section_index > elf->num_sections)
-		fatal("section_index is outside elf->num_sections!\n");
-
-	return ((elf->sechdrs[section_index].sh_flags & SHF_EXECINSTR) == SHF_EXECINSTR);
-}
-
 static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
 				     const struct sectioncheck* const mismatch,
 				     Elf_Rela* r, Elf_Sym* sym,
-- 
2.39.2

Re: [PATCH v5 06/21] modpost: clean up is_executable_section()

[Nick Desaulniers]

On Sun, May 14, 2023 at 8:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> SHF_EXECINSTR is a bit flag (#define SHF_EXECINSTR 0x4).
> Compare the masked flag to '!= 0'.
>
> There is no good reason to stop modpost immediately even if a special
> section index is given. You will get a section mismatch error anyway.
>
> Also, change the return type to bool.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Moving the definition and renaming the parameter seems very
unnecessary, but whatever. Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>
>  scripts/mod/modpost.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index bb7d1d87bae7..0bda2f22c985 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1207,6 +1207,14 @@ static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
>         return near;
>  }
>
> +static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
> +{
> +       if (secndx > elf->num_sections)
> +               return false;
> +
> +       return (elf->sechdrs[secndx].sh_flags & SHF_EXECINSTR) != 0;
> +}
> +
>  static void default_mismatch_handler(const char *modname, struct elf_info *elf,
>                                      const struct sectioncheck* const mismatch,
>                                      Elf_Rela *r, Elf_Sym *sym, const char *fromsec)
> @@ -1252,14 +1260,6 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
>         }
>  }
>
> -static int is_executable_section(struct elf_info* elf, unsigned int section_index)
> -{
> -       if (section_index > elf->num_sections)
> -               fatal("section_index is outside elf->num_sections!\n");
> -
> -       return ((elf->sechdrs[section_index].sh_flags & SHF_EXECINSTR) == SHF_EXECINSTR);
> -}
> -
>  static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
>                                      const struct sectioncheck* const mismatch,
>                                      Elf_Rela* r, Elf_Sym* sym,
> --
> 2.39.2
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v5 06/21] modpost: clean up is_executable_section()

[Masahiro Yamada]

On Thu, May 18, 2023 at 6:10 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, May 14, 2023 at 8:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > SHF_EXECINSTR is a bit flag (#define SHF_EXECINSTR 0x4).
> > Compare the masked flag to '!= 0'.
> >
> > There is no good reason to stop modpost immediately even if a special
> > section index is given. You will get a section mismatch error anyway.
> >
> > Also, change the return type to bool.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>
> Moving the definition and renaming the parameter seems very
> unnecessary, but whatever. Thanks for the patch!


Moving the definition _is_ necessary.

See the next patch, which moves the call-site of
is_executable_section().

The definition must come before the caller.



The current code exceeds 80-cols per line.

I renamed the parameters so that the lines
fit within 80-cols without wrapping.





> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>
> > ---
> >
> >  scripts/mod/modpost.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index bb7d1d87bae7..0bda2f22c985 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1207,6 +1207,14 @@ static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
> >         return near;
> >  }
> >
> > +static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
> > +{
> > +       if (secndx > elf->num_sections)
> > +               return false;
> > +
> > +       return (elf->sechdrs[secndx].sh_flags & SHF_EXECINSTR) != 0;
> > +}
> > +
> >  static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> >                                      const struct sectioncheck* const mismatch,
> >                                      Elf_Rela *r, Elf_Sym *sym, const char *fromsec)
> > @@ -1252,14 +1260,6 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> >         }
> >  }
> >
> > -static int is_executable_section(struct elf_info* elf, unsigned int section_index)
> > -{
> > -       if (section_index > elf->num_sections)
> > -               fatal("section_index is outside elf->num_sections!\n");
> > -
> > -       return ((elf->sechdrs[section_index].sh_flags & SHF_EXECINSTR) == SHF_EXECINSTR);
> > -}
> > -
> >  static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
> >                                      const struct sectioncheck* const mismatch,
> >                                      Elf_Rela* r, Elf_Sym* sym,
> > --
> > 2.39.2
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Best Regards
Masahiro Yamada

[PATCH v5 07/21] modpost: squash extable_mismatch_handler() into default_mismatch_handler()

[Masahiro Yamada]

Merging these two reduces several lines of code. The extable section
mismatch is already distinguished by EXTABLE_TO_NON_TEXT.

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

 scripts/mod/modpost.c | 84 ++++++++++++++-----------------------------
 1 file changed, 26 insertions(+), 58 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 0bda2f22c985..49357a716519 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -881,27 +881,14 @@ enum mismatch {
  * targeting sections in this array (white-list).  Can be empty.
  *
  * @mismatch: Type of mismatch.
- *
- * @handler: Specific handler to call when a match is found.  If NULL,
- * default_mismatch_handler() will be called.
- *
  */
 struct sectioncheck {
 	const char *fromsec[20];
 	const char *bad_tosec[20];
 	const char *good_tosec[20];
 	enum mismatch mismatch;
-	void (*handler)(const char *modname, struct elf_info *elf,
-			const struct sectioncheck* const mismatch,
-			Elf_Rela *r, Elf_Sym *sym, const char *fromsec);
-
 };
 
-static void extable_mismatch_handler(const char *modname, struct elf_info *elf,
-				     const struct sectioncheck* const mismatch,
-				     Elf_Rela *r, Elf_Sym *sym,
-				     const char *fromsec);
-
 static const struct sectioncheck sectioncheck[] = {
 /* Do not reference init/exit code/data from
  * normal code and data
@@ -974,7 +961,6 @@ static const struct sectioncheck sectioncheck[] = {
 	.bad_tosec = { ".altinstr_replacement", NULL },
 	.good_tosec = {ALL_TEXT_SECTIONS , NULL},
 	.mismatch = EXTABLE_TO_NON_TEXT,
-	.handler = extable_mismatch_handler,
 }
 };
 
@@ -1255,60 +1241,42 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 		     modname, tosym, tosec);
 		break;
 	case EXTABLE_TO_NON_TEXT:
-		fatal("There's a special handler for this mismatch type, we should never get here.\n");
+		warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
+		     modname, fromsec, (long)r->r_offset, tosec, tosym);
+
+		if (match(tosec, mismatch->bad_tosec))
+			fatal("The relocation at %s+0x%lx references\n"
+			      "section \"%s\" which is black-listed.\n"
+			      "Something is seriously wrong and should be fixed.\n"
+			      "You might get more information about where this is\n"
+			      "coming from by using scripts/check_extable.sh %s\n",
+			      fromsec, (long)r->r_offset, tosec, modname);
+		else if (is_executable_section(elf, get_secindex(elf, sym)))
+			warn("The relocation at %s+0x%lx references\n"
+			     "section \"%s\" which is not in the list of\n"
+			     "authorized sections.  If you're adding a new section\n"
+			     "and/or if this reference is valid, add \"%s\" to the\n"
+			     "list of authorized sections to jump to on fault.\n"
+			     "This can be achieved by adding \"%s\" to\n"
+			     "OTHER_TEXT_SECTIONS in scripts/mod/modpost.c.\n",
+			     fromsec, (long)r->r_offset, tosec, tosec, tosec);
+		else
+			error("%s+0x%lx references non-executable section '%s'\n",
+			      fromsec, (long)r->r_offset, tosec);
 		break;
 	}
 }
 
-static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
-				     const struct sectioncheck* const mismatch,
-				     Elf_Rela* r, Elf_Sym* sym,
-				     const char *fromsec)
-{
-	const char* tosec = sec_name(elf, get_secindex(elf, sym));
-	Elf_Sym *tosym = find_elf_symbol(elf, r->r_addend, sym);
-	const char *tosym_name = sym_name(elf, tosym);
-
-	sec_mismatch_count++;
-
-	warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
-	     modname, fromsec, (long)r->r_offset, tosec, tosym_name);
-
-	if (match(tosec, mismatch->bad_tosec))
-		fatal("The relocation at %s+0x%lx references\n"
-		      "section \"%s\" which is black-listed.\n"
-		      "Something is seriously wrong and should be fixed.\n"
-		      "You might get more information about where this is\n"
-		      "coming from by using scripts/check_extable.sh %s\n",
-		      fromsec, (long)r->r_offset, tosec, modname);
-	else if (is_executable_section(elf, get_secindex(elf, sym)))
-		warn("The relocation at %s+0x%lx references\n"
-		     "section \"%s\" which is not in the list of\n"
-		     "authorized sections.  If you're adding a new section\n"
-		     "and/or if this reference is valid, add \"%s\" to the\n"
-		     "list of authorized sections to jump to on fault.\n"
-		     "This can be achieved by adding \"%s\" to\n"
-		     "OTHER_TEXT_SECTIONS in scripts/mod/modpost.c.\n",
-		     fromsec, (long)r->r_offset, tosec, tosec, tosec);
-	else
-		error("%s+0x%lx references non-executable section '%s'\n",
-		      fromsec, (long)r->r_offset, tosec);
-}
-
 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 struct sectioncheck *mismatch = section_mismatch(fromsec, tosec);
 
-	if (mismatch) {
-		if (mismatch->handler)
-			mismatch->handler(modname, elf,  mismatch,
-					  r, sym, fromsec);
-		else
-			default_mismatch_handler(modname, elf, mismatch,
-						 r, sym, fromsec);
-	}
+	if (!mismatch)
+		return;
+
+	default_mismatch_handler(modname, elf, mismatch, r, sym, fromsec);
 }
 
 static unsigned int *reloc_location(struct elf_info *elf,
-- 
2.39.2

Re: [PATCH v5 07/21] modpost: squash extable_mismatch_handler() into default_mismatch_handler()

[Nick Desaulniers]

On Sun, May 14, 2023 at 8:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Merging these two reduces several lines of code. The extable section
> mismatch is already distinguished by EXTABLE_TO_NON_TEXT.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

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

> ---
>
>  scripts/mod/modpost.c | 84 ++++++++++++++-----------------------------
>  1 file changed, 26 insertions(+), 58 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 0bda2f22c985..49357a716519 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -881,27 +881,14 @@ enum mismatch {
>   * targeting sections in this array (white-list).  Can be empty.
>   *
>   * @mismatch: Type of mismatch.
> - *
> - * @handler: Specific handler to call when a match is found.  If NULL,
> - * default_mismatch_handler() will be called.
> - *
>   */
>  struct sectioncheck {
>         const char *fromsec[20];
>         const char *bad_tosec[20];
>         const char *good_tosec[20];
>         enum mismatch mismatch;
> -       void (*handler)(const char *modname, struct elf_info *elf,
> -                       const struct sectioncheck* const mismatch,
> -                       Elf_Rela *r, Elf_Sym *sym, const char *fromsec);
> -
>  };
>
> -static void extable_mismatch_handler(const char *modname, struct elf_info *elf,
> -                                    const struct sectioncheck* const mismatch,
> -                                    Elf_Rela *r, Elf_Sym *sym,
> -                                    const char *fromsec);
> -
>  static const struct sectioncheck sectioncheck[] = {
>  /* Do not reference init/exit code/data from
>   * normal code and data
> @@ -974,7 +961,6 @@ static const struct sectioncheck sectioncheck[] = {
>         .bad_tosec = { ".altinstr_replacement", NULL },
>         .good_tosec = {ALL_TEXT_SECTIONS , NULL},
>         .mismatch = EXTABLE_TO_NON_TEXT,
> -       .handler = extable_mismatch_handler,
>  }
>  };
>
> @@ -1255,60 +1241,42 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
>                      modname, tosym, tosec);
>                 break;
>         case EXTABLE_TO_NON_TEXT:
> -               fatal("There's a special handler for this mismatch type, we should never get here.\n");
> +               warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
> +                    modname, fromsec, (long)r->r_offset, tosec, tosym);
> +
> +               if (match(tosec, mismatch->bad_tosec))
> +                       fatal("The relocation at %s+0x%lx references\n"
> +                             "section \"%s\" which is black-listed.\n"
> +                             "Something is seriously wrong and should be fixed.\n"
> +                             "You might get more information about where this is\n"
> +                             "coming from by using scripts/check_extable.sh %s\n",
> +                             fromsec, (long)r->r_offset, tosec, modname);
> +               else if (is_executable_section(elf, get_secindex(elf, sym)))
> +                       warn("The relocation at %s+0x%lx references\n"
> +                            "section \"%s\" which is not in the list of\n"
> +                            "authorized sections.  If you're adding a new section\n"
> +                            "and/or if this reference is valid, add \"%s\" to the\n"
> +                            "list of authorized sections to jump to on fault.\n"
> +                            "This can be achieved by adding \"%s\" to\n"
> +                            "OTHER_TEXT_SECTIONS in scripts/mod/modpost.c.\n",
> +                            fromsec, (long)r->r_offset, tosec, tosec, tosec);
> +               else
> +                       error("%s+0x%lx references non-executable section '%s'\n",
> +                             fromsec, (long)r->r_offset, tosec);
>                 break;
>         }
>  }
>
> -static void extable_mismatch_handler(const char* modname, struct elf_info *elf,
> -                                    const struct sectioncheck* const mismatch,
> -                                    Elf_Rela* r, Elf_Sym* sym,
> -                                    const char *fromsec)
> -{
> -       const char* tosec = sec_name(elf, get_secindex(elf, sym));
> -       Elf_Sym *tosym = find_elf_symbol(elf, r->r_addend, sym);
> -       const char *tosym_name = sym_name(elf, tosym);
> -
> -       sec_mismatch_count++;
> -
> -       warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
> -            modname, fromsec, (long)r->r_offset, tosec, tosym_name);
> -
> -       if (match(tosec, mismatch->bad_tosec))
> -               fatal("The relocation at %s+0x%lx references\n"
> -                     "section \"%s\" which is black-listed.\n"
> -                     "Something is seriously wrong and should be fixed.\n"
> -                     "You might get more information about where this is\n"
> -                     "coming from by using scripts/check_extable.sh %s\n",
> -                     fromsec, (long)r->r_offset, tosec, modname);
> -       else if (is_executable_section(elf, get_secindex(elf, sym)))
> -               warn("The relocation at %s+0x%lx references\n"
> -                    "section \"%s\" which is not in the list of\n"
> -                    "authorized sections.  If you're adding a new section\n"
> -                    "and/or if this reference is valid, add \"%s\" to the\n"
> -                    "list of authorized sections to jump to on fault.\n"
> -                    "This can be achieved by adding \"%s\" to\n"
> -                    "OTHER_TEXT_SECTIONS in scripts/mod/modpost.c.\n",
> -                    fromsec, (long)r->r_offset, tosec, tosec, tosec);
> -       else
> -               error("%s+0x%lx references non-executable section '%s'\n",
> -                     fromsec, (long)r->r_offset, tosec);
> -}
> -
>  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 struct sectioncheck *mismatch = section_mismatch(fromsec, tosec);
>
> -       if (mismatch) {
> -               if (mismatch->handler)
> -                       mismatch->handler(modname, elf,  mismatch,
> -                                         r, sym, fromsec);
> -               else
> -                       default_mismatch_handler(modname, elf, mismatch,
> -                                                r, sym, fromsec);
> -       }
> +       if (!mismatch)
> +               return;
> +
> +       default_mismatch_handler(modname, elf, mismatch, r, sym, fromsec);
>  }
>
>  static unsigned int *reloc_location(struct elf_info *elf,
> --
> 2.39.2
>


-- 
Thanks,
~Nick Desaulniers

[PATCH v5 08/21] modpost: pass 'tosec' down to default_mismatch_handler()

[Masahiro Yamada]

default_mismatch_handler() does not need to compute 'tosec' because
it is calculated by the caller.

Pass it down to default_mismatch_handler() instead of calling
sec_name() twice.

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

 scripts/mod/modpost.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 49357a716519..2cc9c2a4aadc 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1203,9 +1203,9 @@ static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
 
 static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 				     const struct sectioncheck* const mismatch,
-				     Elf_Rela *r, Elf_Sym *sym, const char *fromsec)
+				     Elf_Rela *r, Elf_Sym *sym, const char *fromsec,
+				     const char *tosec)
 {
-	const char *tosec;
 	Elf_Sym *to;
 	Elf_Sym *from;
 	const char *tosym;
@@ -1214,7 +1214,6 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 	from = find_elf_symbol2(elf, r->r_offset, fromsec);
 	fromsym = sym_name(elf, from);
 
-	tosec = sec_name(elf, get_secindex(elf, sym));
 	to = find_elf_symbol(elf, r->r_addend, sym);
 	tosym = sym_name(elf, to);
 
@@ -1276,7 +1275,7 @@ static void check_section_mismatch(const char *modname, struct elf_info *elf,
 	if (!mismatch)
 		return;
 
-	default_mismatch_handler(modname, elf, mismatch, r, sym, fromsec);
+	default_mismatch_handler(modname, elf, mismatch, r, sym, fromsec, tosec);
 }
 
 static unsigned int *reloc_location(struct elf_info *elf,
-- 
2.39.2

Re: [PATCH v5 08/21] modpost: pass 'tosec' down to default_mismatch_handler()

[Nick Desaulniers]

On Sun, May 14, 2023 at 8:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> default_mismatch_handler() does not need to compute 'tosec' because
> it is calculated by the caller.
>
> Pass it down to default_mismatch_handler() instead of calling
> sec_name() twice.

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

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/modpost.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 49357a716519..2cc9c2a4aadc 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1203,9 +1203,9 @@ static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
>
>  static void default_mismatch_handler(const char *modname, struct elf_info *elf,
>                                      const struct sectioncheck* const mismatch,
> -                                    Elf_Rela *r, Elf_Sym *sym, const char *fromsec)
> +                                    Elf_Rela *r, Elf_Sym *sym, const char *fromsec,
> +                                    const char *tosec)
>  {
> -       const char *tosec;
>         Elf_Sym *to;
>         Elf_Sym *from;
>         const char *tosym;
> @@ -1214,7 +1214,6 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
>         from = find_elf_symbol2(elf, r->r_offset, fromsec);
>         fromsym = sym_name(elf, from);
>
> -       tosec = sec_name(elf, get_secindex(elf, sym));
>         to = find_elf_symbol(elf, r->r_addend, sym);
>         tosym = sym_name(elf, to);
>
> @@ -1276,7 +1275,7 @@ static void check_section_mismatch(const char *modname, struct elf_info *elf,
>         if (!mismatch)
>                 return;
>
> -       default_mismatch_handler(modname, elf, mismatch, r, sym, fromsec);
> +       default_mismatch_handler(modname, elf, mismatch, r, sym, fromsec, tosec);
>  }
>
>  static unsigned int *reloc_location(struct elf_info *elf,
> --
> 2.39.2
>


-- 
Thanks,
~Nick Desaulniers

[PATCH v5 09/21] modpost: pass section index to find_elf_symbol2()

[Masahiro Yamada]

find_elf_symbol2() converts the section index to the section name,
then compares the two section names in each iteration. This is slow.

It is faster to compare the section indices (i.e. integers) directly.

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

 scripts/mod/modpost.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 2cc9c2a4aadc..3b7b78e69137 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1169,19 +1169,14 @@ static Elf_Sym *find_elf_symbol(struct elf_info *elf, Elf64_Sword addr,
  * it is, but this works for now.
  **/
 static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
-				 const char *sec)
+				 unsigned int secndx)
 {
 	Elf_Sym *sym;
 	Elf_Sym *near = NULL;
 	Elf_Addr distance = ~0;
 
 	for (sym = elf->symtab_start; sym < elf->symtab_stop; sym++) {
-		const char *symsec;
-
-		if (is_shndx_special(sym->st_shndx))
-			continue;
-		symsec = sec_name(elf, get_secindex(elf, sym));
-		if (strcmp(symsec, sec) != 0)
+		if (get_secindex(elf, sym) != secndx)
 			continue;
 		if (!is_valid_name(elf, sym))
 			continue;
@@ -1203,7 +1198,8 @@ static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
 
 static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 				     const struct sectioncheck* const mismatch,
-				     Elf_Rela *r, Elf_Sym *sym, const char *fromsec,
+				     Elf_Rela *r, Elf_Sym *sym,
+				     unsigned int fsecndx, const char *fromsec,
 				     const char *tosec)
 {
 	Elf_Sym *to;
@@ -1211,7 +1207,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 	const char *tosym;
 	const char *fromsym;
 
-	from = find_elf_symbol2(elf, r->r_offset, fromsec);
+	from = find_elf_symbol2(elf, r->r_offset, fsecndx);
 	fromsym = sym_name(elf, from);
 
 	to = find_elf_symbol(elf, r->r_addend, sym);
@@ -1267,7 +1263,8 @@ static void default_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)
+				   Elf_Rela *r, Elf_Sym *sym,
+				   unsigned int fsecndx, const char *fromsec)
 {
 	const char *tosec = sec_name(elf, get_secindex(elf, sym));
 	const struct sectioncheck *mismatch = section_mismatch(fromsec, tosec);
@@ -1275,7 +1272,8 @@ static void check_section_mismatch(const char *modname, struct elf_info *elf,
 	if (!mismatch)
 		return;
 
-	default_mismatch_handler(modname, elf, mismatch, r, sym, fromsec, tosec);
+	default_mismatch_handler(modname, elf, mismatch, r, sym, fsecndx, fromsec,
+				 tosec);
 }
 
 static unsigned int *reloc_location(struct elf_info *elf,
@@ -1390,12 +1388,11 @@ static void section_rela(const char *modname, struct elf_info *elf,
 	Elf_Rela *rela;
 	Elf_Rela r;
 	unsigned int r_sym;
-	const char *fromsec;
-
+	unsigned int fsecndx = sechdr->sh_info;
+	const char *fromsec = sec_name(elf, fsecndx);
 	Elf_Rela *start = (void *)elf->hdr + sechdr->sh_offset;
 	Elf_Rela *stop  = (void *)start + sechdr->sh_size;
 
-	fromsec = sec_name(elf, sechdr->sh_info);
 	/* if from section (name) is know good then skip it */
 	if (match(fromsec, section_white_list))
 		return;
@@ -1434,7 +1431,7 @@ static void section_rela(const char *modname, struct elf_info *elf,
 		/* Skip special sections */
 		if (is_shndx_special(sym->st_shndx))
 			continue;
-		check_section_mismatch(modname, elf, &r, sym, fromsec);
+		check_section_mismatch(modname, elf, &r, sym, fsecndx, fromsec);
 	}
 }
 
@@ -1445,12 +1442,11 @@ static void section_rel(const char *modname, struct elf_info *elf,
 	Elf_Rel *rel;
 	Elf_Rela r;
 	unsigned int r_sym;
-	const char *fromsec;
-
+	unsigned int fsecndx = sechdr->sh_info;
+	const char *fromsec = sec_name(elf, fsecndx);
 	Elf_Rel *start = (void *)elf->hdr + sechdr->sh_offset;
 	Elf_Rel *stop  = (void *)start + sechdr->sh_size;
 
-	fromsec = sec_name(elf, sechdr->sh_info);
 	/* if from section (name) is know good then skip it */
 	if (match(fromsec, section_white_list))
 		return;
@@ -1493,7 +1489,7 @@ static void section_rel(const char *modname, struct elf_info *elf,
 		/* Skip special sections */
 		if (is_shndx_special(sym->st_shndx))
 			continue;
-		check_section_mismatch(modname, elf, &r, sym, fromsec);
+		check_section_mismatch(modname, elf, &r, sym, fsecndx, fromsec);
 	}
 }
 
-- 
2.39.2

Re: [PATCH v5 09/21] modpost: pass section index to find_elf_symbol2()

[Nick Desaulniers]

On Sun, May 14, 2023 at 8:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> find_elf_symbol2() converts the section index to the section name,
> then compares the two section names in each iteration. This is slow.
>
> It is faster to compare the section indices (i.e. integers) directly.

Good idea!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/modpost.c | 34 +++++++++++++++-------------------
>  1 file changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 2cc9c2a4aadc..3b7b78e69137 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1169,19 +1169,14 @@ static Elf_Sym *find_elf_symbol(struct elf_info *elf, Elf64_Sword addr,
>   * it is, but this works for now.
>   **/
>  static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
> -                                const char *sec)
> +                                unsigned int secndx)
>  {
>         Elf_Sym *sym;
>         Elf_Sym *near = NULL;
>         Elf_Addr distance = ~0;
>
>         for (sym = elf->symtab_start; sym < elf->symtab_stop; sym++) {
> -               const char *symsec;
> -
> -               if (is_shndx_special(sym->st_shndx))
> -                       continue;
> -               symsec = sec_name(elf, get_secindex(elf, sym));
> -               if (strcmp(symsec, sec) != 0)
> +               if (get_secindex(elf, sym) != secndx)
>                         continue;
>                 if (!is_valid_name(elf, sym))
>                         continue;
> @@ -1203,7 +1198,8 @@ static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
>
>  static void default_mismatch_handler(const char *modname, struct elf_info *elf,
>                                      const struct sectioncheck* const mismatch,
> -                                    Elf_Rela *r, Elf_Sym *sym, const char *fromsec,
> +                                    Elf_Rela *r, Elf_Sym *sym,
> +                                    unsigned int fsecndx, const char *fromsec,
>                                      const char *tosec)
>  {
>         Elf_Sym *to;
> @@ -1211,7 +1207,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
>         const char *tosym;
>         const char *fromsym;
>
> -       from = find_elf_symbol2(elf, r->r_offset, fromsec);
> +       from = find_elf_symbol2(elf, r->r_offset, fsecndx);
>         fromsym = sym_name(elf, from);
>
>         to = find_elf_symbol(elf, r->r_addend, sym);
> @@ -1267,7 +1263,8 @@ static void default_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)
> +                                  Elf_Rela *r, Elf_Sym *sym,
> +                                  unsigned int fsecndx, const char *fromsec)
>  {
>         const char *tosec = sec_name(elf, get_secindex(elf, sym));
>         const struct sectioncheck *mismatch = section_mismatch(fromsec, tosec);
> @@ -1275,7 +1272,8 @@ static void check_section_mismatch(const char *modname, struct elf_info *elf,
>         if (!mismatch)
>                 return;
>
> -       default_mismatch_handler(modname, elf, mismatch, r, sym, fromsec, tosec);
> +       default_mismatch_handler(modname, elf, mismatch, r, sym, fsecndx, fromsec,
> +                                tosec);
>  }
>
>  static unsigned int *reloc_location(struct elf_info *elf,
> @@ -1390,12 +1388,11 @@ static void section_rela(const char *modname, struct elf_info *elf,
>         Elf_Rela *rela;
>         Elf_Rela r;
>         unsigned int r_sym;
> -       const char *fromsec;
> -
> +       unsigned int fsecndx = sechdr->sh_info;
> +       const char *fromsec = sec_name(elf, fsecndx);
>         Elf_Rela *start = (void *)elf->hdr + sechdr->sh_offset;
>         Elf_Rela *stop  = (void *)start + sechdr->sh_size;
>
> -       fromsec = sec_name(elf, sechdr->sh_info);
>         /* if from section (name) is know good then skip it */
>         if (match(fromsec, section_white_list))
>                 return;
> @@ -1434,7 +1431,7 @@ static void section_rela(const char *modname, struct elf_info *elf,
>                 /* Skip special sections */
>                 if (is_shndx_special(sym->st_shndx))
>                         continue;
> -               check_section_mismatch(modname, elf, &r, sym, fromsec);
> +               check_section_mismatch(modname, elf, &r, sym, fsecndx, fromsec);
>         }
>  }
>
> @@ -1445,12 +1442,11 @@ static void section_rel(const char *modname, struct elf_info *elf,
>         Elf_Rel *rel;
>         Elf_Rela r;
>         unsigned int r_sym;
> -       const char *fromsec;
> -
> +       unsigned int fsecndx = sechdr->sh_info;
> +       const char *fromsec = sec_name(elf, fsecndx);
>         Elf_Rel *start = (void *)elf->hdr + sechdr->sh_offset;
>         Elf_Rel *stop  = (void *)start + sechdr->sh_size;
>
> -       fromsec = sec_name(elf, sechdr->sh_info);
>         /* if from section (name) is know good then skip it */
>         if (match(fromsec, section_white_list))
>                 return;
> @@ -1493,7 +1489,7 @@ static void section_rel(const char *modname, struct elf_info *elf,
>                 /* Skip special sections */
>                 if (is_shndx_special(sym->st_shndx))
>                         continue;
> -               check_section_mismatch(modname, elf, &r, sym, fromsec);
> +               check_section_mismatch(modname, elf, &r, sym, fsecndx, fromsec);
>         }
>  }
>
> --
> 2.39.2
>


-- 
Thanks,
~Nick Desaulniers

[PATCH v5 10/21] modpost: rename find_elf_symbol() and find_elf_symbol2()

[Masahiro Yamada]

find_elf_symbol() and find_elf_symbol2() are not good names.

Rename them to find_tosym(), find_fromsym(), respectively.

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

Changes in v5:
  - Change the names

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 3b7b78e69137..0d2c2aff2c03 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1124,8 +1124,8 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
  * In other cases the symbol needs to be looked up in the symbol table
  * based on section and address.
  *  **/
-static Elf_Sym *find_elf_symbol(struct elf_info *elf, Elf64_Sword addr,
-				Elf_Sym *relsym)
+static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
+			   Elf_Sym *relsym)
 {
 	Elf_Sym *sym;
 	Elf_Sym *near = NULL;
@@ -1168,8 +1168,8 @@ static Elf_Sym *find_elf_symbol(struct elf_info *elf, Elf64_Sword addr,
  * The ELF format may have a better way to detect what type of symbol
  * it is, but this works for now.
  **/
-static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
-				 unsigned int secndx)
+static Elf_Sym *find_fromsym(struct elf_info *elf, Elf_Addr addr,
+			     unsigned int secndx)
 {
 	Elf_Sym *sym;
 	Elf_Sym *near = NULL;
@@ -1207,10 +1207,10 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 	const char *tosym;
 	const char *fromsym;
 
-	from = find_elf_symbol2(elf, r->r_offset, fsecndx);
+	from = find_fromsym(elf, r->r_offset, fsecndx);
 	fromsym = sym_name(elf, from);
 
-	to = find_elf_symbol(elf, r->r_addend, sym);
+	to = find_tosym(elf, r->r_addend, sym);
 	tosym = sym_name(elf, to);
 
 	/* check whitelist - we may ignore it */
-- 
2.39.2

Re: [PATCH v5 10/21] modpost: rename find_elf_symbol() and find_elf_symbol2()

[Nick Desaulniers]

On Sun, May 14, 2023 at 8:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> find_elf_symbol() and find_elf_symbol2() are not good names.
>
> Rename them to find_tosym(), find_fromsym(), respectively.

The comments maybe could be updated, too. The end of the comment looks
wrong for both.

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

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> Changes in v5:
>   - Change the names
>
>  scripts/mod/modpost.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 3b7b78e69137..0d2c2aff2c03 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1124,8 +1124,8 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
>   * In other cases the symbol needs to be looked up in the symbol table
>   * based on section and address.
>   *  **/
> -static Elf_Sym *find_elf_symbol(struct elf_info *elf, Elf64_Sword addr,
> -                               Elf_Sym *relsym)
> +static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
> +                          Elf_Sym *relsym)
>  {
>         Elf_Sym *sym;
>         Elf_Sym *near = NULL;
> @@ -1168,8 +1168,8 @@ static Elf_Sym *find_elf_symbol(struct elf_info *elf, Elf64_Sword addr,
>   * The ELF format may have a better way to detect what type of symbol
>   * it is, but this works for now.
>   **/
> -static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
> -                                unsigned int secndx)
> +static Elf_Sym *find_fromsym(struct elf_info *elf, Elf_Addr addr,
> +                            unsigned int secndx)
>  {
>         Elf_Sym *sym;
>         Elf_Sym *near = NULL;
> @@ -1207,10 +1207,10 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
>         const char *tosym;
>         const char *fromsym;
>
> -       from = find_elf_symbol2(elf, r->r_offset, fsecndx);
> +       from = find_fromsym(elf, r->r_offset, fsecndx);
>         fromsym = sym_name(elf, from);
>
> -       to = find_elf_symbol(elf, r->r_addend, sym);
> +       to = find_tosym(elf, r->r_addend, sym);
>         tosym = sym_name(elf, to);
>
>         /* check whitelist - we may ignore it */
> --
> 2.39.2
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v5 10/21] modpost: rename find_elf_symbol() and find_elf_symbol2()

[Masahiro Yamada]

On Thu, May 18, 2023 at 6:14 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, May 14, 2023 at 8:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > find_elf_symbol() and find_elf_symbol2() are not good names.
> >
> > Rename them to find_tosym(), find_fromsym(), respectively.
>
> The comments maybe could be updated, too. The end of the comment looks
> wrong for both.


What do you mean?

Please tell me which part should be changed, and how.






> Thanks for the patch!
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> > Changes in v5:
> >   - Change the names
> >
> >  scripts/mod/modpost.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 3b7b78e69137..0d2c2aff2c03 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1124,8 +1124,8 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
> >   * In other cases the symbol needs to be looked up in the symbol table
> >   * based on section and address.
> >   *  **/
> > -static Elf_Sym *find_elf_symbol(struct elf_info *elf, Elf64_Sword addr,
> > -                               Elf_Sym *relsym)
> > +static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
> > +                          Elf_Sym *relsym)
> >  {
> >         Elf_Sym *sym;
> >         Elf_Sym *near = NULL;
> > @@ -1168,8 +1168,8 @@ static Elf_Sym *find_elf_symbol(struct elf_info *elf, Elf64_Sword addr,
> >   * The ELF format may have a better way to detect what type of symbol
> >   * it is, but this works for now.
> >   **/
> > -static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
> > -                                unsigned int secndx)
> > +static Elf_Sym *find_fromsym(struct elf_info *elf, Elf_Addr addr,
> > +                            unsigned int secndx)
> >  {
> >         Elf_Sym *sym;
> >         Elf_Sym *near = NULL;
> > @@ -1207,10 +1207,10 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> >         const char *tosym;
> >         const char *fromsym;
> >
> > -       from = find_elf_symbol2(elf, r->r_offset, fsecndx);
> > +       from = find_fromsym(elf, r->r_offset, fsecndx);
> >         fromsym = sym_name(elf, from);
> >
> > -       to = find_elf_symbol(elf, r->r_addend, sym);
> > +       to = find_tosym(elf, r->r_addend, sym);
> >         tosym = sym_name(elf, to);
> >
> >         /* check whitelist - we may ignore it */
> > --
> > 2.39.2
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v5 10/21] modpost: rename find_elf_symbol() and find_elf_symbol2()

[Nick Desaulniers]

[-- Attachment #1: Type: text/plain, Size: 3263 bytes --]

On Sat, May 20, 2023 at 6:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Thu, May 18, 2023 at 6:14 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Sun, May 14, 2023 at 8:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > find_elf_symbol() and find_elf_symbol2() are not good names.
> > >
> > > Rename them to find_tosym(), find_fromsym(), respectively.
> >
> > The comments maybe could be updated, too. The end of the comment looks
> > wrong for both.
>
>
> What do you mean?
>
> Please tell me which part should be changed, and how.

Attached the comment style changes.  I didn't have precise wording in
mind for the comments; I was suggesting to see if the comments could
be updated to clarify what the functions are doing.

>
>
>
>
>
>
> > Thanks for the patch!
> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> >
> > >
> > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > ---
> > >
> > > Changes in v5:
> > >   - Change the names
> > >
> > >  scripts/mod/modpost.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > > index 3b7b78e69137..0d2c2aff2c03 100644
> > > --- a/scripts/mod/modpost.c
> > > +++ b/scripts/mod/modpost.c
> > > @@ -1124,8 +1124,8 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
> > >   * In other cases the symbol needs to be looked up in the symbol table
> > >   * based on section and address.
> > >   *  **/
> > > -static Elf_Sym *find_elf_symbol(struct elf_info *elf, Elf64_Sword addr,
> > > -                               Elf_Sym *relsym)
> > > +static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
> > > +                          Elf_Sym *relsym)
> > >  {
> > >         Elf_Sym *sym;
> > >         Elf_Sym *near = NULL;
> > > @@ -1168,8 +1168,8 @@ static Elf_Sym *find_elf_symbol(struct elf_info *elf, Elf64_Sword addr,
> > >   * The ELF format may have a better way to detect what type of symbol
> > >   * it is, but this works for now.
> > >   **/
> > > -static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
> > > -                                unsigned int secndx)
> > > +static Elf_Sym *find_fromsym(struct elf_info *elf, Elf_Addr addr,
> > > +                            unsigned int secndx)
> > >  {
> > >         Elf_Sym *sym;
> > >         Elf_Sym *near = NULL;
> > > @@ -1207,10 +1207,10 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> > >         const char *tosym;
> > >         const char *fromsym;
> > >
> > > -       from = find_elf_symbol2(elf, r->r_offset, fsecndx);
> > > +       from = find_fromsym(elf, r->r_offset, fsecndx);
> > >         fromsym = sym_name(elf, from);
> > >
> > > -       to = find_elf_symbol(elf, r->r_addend, sym);
> > > +       to = find_tosym(elf, r->r_addend, sym);
> > >         tosym = sym_name(elf, to);
> > >
> > >         /* check whitelist - we may ignore it */
> > > --
> > > 2.39.2
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
>
>
>
> --
> Best Regards
> Masahiro Yamada



-- 
Thanks,
~Nick Desaulniers

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1074 bytes --]

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index d4531d09984d..1926e503b0d6 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1131,13 +1131,13 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
 	return !is_mapping_symbol(name);
 }
 
-/**
+/*
  * Find symbol based on relocation record info.
  * In some cases the symbol supplied is a valid symbol so
  * return refsym. If st_name != 0 we assume this is a valid symbol.
  * In other cases the symbol needs to be looked up in the symbol table
  * based on section and address.
- *  **/
+ */
 static Elf_Sym *find_elf_symbol(struct elf_info *elf, Elf64_Sword addr,
 				Elf_Sym *relsym)
 {
@@ -1181,7 +1181,7 @@ static Elf_Sym *find_elf_symbol(struct elf_info *elf, Elf64_Sword addr,
  * If we find two symbols with equal offset prefer one with a valid name.
  * The ELF format may have a better way to detect what type of symbol
  * it is, but this works for now.
- **/
+ */
 static Elf_Sym *find_elf_symbol2(struct elf_info *elf, Elf_Addr addr,
 				 const char *sec)
 {

Re: [PATCH v5 10/21] modpost: rename find_elf_symbol() and find_elf_symbol2()

[Masahiro Yamada]

On Tue, May 23, 2023 at 1:59 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sat, May 20, 2023 at 6:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Thu, May 18, 2023 at 6:14 AM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > >
> > > On Sun, May 14, 2023 at 8:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > >
> > > > find_elf_symbol() and find_elf_symbol2() are not good names.
> > > >
> > > > Rename them to find_tosym(), find_fromsym(), respectively.
> > >
> > > The comments maybe could be updated, too. The end of the comment looks
> > > wrong for both.
> >
> >
> > What do you mean?
> >
> > Please tell me which part should be changed, and how.
>
> Attached the comment style changes.  I didn't have precise wording in
> mind for the comments; I was suggesting to see if the comments could
> be updated to clarify what the functions are doing.

Ah, I understood what you meant.

I think some parts of the comments are stale.
Anyway, these comment blocks will be removed by a later commit.

https://patchwork.kernel.org/project/linux-kbuild/patch/20230521160426.1881124-6-masahiroy@kernel.org/



-- 
Best Regards
Masahiro Yamada

[PATCH v5 11/21] modpost: modpost: refactor find_fromsym() and find_tosym()

[Masahiro Yamada]

find_fromsym() and find_tosym() are similar - both of them iterate
in the .symtab section and return the near symbol.

Factor out the common part into find_nearest_sym().

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

Changes in v5:
  - Fix broken symbol name in x86_64

 scripts/mod/modpost.c | 95 ++++++++++++++++++-------------------------
 1 file changed, 40 insertions(+), 55 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 0d2c2aff2c03..d147e8f63e52 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1117,77 +1117,62 @@ static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
 	return !is_mapping_symbol(name);
 }
 
-/**
- * Find symbol based on relocation record info.
- * In some cases the symbol supplied is a valid symbol so
- * return refsym. If st_name != 0 we assume this is a valid symbol.
- * In other cases the symbol needs to be looked up in the symbol table
- * based on section and address.
- *  **/
-static Elf_Sym *find_tosym(struct elf_info *elf, Elf64_Sword addr,
-			   Elf_Sym *relsym)
+/* Look up the nearest symbol based on the section and the address */
+static Elf_Sym *find_nearest_sym(struct elf_info *elf, Elf_Addr addr,
+				 unsigned int secndx, bool allow_negative)
 {
 	Elf_Sym *sym;
 	Elf_Sym *near = NULL;
-	Elf64_Sword distance = 20;
-	Elf64_Sword d;
-	unsigned int relsym_secindex;
-
-	if (relsym->st_name != 0)
-		return relsym;
-
-	relsym_secindex = get_secindex(elf, relsym);
-	for (sym = elf->symtab_start; sym < elf->symtab_stop; sym++) {
-		if (get_secindex(elf, sym) != relsym_secindex)
-			continue;
-		if (ELF_ST_TYPE(sym->st_info) == STT_SECTION)
-			continue;
-		if (!is_valid_name(elf, sym))
-			continue;
-		if (sym->st_value == addr)
-			return sym;
-		/* Find a symbol nearby - addr are maybe negative */
-		d = sym->st_value - addr;
-		if (d < 0)
-			d = addr - sym->st_value;
-		if (d < distance) {
-			distance = d;
-			near = sym;
-		}
-	}
-	/* We need a close match */
-	if (distance < 20)
-		return near;
-	else
-		return NULL;
-}
-
-/*
- * Find symbols before or equal addr and after addr - in the section sec.
- * If we find two symbols with equal offset prefer one with a valid name.
- * The ELF format may have a better way to detect what type of symbol
- * it is, but this works for now.
- **/
-static Elf_Sym *find_fromsym(struct elf_info *elf, Elf_Addr addr,
-			     unsigned int secndx)
-{
-	Elf_Sym *sym;
-	Elf_Sym *near = NULL;
-	Elf_Addr distance = ~0;
+	Elf_Addr min_distance = ~0;
+	Elf_Addr distance;
 
 	for (sym = elf->symtab_start; sym < elf->symtab_stop; sym++) {
 		if (get_secindex(elf, sym) != secndx)
 			continue;
 		if (!is_valid_name(elf, sym))
 			continue;
-		if (sym->st_value <= addr && addr - sym->st_value <= distance) {
+
+		if (addr >= sym->st_value)
 			distance = addr - sym->st_value;
+		else if (allow_negative)
+			distance = sym->st_value - addr;
+		else
+			continue;
+
+		if (distance <= min_distance) {
+			min_distance = distance;
 			near = sym;
 		}
+
+		if (min_distance == 0)
+			break;
 	}
+
 	return near;
 }
 
+static Elf_Sym *find_fromsym(struct elf_info *elf, Elf_Addr addr,
+			     unsigned int secndx)
+{
+	return find_nearest_sym(elf, addr, secndx, false);
+}
+
+static Elf_Sym *find_tosym(struct elf_info *elf, Elf_Addr addr, Elf_Sym *sym)
+{
+	/* If the supplied symbol has a name, return it */
+	if (sym->st_name != 0)
+		return sym;
+
+	/*
+	 * Otherwise, look up the symbol.
+	 * For example, if the referenced symbol is static, sym->st_name is
+	 * zero. So, we need the look-up to know the symbol name.
+	 * The 'addr' might be smaller than the st_value of the symbol we are
+	 * looking for. Hence, allow_negative=true.
+	 */
+	return find_nearest_sym(elf, addr, get_secindex(elf, sym), true);
+}
+
 static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
 {
 	if (secndx > elf->num_sections)
-- 
2.39.2

[PATCH v5 12/21] modpost: unify 'sym' and 'to' in default_mismatch_handler()

[Masahiro Yamada]

find_tosym() takes 'sym' and stores the return value to another
variable 'to'. You can use the same variable because we want to
replace the original one when appropriate.

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index d147e8f63e52..7601f2ca28ed 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1183,11 +1183,10 @@ static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
 
 static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 				     const struct sectioncheck* const mismatch,
-				     Elf_Rela *r, Elf_Sym *sym,
+				     Elf_Rela *r, Elf_Sym *tsym,
 				     unsigned int fsecndx, const char *fromsec,
 				     const char *tosec)
 {
-	Elf_Sym *to;
 	Elf_Sym *from;
 	const char *tosym;
 	const char *fromsym;
@@ -1195,8 +1194,8 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 	from = find_fromsym(elf, r->r_offset, fsecndx);
 	fromsym = sym_name(elf, from);
 
-	to = find_tosym(elf, r->r_addend, sym);
-	tosym = sym_name(elf, to);
+	tsym = find_tosym(elf, r->r_addend, tsym);
+	tosym = sym_name(elf, tsym);
 
 	/* check whitelist - we may ignore it */
 	if (!secref_whitelist(mismatch, fromsec, fromsym, tosec, tosym))
@@ -1231,7 +1230,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 			      "You might get more information about where this is\n"
 			      "coming from by using scripts/check_extable.sh %s\n",
 			      fromsec, (long)r->r_offset, tosec, modname);
-		else if (is_executable_section(elf, get_secindex(elf, sym)))
+		else if (is_executable_section(elf, get_secindex(elf, tsym)))
 			warn("The relocation at %s+0x%lx references\n"
 			     "section \"%s\" which is not in the list of\n"
 			     "authorized sections.  If you're adding a new section\n"
-- 
2.39.2

[PATCH v5 13/21] modpost: replace r->r_offset, r->r_addend with faddr, taddr

[Masahiro Yamada]

r_offset/r_addend holds the offset address from/to which a symbol is
referenced. It is unclear unless you are familiar with ELF.

Rename them to faddr, taddr, respectively. The prefix 'f' means 'from',
't' means 'to'.

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

 scripts/mod/modpost.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 7601f2ca28ed..0ef9d6964b6a 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1183,18 +1183,18 @@ static bool is_executable_section(struct elf_info *elf, unsigned int secndx)
 
 static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 				     const struct sectioncheck* const mismatch,
-				     Elf_Rela *r, Elf_Sym *tsym,
-				     unsigned int fsecndx, const char *fromsec,
-				     const char *tosec)
+				     Elf_Sym *tsym,
+				     unsigned int fsecndx, const char *fromsec, Elf_Addr faddr,
+				     const char *tosec, Elf_Addr taddr)
 {
 	Elf_Sym *from;
 	const char *tosym;
 	const char *fromsym;
 
-	from = find_fromsym(elf, r->r_offset, fsecndx);
+	from = find_fromsym(elf, faddr, fsecndx);
 	fromsym = sym_name(elf, from);
 
-	tsym = find_tosym(elf, r->r_addend, tsym);
+	tsym = find_tosym(elf, taddr, tsym);
 	tosym = sym_name(elf, tsym);
 
 	/* check whitelist - we may ignore it */
@@ -1221,7 +1221,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 		break;
 	case EXTABLE_TO_NON_TEXT:
 		warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
-		     modname, fromsec, (long)r->r_offset, tosec, tosym);
+		     modname, fromsec, (long)faddr, tosec, tosym);
 
 		if (match(tosec, mismatch->bad_tosec))
 			fatal("The relocation at %s+0x%lx references\n"
@@ -1229,7 +1229,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 			      "Something is seriously wrong and should be fixed.\n"
 			      "You might get more information about where this is\n"
 			      "coming from by using scripts/check_extable.sh %s\n",
-			      fromsec, (long)r->r_offset, tosec, modname);
+			      fromsec, (long)faddr, tosec, modname);
 		else if (is_executable_section(elf, get_secindex(elf, tsym)))
 			warn("The relocation at %s+0x%lx references\n"
 			     "section \"%s\" which is not in the list of\n"
@@ -1238,17 +1238,18 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 			     "list of authorized sections to jump to on fault.\n"
 			     "This can be achieved by adding \"%s\" to\n"
 			     "OTHER_TEXT_SECTIONS in scripts/mod/modpost.c.\n",
-			     fromsec, (long)r->r_offset, tosec, tosec, tosec);
+			     fromsec, (long)faddr, tosec, tosec, tosec);
 		else
 			error("%s+0x%lx references non-executable section '%s'\n",
-			      fromsec, (long)r->r_offset, tosec);
+			      fromsec, (long)faddr, tosec);
 		break;
 	}
 }
 
 static void check_section_mismatch(const char *modname, struct elf_info *elf,
-				   Elf_Rela *r, Elf_Sym *sym,
-				   unsigned int fsecndx, const char *fromsec)
+				   Elf_Sym *sym,
+				   unsigned int fsecndx, const char *fromsec,
+				   Elf_Addr faddr, Elf_Addr taddr)
 {
 	const char *tosec = sec_name(elf, get_secindex(elf, sym));
 	const struct sectioncheck *mismatch = section_mismatch(fromsec, tosec);
@@ -1256,8 +1257,9 @@ static void check_section_mismatch(const char *modname, struct elf_info *elf,
 	if (!mismatch)
 		return;
 
-	default_mismatch_handler(modname, elf, mismatch, r, sym, fsecndx, fromsec,
-				 tosec);
+	default_mismatch_handler(modname, elf, mismatch, sym,
+				 fsecndx, fromsec, faddr,
+				 tosec, taddr);
 }
 
 static unsigned int *reloc_location(struct elf_info *elf,
@@ -1415,7 +1417,8 @@ static void section_rela(const char *modname, struct elf_info *elf,
 		/* Skip special sections */
 		if (is_shndx_special(sym->st_shndx))
 			continue;
-		check_section_mismatch(modname, elf, &r, sym, fsecndx, fromsec);
+		check_section_mismatch(modname, elf, sym,
+				       fsecndx, fromsec, r.r_offset, r.r_addend);
 	}
 }
 
@@ -1473,7 +1476,8 @@ static void section_rel(const char *modname, struct elf_info *elf,
 		/* Skip special sections */
 		if (is_shndx_special(sym->st_shndx))
 			continue;
-		check_section_mismatch(modname, elf, &r, sym, fsecndx, fromsec);
+		check_section_mismatch(modname, elf, sym,
+				       fsecndx, fromsec, r.r_offset, r.r_addend);
 	}
 }
 
-- 
2.39.2

[PATCH v5 14/21] modpost: remove is_shndx_special() check from section_rel(a)

[Masahiro Yamada]

This check is unneeded. Without it, sec_name() will returns the null
string "", then section_mismatch() will return immediately.

Anyway, special section indices do not appear quite often in these
loops.

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 0ef9d6964b6a..28db215ecc71 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1370,7 +1370,6 @@ static int addend_mips_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
 static void section_rela(const char *modname, struct elf_info *elf,
 			 Elf_Shdr *sechdr)
 {
-	Elf_Sym  *sym;
 	Elf_Rela *rela;
 	Elf_Rela r;
 	unsigned int r_sym;
@@ -1413,11 +1412,8 @@ static void section_rela(const char *modname, struct elf_info *elf,
 				continue;
 			break;
 		}
-		sym = elf->symtab_start + r_sym;
-		/* Skip special sections */
-		if (is_shndx_special(sym->st_shndx))
-			continue;
-		check_section_mismatch(modname, elf, sym,
+
+		check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
 				       fsecndx, fromsec, r.r_offset, r.r_addend);
 	}
 }
@@ -1425,7 +1421,6 @@ static void section_rela(const char *modname, struct elf_info *elf,
 static void section_rel(const char *modname, struct elf_info *elf,
 			Elf_Shdr *sechdr)
 {
-	Elf_Sym *sym;
 	Elf_Rel *rel;
 	Elf_Rela r;
 	unsigned int r_sym;
@@ -1472,11 +1467,8 @@ static void section_rel(const char *modname, struct elf_info *elf,
 		default:
 			fatal("Please add code to calculate addend for this architecture\n");
 		}
-		sym = elf->symtab_start + r_sym;
-		/* Skip special sections */
-		if (is_shndx_special(sym->st_shndx))
-			continue;
-		check_section_mismatch(modname, elf, sym,
+
+		check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
 				       fsecndx, fromsec, r.r_offset, r.r_addend);
 	}
 }
-- 
2.39.2

Re: [PATCH v5 14/21] modpost: remove is_shndx_special() check from section_rel(a)

[Nick Desaulniers]

On Sun, May 14, 2023 at 8:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> This check is unneeded. Without it, sec_name() will returns the null
> string "", then section_mismatch() will return immediately.
>
> Anyway, special section indices do not appear quite often in these
> loops.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Could almost eliminate is_shndx_special after this. I think there's
only one callsite after this? (I should check with the series applied;
this patch fails to apply on mainline if that's relevant for potential
merge conflicts).

Does doing that additional work speed things up though?

> ---
>
>  scripts/mod/modpost.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 0ef9d6964b6a..28db215ecc71 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1370,7 +1370,6 @@ static int addend_mips_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
>  static void section_rela(const char *modname, struct elf_info *elf,
>                          Elf_Shdr *sechdr)
>  {
> -       Elf_Sym  *sym;
>         Elf_Rela *rela;
>         Elf_Rela r;
>         unsigned int r_sym;
> @@ -1413,11 +1412,8 @@ static void section_rela(const char *modname, struct elf_info *elf,
>                                 continue;
>                         break;
>                 }
> -               sym = elf->symtab_start + r_sym;
> -               /* Skip special sections */
> -               if (is_shndx_special(sym->st_shndx))
> -                       continue;
> -               check_section_mismatch(modname, elf, sym,
> +
> +               check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
>                                        fsecndx, fromsec, r.r_offset, r.r_addend);
>         }
>  }
> @@ -1425,7 +1421,6 @@ static void section_rela(const char *modname, struct elf_info *elf,
>  static void section_rel(const char *modname, struct elf_info *elf,
>                         Elf_Shdr *sechdr)
>  {
> -       Elf_Sym *sym;
>         Elf_Rel *rel;
>         Elf_Rela r;
>         unsigned int r_sym;
> @@ -1472,11 +1467,8 @@ static void section_rel(const char *modname, struct elf_info *elf,
>                 default:
>                         fatal("Please add code to calculate addend for this architecture\n");
>                 }
> -               sym = elf->symtab_start + r_sym;
> -               /* Skip special sections */
> -               if (is_shndx_special(sym->st_shndx))
> -                       continue;
> -               check_section_mismatch(modname, elf, sym,
> +
> +               check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
>                                        fsecndx, fromsec, r.r_offset, r.r_addend);
>         }
>  }
> --
> 2.39.2
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v5 14/21] modpost: remove is_shndx_special() check from section_rel(a)

[Nick Desaulniers]

On Wed, May 17, 2023 at 2:23 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, May 14, 2023 at 8:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > This check is unneeded. Without it, sec_name() will returns the null
> > string "", then section_mismatch() will return immediately.
> >
> > Anyway, special section indices do not appear quite often in these
> > loops.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>
> Could almost eliminate is_shndx_special after this. I think there's
> only one callsite after this? (I should check with the series applied;
> this patch fails to apply on mainline if that's relevant for potential
> merge conflicts).
>
> Does doing that additional work speed things up though?

Either way,
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> > ---
> >
> >  scripts/mod/modpost.c | 16 ++++------------
> >  1 file changed, 4 insertions(+), 12 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 0ef9d6964b6a..28db215ecc71 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1370,7 +1370,6 @@ static int addend_mips_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
> >  static void section_rela(const char *modname, struct elf_info *elf,
> >                          Elf_Shdr *sechdr)
> >  {
> > -       Elf_Sym  *sym;
> >         Elf_Rela *rela;
> >         Elf_Rela r;
> >         unsigned int r_sym;
> > @@ -1413,11 +1412,8 @@ static void section_rela(const char *modname, struct elf_info *elf,
> >                                 continue;
> >                         break;
> >                 }
> > -               sym = elf->symtab_start + r_sym;
> > -               /* Skip special sections */
> > -               if (is_shndx_special(sym->st_shndx))
> > -                       continue;
> > -               check_section_mismatch(modname, elf, sym,
> > +
> > +               check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
> >                                        fsecndx, fromsec, r.r_offset, r.r_addend);
> >         }
> >  }
> > @@ -1425,7 +1421,6 @@ static void section_rela(const char *modname, struct elf_info *elf,
> >  static void section_rel(const char *modname, struct elf_info *elf,
> >                         Elf_Shdr *sechdr)
> >  {
> > -       Elf_Sym *sym;
> >         Elf_Rel *rel;
> >         Elf_Rela r;
> >         unsigned int r_sym;
> > @@ -1472,11 +1467,8 @@ static void section_rel(const char *modname, struct elf_info *elf,
> >                 default:
> >                         fatal("Please add code to calculate addend for this architecture\n");
> >                 }
> > -               sym = elf->symtab_start + r_sym;
> > -               /* Skip special sections */
> > -               if (is_shndx_special(sym->st_shndx))
> > -                       continue;
> > -               check_section_mismatch(modname, elf, sym,
> > +
> > +               check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
> >                                        fsecndx, fromsec, r.r_offset, r.r_addend);
> >         }
> >  }
> > --
> > 2.39.2
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v5 14/21] modpost: remove is_shndx_special() check from section_rel(a)

[Masahiro Yamada]

On Thu, May 18, 2023 at 6:23 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, May 14, 2023 at 8:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > This check is unneeded. Without it, sec_name() will returns the null
> > string "", then section_mismatch() will return immediately.
> >
> > Anyway, special section indices do not appear quite often in these
> > loops.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>
> Could almost eliminate is_shndx_special after this. I think there's
> only one callsite after this?

There will be no callsite of is_shndx_special().
I will remove the definition as well.



 (I should check with the series applied;
> this patch fails to apply on mainline if that's relevant for potential
> merge conflicts).
>
> Does doing that additional work speed things up though?


I do not know.

As I wrote in the comment, special indices rarely appear in
the relocation.
I do not believe "let's bail out earlier" does not buy us.



>
> > ---
> >
> >  scripts/mod/modpost.c | 16 ++++------------
> >  1 file changed, 4 insertions(+), 12 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 0ef9d6964b6a..28db215ecc71 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1370,7 +1370,6 @@ static int addend_mips_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
> >  static void section_rela(const char *modname, struct elf_info *elf,
> >                          Elf_Shdr *sechdr)
> >  {
> > -       Elf_Sym  *sym;
> >         Elf_Rela *rela;
> >         Elf_Rela r;
> >         unsigned int r_sym;
> > @@ -1413,11 +1412,8 @@ static void section_rela(const char *modname, struct elf_info *elf,
> >                                 continue;
> >                         break;
> >                 }
> > -               sym = elf->symtab_start + r_sym;
> > -               /* Skip special sections */
> > -               if (is_shndx_special(sym->st_shndx))
> > -                       continue;
> > -               check_section_mismatch(modname, elf, sym,
> > +
> > +               check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
> >                                        fsecndx, fromsec, r.r_offset, r.r_addend);
> >         }
> >  }
> > @@ -1425,7 +1421,6 @@ static void section_rela(const char *modname, struct elf_info *elf,
> >  static void section_rel(const char *modname, struct elf_info *elf,
> >                         Elf_Shdr *sechdr)
> >  {
> > -       Elf_Sym *sym;
> >         Elf_Rel *rel;
> >         Elf_Rela r;
> >         unsigned int r_sym;
> > @@ -1472,11 +1467,8 @@ static void section_rel(const char *modname, struct elf_info *elf,
> >                 default:
> >                         fatal("Please add code to calculate addend for this architecture\n");
> >                 }
> > -               sym = elf->symtab_start + r_sym;
> > -               /* Skip special sections */
> > -               if (is_shndx_special(sym->st_shndx))
> > -                       continue;
> > -               check_section_mismatch(modname, elf, sym,
> > +
> > +               check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
> >                                        fsecndx, fromsec, r.r_offset, r.r_addend);
> >         }
> >  }
> > --
> > 2.39.2
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Best Regards
Masahiro Yamada

[PATCH v5 15/21] modpost: pass struct module pointer to check_section_mismatch()

[Masahiro Yamada]

The next commit will use it.

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 28db215ecc71..23a352f52b8f 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1246,7 +1246,7 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 	}
 }
 
-static void check_section_mismatch(const char *modname, struct elf_info *elf,
+static void check_section_mismatch(struct module *mod, struct elf_info *elf,
 				   Elf_Sym *sym,
 				   unsigned int fsecndx, const char *fromsec,
 				   Elf_Addr faddr, Elf_Addr taddr)
@@ -1257,7 +1257,7 @@ static void check_section_mismatch(const char *modname, struct elf_info *elf,
 	if (!mismatch)
 		return;
 
-	default_mismatch_handler(modname, elf, mismatch, sym,
+	default_mismatch_handler(mod->name, elf, mismatch, sym,
 				 fsecndx, fromsec, faddr,
 				 tosec, taddr);
 }
@@ -1367,7 +1367,7 @@ static int addend_mips_rel(struct elf_info *elf, Elf_Shdr *sechdr, Elf_Rela *r)
 #define R_LARCH_SUB32		55
 #endif
 
-static void section_rela(const char *modname, struct elf_info *elf,
+static void section_rela(struct module *mod, struct elf_info *elf,
 			 Elf_Shdr *sechdr)
 {
 	Elf_Rela *rela;
@@ -1413,12 +1413,12 @@ static void section_rela(const char *modname, struct elf_info *elf,
 			break;
 		}
 
-		check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
+		check_section_mismatch(mod, elf, elf->symtab_start + r_sym,
 				       fsecndx, fromsec, r.r_offset, r.r_addend);
 	}
 }
 
-static void section_rel(const char *modname, struct elf_info *elf,
+static void section_rel(struct module *mod, struct elf_info *elf,
 			Elf_Shdr *sechdr)
 {
 	Elf_Rel *rel;
@@ -1468,7 +1468,7 @@ static void section_rel(const char *modname, struct elf_info *elf,
 			fatal("Please add code to calculate addend for this architecture\n");
 		}
 
-		check_section_mismatch(modname, elf, elf->symtab_start + r_sym,
+		check_section_mismatch(mod, elf, elf->symtab_start + r_sym,
 				       fsecndx, fromsec, r.r_offset, r.r_addend);
 	}
 }
@@ -1485,19 +1485,19 @@ static void section_rel(const char *modname, struct elf_info *elf,
  * to find all references to a section that reference a section that will
  * be discarded and warns about it.
  **/
-static void check_sec_ref(const char *modname, struct elf_info *elf)
+static void check_sec_ref(struct module *mod, struct elf_info *elf)
 {
 	int i;
 	Elf_Shdr *sechdrs = elf->sechdrs;
 
 	/* Walk through all sections */
 	for (i = 0; i < elf->num_sections; i++) {
-		check_section(modname, elf, &elf->sechdrs[i]);
+		check_section(mod->name, elf, &elf->sechdrs[i]);
 		/* We want to process only relocation sections and not .init */
 		if (sechdrs[i].sh_type == SHT_RELA)
-			section_rela(modname, elf, &elf->sechdrs[i]);
+			section_rela(mod, elf, &elf->sechdrs[i]);
 		else if (sechdrs[i].sh_type == SHT_REL)
-			section_rel(modname, elf, &elf->sechdrs[i]);
+			section_rel(mod, elf, &elf->sechdrs[i]);
 	}
 }
 
@@ -1668,7 +1668,7 @@ static void read_symbols(const char *modname)
 					     sym_get_data(&info, sym));
 	}
 
-	check_sec_ref(modname, &info);
+	check_sec_ref(mod, &info);
 
 	if (!mod->is_vmlinux) {
 		version = get_modinfo(&info, "version");
-- 
2.39.2

[PATCH v5 16/21] kbuild: generate KSYMTAB entries by modpost

[Masahiro Yamada]

Commit 7b4537199a4a ("kbuild: link symbol CRCs at final link, removing
CONFIG_MODULE_REL_CRCS") made modpost output CRCs in the same way
whether the EXPORT_SYMBOL() is placed in *.c or *.S.

This commit applies a similar approach to the entire data structure of
EXPORT_SYMBOL() for further cleanups. The EXPORT_SYMBOL() compilation
is split into two stages.

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

For example,

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

will be encoded into the following assembly code:

    .section ".export_symbol","a"
    __export_symbol__foo:
            .asciz ""
            .balign 8
            .quad foo
    .previous

    .section ".export_symbol","a"
    __export_symbol_gpl_bar:
            .asciz "BAR_NAMESPACE"
            .balign 8
            .quad bar
    .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 *(.export_symbol) will go into /DISCARD/ in the linker script.

Then, modpost extracts all the information about EXPORT_SYMBOL() from the
.export_symbol section, and generates C code:

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

KSYMTAB_FUNC() (or KSYMTAB_DATA() if it is data) 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
a symbol in *.S, EXPORT_SYMBOL() was placed in a separate *.c file.
arch/arm/kernel/armksyms.c is one example written in the classic manner.

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.

In the new approach, modpost checks symbol's type (STT_FUNC or not),
and outputs KSYMTAB_FUNC() or KSYMTAB_DATA() accordingly.

There are only two users of EXPORT_DATA_SYMBOL:

  EXPORT_DATA_SYMBOL_GPL(empty_zero_page)    (arch/ia64/kernel/head.S)
  EXPORT_DATA_SYMBOL(ia64_ivt)               (arch/ia64/kernel/ivt.S)

They are transformed as follows and output into .vmlinux.export.c

  KSYMTAB_DATA(empty_zero_page, "_gpl", "");
  KSYMTAB_DATA(ia64_ivt, "", "");

The other EXPORT_SYMBOL users in ia64 assembly are output as
KSYMTAB_FUNC().

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 implementation part to C, which supports
EXPORT_SYMBOL_NS() 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 with #include <linux/export.h>.

[3] Implement CONFIG_TRIM_UNUSED_KSYMS in one-pass algorithm (by a later commit)

When CONFIG_TRIM_UNUSED_KSYMS is enabled, Kbuild recursively traverses
the directory tree to determine which EXPORT_SYMBOL to trim. If an
EXPORT_SYMBOL turns out to be unused by anyone, Kbuild begins the
second traverse, where some source files are recompiled with their
EXPORT_SYMBOL() tuned into a no-op.

We can do this better now; modpost can selectively emit KSYMTAB entries
that are really used by modules.

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

Changes in v5:
  - Fix build error on ARM

Changes in v4:
  - Version 3 did not work if a same name symbol exists in a different compilation unit
    Fix it.

Changes in v3:
  - Move struct kernel_symbol to kernel/module/internal.h

Changes in v2:
  - Use KSYMTAB_FUNC and KSYMTAB_DATA for functions and data, respectively
    This distinction is needed for ia64.

 arch/ia64/include/asm/Kbuild      |   1 +
 arch/ia64/include/asm/export.h    |   3 -
 include/asm-generic/export.h      |  84 ++-----------------------
 include/asm-generic/vmlinux.lds.h |   1 +
 include/linux/export-internal.h   |  49 +++++++++++++++
 include/linux/export.h            |  98 +++++++++++------------------
 include/linux/pm.h                |   8 +--
 kernel/module/internal.h          |  12 ++++
 scripts/Makefile.build            |   8 +--
 scripts/check-local-export        |   4 +-
 scripts/mod/modpost.c             | 101 ++++++++++++++++++++----------
 scripts/mod/modpost.h             |   1 +
 12 files changed, 182 insertions(+), 188 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 aefae2efde9f..33733245f42b 100644
--- a/arch/ia64/include/asm/Kbuild
+++ b/arch/ia64/include/asm/Kbuild
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 generated-y += syscall_table.h
 generic-y += agp.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/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index d1f57e4868ed..e65d55e8819c 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -1006,6 +1006,7 @@
 	PATCHABLE_DISCARDS						\
 	*(.discard)							\
 	*(.discard.*)							\
+	*(.export_symbol)						\
 	*(.modinfo)							\
 	/* ld.bfd warns about .gnu.version* even when not emitted */	\
 	*(.gnu.version*)						\
diff --git a/include/linux/export-internal.h b/include/linux/export-internal.h
index fe7e6ba918f1..1c849db953a5 100644
--- a/include/linux/export-internal.h
+++ b/include/linux/export-internal.h
@@ -10,6 +10,55 @@
 #include <linux/compiler.h>
 #include <linux/types.h>
 
+#if defined(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)
+/*
+ * relative reference: 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 __KSYM_REF(sym)		".long " #sym "- ."
+#elif defined(CONFIG_64BIT)
+#define __KSYM_REF(sym)		".quad " #sym
+#else
+#define __KSYM_REF(sym)		".long " #sym
+#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(name, sym, sec, ns)						\
+	asm("	.section \"__ksymtab_strings\",\"aMS\",%progbits,1"	"\n"	\
+	    "__kstrtab_" #name ":"					"\n"	\
+	    "	.asciz \"" #name "\""					"\n"	\
+	    "__kstrtabns_" #name ":"					"\n"	\
+	    "	.asciz \"" ns "\""					"\n"	\
+	    "	.previous"						"\n"	\
+	    "	.section \"___ksymtab" sec "+" #name "\", \"a\""	"\n"	\
+	    "	.balign	4"						"\n"	\
+	    "__ksymtab_" #name ":"					"\n"	\
+		__KSYM_REF(sym)						"\n"	\
+		__KSYM_REF(__kstrtab_ ##name)				"\n"	\
+		__KSYM_REF(__kstrtabns_ ##name)				"\n"	\
+	    "	.previous"						"\n"	\
+	)
+
+#ifdef CONFIG_IA64
+#define KSYM_FUNC(name)		@fptr(name)
+#else
+#define KSYM_FUNC(name)		name
+#endif
+
+#define KSYMTAB_FUNC(name, sec, ns)	__KSYMTAB(name, KSYM_FUNC(name), sec, ns)
+#define KSYMTAB_DATA(name, sec, ns)	__KSYMTAB(name, name, sec, ns)
+
 #define SYMBOL_CRC(sym, crc, sec)   \
 	asm(".section \"___kcrctab" sec "+" #sym "\",\"a\""	"\n" \
 	    "__crc_" #sym ":"					"\n" \
diff --git a/include/linux/export.h b/include/linux/export.h
index 3f31ced0d977..32461a01608c 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,35 @@ extern struct module __this_module;
 #else
 #define THIS_MODULE ((struct module *)0)
 #endif
+#endif /* __ASSEMBLY__ */
 
-#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;
-};
+#ifdef CONFIG_64BIT
+#define __EXPORT_SYMBOL_REF(sym)	.balign 8; .quad sym
 #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;
-};
+#define __EXPORT_SYMBOL_REF(sym)	.balign 4; .long sym
 #endif
 
+#define ____EXPORT_SYMBOL(sym, license, ns)			\
+	.section ".export_symbol","a" ;				\
+	__export_symbol_##license##_##sym: ;			\
+	.asciz ns ;						\
+	__EXPORT_SYMBOL_REF(sym) ;				\
+	.previous
+
 #ifdef __GENKSYMS__
 
 #define ___EXPORT_SYMBOL(sym, sec, ns)	__GENKSYMS_EXPORT_SYMBOL(sym)
 
+#elif defined(__ASSEMBLY__)
+
+#define ___EXPORT_SYMBOL(sym, license, ns) \
+	____EXPORT_SYMBOL(sym, license, ns)
+
 #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, license, ns)			\
+	__ADDRESSABLE(sym)					\
+	asm(__stringify(____EXPORT_SYMBOL(sym, license, ns)))
 
 #endif
 
@@ -117,9 +81,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 +123,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/include/linux/pm.h b/include/linux/pm.h
index 035d9649eba4..aabb6bd8f89e 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -388,10 +388,10 @@ const struct dev_pm_ops name = { \
 #define EXPORT_PM_FN_NS_GPL(name, ns)
 #endif
 
-#define EXPORT_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(name, "", "")
-#define EXPORT_GPL_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(name, "_gpl", "")
-#define EXPORT_NS_DEV_PM_OPS(name, ns) _EXPORT_DEV_PM_OPS(name, "", #ns)
-#define EXPORT_NS_GPL_DEV_PM_OPS(name, ns) _EXPORT_DEV_PM_OPS(name, "_gpl", #ns)
+#define EXPORT_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(name,, "")
+#define EXPORT_GPL_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(name,gpl, "")
+#define EXPORT_NS_DEV_PM_OPS(name, ns) _EXPORT_DEV_PM_OPS(name,, #ns)
+#define EXPORT_NS_GPL_DEV_PM_OPS(name, ns) _EXPORT_DEV_PM_OPS(name,gpl, #ns)
 
 /*
  * Use this if you want to use the same suspend and resume callbacks for suspend
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index dc7b0160c480..c8b7b4dcf782 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -32,6 +32,18 @@
 /* Maximum number of characters written by module_flags() */
 #define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
 
+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/scripts/Makefile.build b/scripts/Makefile.build
index 9f94fc83f086..6bf026a304e4 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -161,7 +161,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.
@@ -169,7 +169,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
@@ -340,9 +340,7 @@ $(obj)/%.ll: $(src)/%.rs 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 f90b5a9c67b3..969a313b9299 100755
--- a/scripts/check-local-export
+++ b/scripts/check-local-export
@@ -46,9 +46,9 @@ BEGIN {
 { symbol_types[$3]=$2 }
 
 # append the exported symbol to the array
-($3 ~ /^__ksymtab_/) {
+($3 ~ /^__export_symbol_(gpl)?_.*/) {
 	export_symbols[i] = $3
-	sub(/^__ksymtab_/, "", export_symbols[i])
+	sub(/^__export_symbol_(gpl)?_/, "", export_symbols[i])
 	i++
 }
 
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 23a352f52b8f..921a61fea0b4 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -215,6 +215,7 @@ struct symbol {
 	unsigned int crc;
 	bool crc_valid;
 	bool weak;
+	bool is_func;
 	bool is_gpl_only;	/* exported by EXPORT_SYMBOL_GPL */
 	char name[];
 };
@@ -531,6 +532,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, ".export_symbol")) {
+			info->export_symbol_secndx = i;
 		}
 
 		if (sechdrs[i].sh_type == SHT_SYMTAB) {
@@ -653,18 +656,6 @@ 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;
-
-			name = symname + strlen("__ksymtab_");
-			secname = sec_name(info, get_secindex(info, sym));
-
-			if (strstarts(secname, "___ksymtab_gpl+"))
-				sym_add_exported(name, mod, true);
-			else if (strstarts(secname, "___ksymtab+"))
-				sym_add_exported(name, mod, false);
-		}
 		if (strcmp(symname, "init_module") == 0)
 			mod->has_init = true;
 		if (strcmp(symname, "cleanup_module") == 0)
@@ -865,7 +856,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,
 };
 
@@ -947,12 +937,6 @@ static const struct sectioncheck sectioncheck[] = {
 	.bad_tosec = { INIT_SECTIONS, NULL },
 	.mismatch = ANY_INIT_TO_ANY_EXIT,
 },
-/* Do not export init/exit functions or data */
-{
-	.fromsec = { "___ksymtab*", NULL },
-	.bad_tosec = { INIT_SECTIONS, EXIT_SECTIONS, NULL },
-	.mismatch = EXPORT_TO_INIT_EXIT,
-},
 {
 	.fromsec = { "__ex_table", NULL },
 	/* If you're adding any new black-listed sections in here, consider
@@ -1215,10 +1199,6 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 		warn("%s: section mismatch in reference: %s (section: %s) -> %s (section: %s)\n",
 		     modname, fromsym, fromsec, tosym, tosec);
 		break;
-	case EXPORT_TO_INIT_EXIT:
-		warn("%s: EXPORT_SYMBOL used for init/exit symbol: %s (section: %s)\n",
-		     modname, tosym, tosec);
-		break;
 	case EXTABLE_TO_NON_TEXT:
 		warn("%s(%s+0x%lx): Section mismatch in reference to the %s:%s\n",
 		     modname, fromsec, (long)faddr, tosec, tosym);
@@ -1246,14 +1226,70 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
 	}
 }
 
+static void check_export_symbol(struct module *mod, struct elf_info *elf,
+				Elf_Addr faddr, const char *secname,
+				Elf_Addr addr, Elf_Sym *sym)
+{
+	const char *label_name, *name, *prefix;
+	Elf_Sym *label;
+	struct symbol *s;
+	bool is_gpl;
+
+	label = find_fromsym(elf, faddr, elf->export_symbol_secndx);
+	label_name = sym_name(elf, label);
+
+	sym = find_tosym(elf, addr, sym);
+	name = sym_name(elf, sym);
+
+	if (strstarts(label_name, "__export_symbol_gpl_")) {
+		prefix = "__export_symbol_gpl_";
+		is_gpl = true;
+	} else if (strstarts(label_name, "__export_symbol__")) {
+		prefix = "__export_symbol__";
+		is_gpl = false;
+	} else {
+		error("%s: .export_symbol section contains strange symbol '%s'\n",
+		      mod->name, label_name);
+		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);
+		return;
+	}
+
+	s = sym_add_exported(name, mod, is_gpl);
+	sym_update_namespace(name, sym_get_data(elf, label));
+
+	/*
+	 * We need to be aware whether we are exporting a function or
+	 * a data on some architectures.
+	 */
+	s->is_func = (ELF_ST_TYPE(sym->st_info) == STT_FUNC);
+
+	if (match(secname, PATTERNS(INIT_SECTIONS)))
+		error("%s: %s: EXPORT_SYMBOL used for init symbol. Remove __init or EXPORT_SYMBOL.\n",
+		      mod->name, name);
+	else if (match(secname, PATTERNS(EXIT_SECTIONS)))
+		error("%s: %s: EXPORT_SYMBOL used for exit symbol. Remove __exit or EXPORT_SYMBOL.\n",
+		      mod->name, name);
+}
+
 static void check_section_mismatch(struct module *mod, struct elf_info *elf,
 				   Elf_Sym *sym,
 				   unsigned int fsecndx, const char *fromsec,
 				   Elf_Addr faddr, Elf_Addr taddr)
 {
 	const char *tosec = sec_name(elf, get_secindex(elf, sym));
-	const struct sectioncheck *mismatch = section_mismatch(fromsec, tosec);
+	const struct sectioncheck *mismatch;
 
+	if (elf->export_symbol_secndx == fsecndx) {
+		check_export_symbol(mod, elf, faddr, tosec, taddr, sym);
+		return;
+	}
+
+	mismatch = section_mismatch(fromsec, tosec);
 	if (!mismatch)
 		return;
 
@@ -1659,15 +1695,6 @@ 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(mod, &info);
 
 	if (!mod->is_vmlinux) {
@@ -1851,6 +1878,14 @@ 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_%s(%s, \"%s\", \"%s\");\n",
+			   sym->is_func ? "FUNC" : "DATA", 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 1178f40a73f3..f518fbe46541 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -137,6 +137,7 @@ struct elf_info {
 	Elf_Shdr     *sechdrs;
 	Elf_Sym      *symtab_start;
 	Elf_Sym      *symtab_stop;
+	unsigned int export_symbol_secndx;	/* .export_symbol section */
 	char         *strtab;
 	char	     *modinfo;
 	unsigned int modinfo_len;
-- 
2.39.2

Re: [PATCH v5 16/21] kbuild: generate KSYMTAB entries by modpost

[Greg Ungerer]

Hi Masahiro,

On 15/5/23 01:27, Masahiro Yamada wrote:
> Commit 7b4537199a4a ("kbuild: link symbol CRCs at final link, removing
> CONFIG_MODULE_REL_CRCS") made modpost output CRCs in the same way
> whether the EXPORT_SYMBOL() is placed in *.c or *.S.
> 
> This commit applies a similar approach to the entire data structure of
> EXPORT_SYMBOL() for further cleanups. The EXPORT_SYMBOL() compilation
> is split into two stages.
> 
> When a source file is compiled, EXPORT_SYMBOL() is converted into a
> dummy symbol in the .export_symbol section.
> 
> For example,
> 
>      EXPORT_SYMBOL(foo);
>      EXPORT_SYMBOL_NS_GPL(bar, BAR_NAMESPACE);
> 
> will be encoded into the following assembly code:
> 
>      .section ".export_symbol","a"
>      __export_symbol__foo:
>              .asciz ""
>              .balign 8
>              .quad foo
>      .previous
> 
>      .section ".export_symbol","a"
>      __export_symbol_gpl_bar:
>              .asciz "BAR_NAMESPACE"
>              .balign 8
>              .quad bar
>      .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 *(.export_symbol) will go into /DISCARD/ in the linker script.
> 
> Then, modpost extracts all the information about EXPORT_SYMBOL() from the
> .export_symbol section, and generates C code:
> 
>      KSYMTAB_FUNC(foo, "", "");
>      KSYMTAB_FUNC(bar, "_gpl", "BAR_NAMESPACE");
> 
> KSYMTAB_FUNC() (or KSYMTAB_DATA() if it is data) 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
> a symbol in *.S, EXPORT_SYMBOL() was placed in a separate *.c file.
> arch/arm/kernel/armksyms.c is one example written in the classic manner.
> 
> 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.
> 
> In the new approach, modpost checks symbol's type (STT_FUNC or not),
> and outputs KSYMTAB_FUNC() or KSYMTAB_DATA() accordingly.
> 
> There are only two users of EXPORT_DATA_SYMBOL:
> 
>    EXPORT_DATA_SYMBOL_GPL(empty_zero_page)    (arch/ia64/kernel/head.S)
>    EXPORT_DATA_SYMBOL(ia64_ivt)               (arch/ia64/kernel/ivt.S)
> 
> They are transformed as follows and output into .vmlinux.export.c
> 
>    KSYMTAB_DATA(empty_zero_page, "_gpl", "");
>    KSYMTAB_DATA(ia64_ivt, "", "");
> 
> The other EXPORT_SYMBOL users in ia64 assembly are output as
> KSYMTAB_FUNC().
> 
> 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 implementation part to C, which supports
> EXPORT_SYMBOL_NS() 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 with #include <linux/export.h>.
> 
> [3] Implement CONFIG_TRIM_UNUSED_KSYMS in one-pass algorithm (by a later commit)
> 
> When CONFIG_TRIM_UNUSED_KSYMS is enabled, Kbuild recursively traverses
> the directory tree to determine which EXPORT_SYMBOL to trim. If an
> EXPORT_SYMBOL turns out to be unused by anyone, Kbuild begins the
> second traverse, where some source files are recompiled with their
> EXPORT_SYMBOL() tuned into a no-op.
> 
> We can do this better now; modpost can selectively emit KSYMTAB entries
> that are really used by modules.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

This breaks building kernels with an m68k-uclinux-gcc toolchain that have
modules configured. Before this change they built and ran fine.
They build and run fine if CONFIG_MODULES is not set.

A few hundred errors like this spew out:

     scripts/mod/modpost -o Module.symvers -T modules.order vmlinux.o
     ERROR: modpost: vmlinux: .export_symbol section references '', but it does not seem to be an export symbol
     ERROR: modpost: vmlinux: .export_symbol section references '', but it does not seem to be an export symbol
     ERROR: modpost: vmlinux: .export_symbol section references '', but it does not seem to be an export symbol
     ...

This is still broken all the way through to the current 6.6-rc7, though the error
messages are slightly better:

     ERROR: modpost: vmlinux: local symbol 'system_state' was exported
     ERROR: modpost: vmlinux: local symbol 'static_key_initialized' was exported
     ERROR: modpost: vmlinux: local symbol 'reset_devices' was exported
     ...

I tried a bunch of different binutils and gcc versions (binutils-2.251 through
2.40 and gcc versions 8.3.0 through 12.3.0). If I compile with an m68k-linux
targeted toolchain then it works - no modpost processing problems.

nm reports the same information for symbols in both cases, eg:

$ m68k-uclinux-nm vmlinux.o | grep system_state
00000000 r __export_symbol_system_state
00000008 B system_state
0000000c d __UNIQUE_ID___addressable_system_state320

$ m68k-linux-nm vmlinux.o | grep system_state
00000000 r __export_symbol_system_state
00000008 B system_state
0000000c d __UNIQUE_ID___addressable_system_state320

Tracing in scripts/mod/modpost.c I see that for this symbol example
("system_state") that ELF_ST_BIND(sym->st_info) is 0x0 for the
m68k-uclinux toolchain case, so STB_LOCAL, whereas for the m68k-linux
case it is 0x1, so STB_GLOBAL.

Any idea what is going on here?

Regards
Greg

Re: [PATCH v5 16/21] kbuild: generate KSYMTAB entries by modpost

[Masahiro Yamada]

On Fri, Oct 27, 2023 at 1:30 PM Greg Ungerer <gregungerer@westnet.com.au> wrote:
>
> Hi Masahiro,
>
> On 15/5/23 01:27, Masahiro Yamada wrote:
> > Commit 7b4537199a4a ("kbuild: link symbol CRCs at final link, removing
> > CONFIG_MODULE_REL_CRCS") made modpost output CRCs in the same way
> > whether the EXPORT_SYMBOL() is placed in *.c or *.S.
> >
> > This commit applies a similar approach to the entire data structure of
> > EXPORT_SYMBOL() for further cleanups. The EXPORT_SYMBOL() compilation
> > is split into two stages.
> >
> > When a source file is compiled, EXPORT_SYMBOL() is converted into a
> > dummy symbol in the .export_symbol section.
> >
> > For example,
> >
> >      EXPORT_SYMBOL(foo);
> >      EXPORT_SYMBOL_NS_GPL(bar, BAR_NAMESPACE);
> >
> > will be encoded into the following assembly code:
> >
> >      .section ".export_symbol","a"
> >      __export_symbol__foo:
> >              .asciz ""
> >              .balign 8
> >              .quad foo
> >      .previous
> >
> >      .section ".export_symbol","a"
> >      __export_symbol_gpl_bar:
> >              .asciz "BAR_NAMESPACE"
> >              .balign 8
> >              .quad bar
> >      .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 *(.export_symbol) will go into /DISCARD/ in the linker script.
> >
> > Then, modpost extracts all the information about EXPORT_SYMBOL() from the
> > .export_symbol section, and generates C code:
> >
> >      KSYMTAB_FUNC(foo, "", "");
> >      KSYMTAB_FUNC(bar, "_gpl", "BAR_NAMESPACE");
> >
> > KSYMTAB_FUNC() (or KSYMTAB_DATA() if it is data) 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
> > a symbol in *.S, EXPORT_SYMBOL() was placed in a separate *.c file.
> > arch/arm/kernel/armksyms.c is one example written in the classic manner.
> >
> > 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.
> >
> > In the new approach, modpost checks symbol's type (STT_FUNC or not),
> > and outputs KSYMTAB_FUNC() or KSYMTAB_DATA() accordingly.
> >
> > There are only two users of EXPORT_DATA_SYMBOL:
> >
> >    EXPORT_DATA_SYMBOL_GPL(empty_zero_page)    (arch/ia64/kernel/head.S)
> >    EXPORT_DATA_SYMBOL(ia64_ivt)               (arch/ia64/kernel/ivt.S)
> >
> > They are transformed as follows and output into .vmlinux.export.c
> >
> >    KSYMTAB_DATA(empty_zero_page, "_gpl", "");
> >    KSYMTAB_DATA(ia64_ivt, "", "");
> >
> > The other EXPORT_SYMBOL users in ia64 assembly are output as
> > KSYMTAB_FUNC().
> >
> > 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 implementation part to C, which supports
> > EXPORT_SYMBOL_NS() 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 with #include <linux/export.h>.
> >
> > [3] Implement CONFIG_TRIM_UNUSED_KSYMS in one-pass algorithm (by a later commit)
> >
> > When CONFIG_TRIM_UNUSED_KSYMS is enabled, Kbuild recursively traverses
> > the directory tree to determine which EXPORT_SYMBOL to trim. If an
> > EXPORT_SYMBOL turns out to be unused by anyone, Kbuild begins the
> > second traverse, where some source files are recompiled with their
> > EXPORT_SYMBOL() tuned into a no-op.
> >
> > We can do this better now; modpost can selectively emit KSYMTAB entries
> > that are really used by modules.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>
> This breaks building kernels with an m68k-uclinux-gcc toolchain that have
> modules configured. Before this change they built and ran fine.
> They build and run fine if CONFIG_MODULES is not set.
>
> A few hundred errors like this spew out:
>
>      scripts/mod/modpost -o Module.symvers -T modules.order vmlinux.o
>      ERROR: modpost: vmlinux: .export_symbol section references '', but it does not seem to be an export symbol
>      ERROR: modpost: vmlinux: .export_symbol section references '', but it does not seem to be an export symbol
>      ERROR: modpost: vmlinux: .export_symbol section references '', but it does not seem to be an export symbol
>      ...



Where can I download a prebuilt m68k-uclinux-gcc?









--
Best Regards
Masahiro Yamada

[PATCH v5 17/21] ia64,export.h: replace EXPORT_DATA_SYMBOL* with EXPORT_SYMBOL*

[Masahiro Yamada]

With the previous refactoring, you can always use EXPORT_SYMBOL*.

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.39.2

[PATCH v5 18/21] modpost: check static EXPORT_SYMBOL* by modpost again

[Masahiro Yamada]

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@kernel.org>
---

 scripts/Makefile.build     |  4 ---
 scripts/check-local-export | 70 --------------------------------------
 scripts/mod/modpost.c      |  6 ++++
 3 files changed, 6 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@kernel.org>
-# Copyright (C) 2022 Owen Rafferty <owen@owenrafferty.com>
-#
-# 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 921a61fea0b4..5db6a6f76971 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1259,6 +1259,12 @@ 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, name);
+		return;
+	}
+
 	s = sym_add_exported(name, mod, is_gpl);
 	sym_update_namespace(name, sym_get_data(elf, label));
 
-- 
2.39.2

[PATCH v5 19/21] modpost: squash 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>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 5db6a6f76971..7edb9a843ead 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -353,26 +353,8 @@ static const char *sec_name(const struct elf_info *info, unsigned int secindex)
 
 #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);
 
@@ -385,6 +367,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);
 
@@ -1265,8 +1248,7 @@ static void check_export_symbol(struct module *mod, struct elf_info *elf,
 		return;
 	}
 
-	s = sym_add_exported(name, mod, is_gpl);
-	sym_update_namespace(name, sym_get_data(elf, label));
+	s = sym_add_exported(name, mod, is_gpl, sym_get_data(elf, label));
 
 	/*
 	 * We need to be aware whether we are exporting a function or
@@ -2135,9 +2117,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.39.2

[PATCH v5 20/21] 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>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 7edb9a843ead..010c4f88d77b 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -298,6 +298,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;
@@ -367,7 +374,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);
 
@@ -1784,8 +1791,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);
@@ -1871,8 +1877,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_%s(%s, \"%s\", \"%s\");\n",
 			   sym->is_func ? "FUNC" : "DATA", sym->name,
-			   sym->is_gpl_only ? "_gpl" : "",
-			   sym->namespace ?: "");
+			   sym->is_gpl_only ? "_gpl" : "", sym->namespace);
 
 	if (!modversions)
 		return;
@@ -2140,7 +2145,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.39.2

[PATCH v5 21/21] kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion

[Masahiro Yamada]

When CONFIG_TRIM_UNUSED_KSYMS is enabled, Kbuild recursively traverses
the directory tree to determine which EXPORT_SYMBOL to trim. If an
EXPORT_SYMBOL turns out to be unused by anyone, Kbuild begins the
second traverse, where some source files are recompiled with their
EXPORT_SYMBOL() tuned into a no-op.

Linus stated negative opinions about this slowness in commits:

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

We can do this better now. The final data structures of EXPORT_SYMBOL
are generated by the modpost stage, so modpost can selectively emit
KSYMTAB entries that are really used by modules.

Commit 2cce989f8461 ("kbuild: unify two modpost invocations") is another
ground-work to do this in a one-pass algorithm. With the list of modules,
modpost sets sym->used if it is used by a module. modpost emits KSYMTAB
only for symbols with sym->used==true.

BTW, 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 I guess
that is unlikely to happen.

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

Changes in v5:
  - Clean up more

 .gitignore                  |  1 -
 Makefile                    | 19 +---------
 include/linux/export.h      | 65 +++++----------------------------
 scripts/Makefile.build      |  7 ----
 scripts/Makefile.modpost    |  7 ++++
 scripts/adjust_autoksyms.sh | 73 -------------------------------------
 scripts/basic/fixdep.c      |  3 +-
 scripts/gen_ksymdeps.sh     | 30 ---------------
 scripts/mod/modpost.c       | 54 ++++++++++++++++++++++++---
 scripts/remove-stale-files  |  2 +
 10 files changed, 70 insertions(+), 191 deletions(-)
 delete mode 100755 scripts/adjust_autoksyms.sh
 delete mode 100755 scripts/gen_ksymdeps.sh

diff --git a/.gitignore b/.gitignore
index 7f86e0837909..172e3874adfd 100644
--- a/.gitignore
+++ b/.gitignore
@@ -112,7 +112,6 @@ modules.order
 #
 /include/config/
 /include/generated/
-/include/ksym/
 /arch/*/include/generated/
 
 # stgit generated dirs
diff --git a/Makefile b/Makefile
index 9d765ebcccf1..2a081f2e9911 100644
--- a/Makefile
+++ b/Makefile
@@ -1193,28 +1193,13 @@ endif
 export KBUILD_VMLINUX_LIBS
 export KBUILD_LDS          := arch/$(SRCARCH)/kernel/vmlinux.lds
 
-# Recurse until adjust_autoksyms.sh is satisfied
-PHONY += autoksyms_recursive
 ifdef CONFIG_TRIM_UNUSED_KSYMS
 # For the kernel to actually contain only the needed exported symbols,
 # we have to build modules as well to determine what those symbols are.
 # (this can be evaluated only once include/config/auto.conf has been included)
 KBUILD_MODULES := 1
-
-autoksyms_recursive: $(build-dir) modules.order
-	$(Q)$(CONFIG_SHELL) $(srctree)/scripts/adjust_autoksyms.sh \
-	  "$(MAKE) -f $(srctree)/Makefile autoksyms_recursive"
 endif
 
-autoksyms_h := $(if $(CONFIG_TRIM_UNUSED_KSYMS), include/generated/autoksyms.h)
-
-quiet_cmd_autoksyms_h = GEN     $@
-      cmd_autoksyms_h = mkdir -p $(dir $@); \
-			$(CONFIG_SHELL) $(srctree)/scripts/gen_autoksyms.sh $@
-
-$(autoksyms_h):
-	$(call cmd,autoksyms_h)
-
 # '$(AR) mPi' needs 'T' to workaround the bug of llvm-ar <= 14
 quiet_cmd_ar_vmlinux.a = AR      $@
       cmd_ar_vmlinux.a = \
@@ -1223,7 +1208,7 @@ quiet_cmd_ar_vmlinux.a = AR      $@
 	$(AR) mPiT $$($(AR) t $@ | sed -n 1p) $@ $$($(AR) t $@ | grep -F -f $(srctree)/scripts/head-object-list.txt)
 
 targets += vmlinux.a
-vmlinux.a: $(KBUILD_VMLINUX_OBJS) scripts/head-object-list.txt autoksyms_recursive FORCE
+vmlinux.a: $(KBUILD_VMLINUX_OBJS) scripts/head-object-list.txt FORCE
 	$(call if_changed,ar_vmlinux.a)
 
 PHONY += vmlinux_o
@@ -1279,7 +1264,7 @@ scripts: scripts_basic scripts_dtc
 PHONY += prepare archprepare
 
 archprepare: outputmakefile archheaders archscripts scripts include/config/kernel.release \
-	asm-generic $(version_h) $(autoksyms_h) include/generated/utsrelease.h \
+	asm-generic $(version_h) include/generated/utsrelease.h \
 	include/generated/compile.h include/generated/autoconf.h remove-stale-files
 
 prepare0: archprepare
diff --git a/include/linux/export.h b/include/linux/export.h
index 32461a01608c..9bf081ff9903 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -37,30 +37,13 @@ extern struct module __this_module;
 #define __EXPORT_SYMBOL_REF(sym)	.balign 4; .long sym
 #endif
 
-#define ____EXPORT_SYMBOL(sym, license, ns)			\
+#define ___EXPORT_SYMBOL(sym, license, ns)			\
 	.section ".export_symbol","a" ;				\
 	__export_symbol_##license##_##sym: ;			\
 	.asciz ns ;						\
 	__EXPORT_SYMBOL_REF(sym) ;				\
 	.previous
 
-#ifdef __GENKSYMS__
-
-#define ___EXPORT_SYMBOL(sym, sec, ns)	__GENKSYMS_EXPORT_SYMBOL(sym)
-
-#elif defined(__ASSEMBLY__)
-
-#define ___EXPORT_SYMBOL(sym, license, ns) \
-	____EXPORT_SYMBOL(sym, license, ns)
-
-#else
-
-#define ___EXPORT_SYMBOL(sym, license, ns)			\
-	__ADDRESSABLE(sym)					\
-	asm(__stringify(____EXPORT_SYMBOL(sym, license, ns)))
-
-#endif
-
 #if !defined(CONFIG_MODULES) || defined(__DISABLE_EXPORTS)
 
 /*
@@ -70,50 +53,20 @@ extern struct module __this_module;
  */
 #define __EXPORT_SYMBOL(sym, sec, ns)
 
-#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
+#elif defined(__GENKSYMS__)
 
-#include <generated/autoksyms.h>
+#define __EXPORT_SYMBOL(sym, sec, ns)	__GENKSYMS_EXPORT_SYMBOL(sym)
 
-/*
- * For fine grained build dependencies, we want to tell the build system
- * about each possible exported symbol even if they're not actually exported.
- * We use a symbol pattern __ksym_marker_<symbol> that the build system filters
- * from the $(NM) output (see scripts/gen_ksymdeps.sh). These symbols are
- * discarded in the final link stage.
- */
+#elif defined(__ASSEMBLY__)
 
-#ifdef __ASSEMBLY__
-
-#define __ksym_marker(sym)					\
-	.section ".discard.ksym","a" ;				\
-__ksym_marker_##sym: ;						\
-	.previous
+#define __EXPORT_SYMBOL(sym, license, ns) \
+	___EXPORT_SYMBOL(sym, license, ns)
 
 #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))
-#define __cond_export_sym(sym, sec, ns, conf)				\
-	___cond_export_sym(sym, sec, ns, conf)
-#define ___cond_export_sym(sym, sec, ns, enabled)			\
-	__cond_export_sym_##enabled(sym, sec, ns)
-#define __cond_export_sym_1(sym, sec, ns) ___EXPORT_SYMBOL(sym, sec, ns)
-
-#ifdef __GENKSYMS__
-#define __cond_export_sym_0(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
-#else
-#define __cond_export_sym_0(sym, sec, ns) /* nothing */
-#endif
-
-#else
-
-#define __EXPORT_SYMBOL(sym, sec, ns)	___EXPORT_SYMBOL(sym, sec, ns)
+#define __EXPORT_SYMBOL(sym, license, ns)			\
+	__ADDRESSABLE(sym)					\
+	asm(__stringify(___EXPORT_SYMBOL(sym, license, ns)))
 
 #endif /* CONFIG_MODULES */
 
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index bd4123795299..8154bd962eea 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -215,18 +215,12 @@ is-standard-object = $(if $(filter-out y%, $(OBJECT_FILES_NON_STANDARD_$(basetar
 
 $(obj)/%.o: objtool-enabled = $(if $(is-standard-object),$(if $(delay-objtool),$(is-single-obj-m),y))
 
-ifdef CONFIG_TRIM_UNUSED_KSYMS
-cmd_gen_ksymdeps = \
-	$(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ >> $(dot-target).cmd
-endif
-
 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
 
 define rule_cc_o_c
 	$(call cmd_and_fixdep,cc_o_c)
-	$(call cmd,gen_ksymdeps)
 	$(call cmd,checksrc)
 	$(call cmd,checkdoc)
 	$(call cmd,gen_objtooldep)
@@ -237,7 +231,6 @@ endef
 
 define rule_as_o_S
 	$(call cmd_and_fixdep,as_o_S)
-	$(call cmd,gen_ksymdeps)
 	$(call cmd,gen_objtooldep)
 	$(call cmd,gen_symversions_S)
 	$(call cmd,warn_shared_object)
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 0980c58d8afc..1e0b47cbabd9 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -90,6 +90,13 @@ targets += .vmlinux.objs
 .vmlinux.objs: vmlinux.a $(KBUILD_VMLINUX_LIBS) FORCE
 	$(call if_changed,vmlinux_objs)
 
+ifdef CONFIG_TRIM_UNUSED_KSYMS
+ksym-wl := $(CONFIG_UNUSED_KSYMS_WHITELIST)
+ksym-wl := $(if $(filter-out /%, $(ksym-wl)),$(srctree)/)$(ksym-wl)
+modpost-args += -t $(addprefix -W, $(ksym-wl))
+modpost-deps += $(ksym-wl)
+endif
+
 ifeq ($(wildcard vmlinux.o),)
 missing-input := vmlinux.o
 output-symdump := modules-only.symvers
diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
deleted file mode 100755
index f1b5ac818411..000000000000
--- a/scripts/adjust_autoksyms.sh
+++ /dev/null
@@ -1,73 +0,0 @@
-#!/bin/sh
-# SPDX-License-Identifier: GPL-2.0-only
-
-# Script to update include/generated/autoksyms.h and dependency files
-#
-# Copyright:	(C) 2016  Linaro Limited
-# Created by:	Nicolas Pitre, January 2016
-#
-
-# Update the include/generated/autoksyms.h file.
-#
-# For each symbol being added or removed, the corresponding dependency
-# file's timestamp is updated to force a rebuild of the affected source
-# file. All arguments passed to this script are assumed to be a command
-# to be exec'd to trigger a rebuild of those files.
-
-set -e
-
-cur_ksyms_file="include/generated/autoksyms.h"
-new_ksyms_file="include/generated/autoksyms.h.tmpnew"
-
-info() {
-	if [ "$quiet" != "silent_" ]; then
-		printf "  %-7s %s\n" "$1" "$2"
-	fi
-}
-
-info "CHK" "$cur_ksyms_file"
-
-# Use "make V=1" to debug this script.
-case "$KBUILD_VERBOSE" in
-*1*)
-	set -x
-	;;
-esac
-
-# Generate a new symbol list file
-$CONFIG_SHELL $srctree/scripts/gen_autoksyms.sh --modorder "$new_ksyms_file"
-
-# Extract changes between old and new list and touch corresponding
-# dependency files.
-changed=$(
-count=0
-sort "$cur_ksyms_file" "$new_ksyms_file" | uniq -u |
-sed -n 's/^#define __KSYM_\(.*\) 1/\1/p' |
-while read sympath; do
-	if [ -z "$sympath" ]; then continue; fi
-	depfile="include/ksym/${sympath}"
-	mkdir -p "$(dirname "$depfile")"
-	touch "$depfile"
-	# Filesystems with coarse time precision may create timestamps
-	# equal to the one from a file that was very recently built and that
-	# needs to be rebuild. Let's guard against that by making sure our
-	# dep files are always newer than the first file we created here.
-	while [ ! "$depfile" -nt "$new_ksyms_file" ]; do
-		touch "$depfile"
-	done
-	echo $((count += 1))
-done | tail -1 )
-changed=${changed:-0}
-
-if [ $changed -gt 0 ]; then
-	# Replace the old list with tne new one
-	old=$(grep -c "^#define __KSYM_" "$cur_ksyms_file" || true)
-	new=$(grep -c "^#define __KSYM_" "$new_ksyms_file" || true)
-	info "KSYMS" "symbols: before=$old, after=$new, changed=$changed"
-	info "UPD" "$cur_ksyms_file"
-	mv -f "$new_ksyms_file" "$cur_ksyms_file"
-	# Then trigger a rebuild of affected source files
-	exec $@
-else
-	rm -f "$new_ksyms_file"
-fi
diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index fa562806c2be..84b6efa849f4 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -246,8 +246,7 @@ static void *read_file(const char *filename)
 /* Ignore certain dependencies */
 static int is_ignored_file(const char *s, int len)
 {
-	return str_ends_with(s, len, "include/generated/autoconf.h") ||
-	       str_ends_with(s, len, "include/generated/autoksyms.h");
+	return str_ends_with(s, len, "include/generated/autoconf.h");
 }
 
 /* Do not parse these files */
diff --git a/scripts/gen_ksymdeps.sh b/scripts/gen_ksymdeps.sh
deleted file mode 100755
index 8ee533f33659..000000000000
--- a/scripts/gen_ksymdeps.sh
+++ /dev/null
@@ -1,30 +0,0 @@
-#!/bin/sh
-# SPDX-License-Identifier: GPL-2.0
-
-set -e
-
-# List of exported symbols
-#
-# If the object has no symbol, $NM warns 'no symbols'.
-# Suppress the stderr.
-# TODO:
-#   Use -q instead of 2>/dev/null when we upgrade the minimum version of
-#   binutils to 2.37, llvm to 13.0.0.
-ksyms=$($NM $1 2>/dev/null | sed -n 's/.*__ksym_marker_\(.*\)/\1/p')
-
-if [ -z "$ksyms" ]; then
-	exit 0
-fi
-
-echo
-echo "ksymdeps_$1 := \\"
-
-for s in $ksyms
-do
-	printf '    $(wildcard include/ksym/%s) \\\n' "$s"
-done
-
-echo
-echo "$1: \$(ksymdeps_$1)"
-echo
-echo "\$(ksymdeps_$1):"
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 010c4f88d77b..ff0b8e562663 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -35,6 +35,9 @@ static bool warn_unresolved;
 
 static int sec_mismatch_count;
 static bool sec_mismatch_warn_only = true;
+/* Trim EXPORT_SYMBOLs that are unused by in-tree modules */
+static bool trim_unused_exports;
+
 /* ignore missing files */
 static bool ignore_missing_files;
 /* If set to 1, only warn (instead of error) about missing ns imports */
@@ -217,6 +220,7 @@ struct symbol {
 	bool weak;
 	bool is_func;
 	bool is_gpl_only;	/* exported by EXPORT_SYMBOL_GPL */
+	bool used;		/* there exists a user of this symbol */
 	char name[];
 };
 
@@ -1781,6 +1785,7 @@ static void check_exports(struct module *mod)
 			continue;
 		}
 
+		exp->used = true;
 		s->module = exp->module;
 		s->crc_valid = exp->crc_valid;
 		s->crc = exp->crc;
@@ -1804,6 +1809,23 @@ static void check_exports(struct module *mod)
 	}
 }
 
+static void handle_white_list_exports(const char *white_list)
+{
+	char *buf, *p, *name;
+
+	buf = read_text_file(white_list);
+	p = buf;
+
+	while ((name = strsep(&p, "\n"))) {
+		struct symbol *sym = find_symbol(name);
+
+		if (sym)
+			sym->used = true;
+	}
+
+	free(buf);
+}
+
 static void check_modname_len(struct module *mod)
 {
 	const char *mod_name;
@@ -1874,10 +1896,14 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod)
 
 	/* generate struct for exported symbols */
 	buf_printf(buf, "\n");
-	list_for_each_entry(sym, &mod->exported_symbols, list)
+	list_for_each_entry(sym, &mod->exported_symbols, list) {
+		if (trim_unused_exports && !sym->used)
+			continue;
+
 		buf_printf(buf, "KSYMTAB_%s(%s, \"%s\", \"%s\");\n",
 			   sym->is_func ? "FUNC" : "DATA", sym->name,
 			   sym->is_gpl_only ? "_gpl" : "", sym->namespace);
+	}
 
 	if (!modversions)
 		return;
@@ -1885,6 +1911,9 @@ static void add_exported_symbols(struct buffer *buf, struct module *mod)
 	/* record CRCs for exported symbols */
 	buf_printf(buf, "\n");
 	list_for_each_entry(sym, &mod->exported_symbols, list) {
+		if (trim_unused_exports && !sym->used)
+			continue;
+
 		if (!sym->crc_valid)
 			warn("EXPORT symbol \"%s\" [%s%s] version generation failed, symbol will not be versioned.\n"
 			     "Is \"%s\" prototyped in <asm/asm-prototypes.h>?\n",
@@ -2048,9 +2077,6 @@ static void write_mod_c_file(struct module *mod)
 	char fname[PATH_MAX];
 	int ret;
 
-	check_modname_len(mod);
-	check_exports(mod);
-
 	add_header(&buf, mod);
 	add_exported_symbols(&buf, mod);
 	add_versions(&buf, mod);
@@ -2184,12 +2210,13 @@ int main(int argc, char **argv)
 {
 	struct module *mod;
 	char *missing_namespace_deps = NULL;
+	char *unused_exports_white_list = NULL;
 	char *dump_write = NULL, *files_source = NULL;
 	int opt;
 	LIST_HEAD(dump_lists);
 	struct dump_list *dl, *dl2;
 
-	while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) {
+	while ((opt = getopt(argc, argv, "ei:mntT:tW:o:awENd:")) != -1) {
 		switch (opt) {
 		case 'e':
 			external_module = true;
@@ -2214,6 +2241,12 @@ int main(int argc, char **argv)
 		case 'T':
 			files_source = optarg;
 			break;
+		case 't':
+			trim_unused_exports = true;
+			break;
+		case 'W':
+			unused_exports_white_list = optarg;
+			break;
 		case 'w':
 			warn_unresolved = true;
 			break;
@@ -2243,6 +2276,17 @@ int main(int argc, char **argv)
 	if (files_source)
 		read_symbols_from_files(files_source);
 
+	list_for_each_entry(mod, &modules, list) {
+		if (mod->from_dump || mod->is_vmlinux)
+			continue;
+
+		check_modname_len(mod);
+		check_exports(mod);
+	}
+
+	if (unused_exports_white_list)
+		handle_white_list_exports(unused_exports_white_list);
+
 	list_for_each_entry(mod, &modules, list) {
 		if (mod->from_dump)
 			continue;
diff --git a/scripts/remove-stale-files b/scripts/remove-stale-files
index 7f432900671a..8502a17d47df 100755
--- a/scripts/remove-stale-files
+++ b/scripts/remove-stale-files
@@ -33,3 +33,5 @@ rm -f rust/target.json
 rm -f scripts/bin2c
 
 rm -f .scmversion
+
+rm -rf include/ksym
-- 
2.39.2

Re: [PATCH v5 21/21] kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion

[Nicolas Pitre]

On Mon, 15 May 2023, Masahiro Yamada wrote:

> When CONFIG_TRIM_UNUSED_KSYMS is enabled, Kbuild recursively traverses
> the directory tree to determine which EXPORT_SYMBOL to trim. If an
> EXPORT_SYMBOL turns out to be unused by anyone, Kbuild begins the
> second traverse, where some source files are recompiled with their
> EXPORT_SYMBOL() tuned into a no-op.
> 
> Linus stated negative opinions about this slowness in commits:
> 
>  - 5cf0fd591f2e ("Kbuild: disable TRIM_UNUSED_KSYMS option")
>  - a555bdd0c58c ("Kbuild: enable TRIM_UNUSED_KSYMS again, with some guarding")
> 
> We can do this better now. The final data structures of EXPORT_SYMBOL
> are generated by the modpost stage, so modpost can selectively emit
> KSYMTAB entries that are really used by modules.
> 
> Commit 2cce989f8461 ("kbuild: unify two modpost invocations") is another
> ground-work to do this in a one-pass algorithm. With the list of modules,
> modpost sets sym->used if it is used by a module. modpost emits KSYMTAB
> only for symbols with sym->used==true.
> 
> BTW, 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

I did achieve it using LTO with gcc back then. See the section called 
"The tree that hides the forest" of https://lwn.net/Articles/746780/ for 
example results.

> If deeper trimming is required, we need to revisit this, but I guess
> that is unlikely to happen.

Would have been nicer to keep this possibility as an option. The code is 
already there and working as intended. The build cost is intrinsic to 
the approach of course. The actual bug is to impose that cost onto 
people who didn't explicitly ask for it.

But I'm no longer fighting this battle.


Nicolas

Re: [PATCH v5 21/21] kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion

[Sami Tolvanen]

On Mon, May 15, 2023 at 2:39 PM Nicolas Pitre <nico@fluxnic.net> wrote:
>
> On Mon, 15 May 2023, Masahiro Yamada wrote:
>
> > When CONFIG_TRIM_UNUSED_KSYMS is enabled, Kbuild recursively traverses
> > the directory tree to determine which EXPORT_SYMBOL to trim. If an
> > EXPORT_SYMBOL turns out to be unused by anyone, Kbuild begins the
> > second traverse, where some source files are recompiled with their
> > EXPORT_SYMBOL() tuned into a no-op.
> >
> > Linus stated negative opinions about this slowness in commits:
> >
> >  - 5cf0fd591f2e ("Kbuild: disable TRIM_UNUSED_KSYMS option")
> >  - a555bdd0c58c ("Kbuild: enable TRIM_UNUSED_KSYMS again, with some guarding")
> >
> > We can do this better now. The final data structures of EXPORT_SYMBOL
> > are generated by the modpost stage, so modpost can selectively emit
> > KSYMTAB entries that are really used by modules.
> >
> > Commit 2cce989f8461 ("kbuild: unify two modpost invocations") is another
> > ground-work to do this in a one-pass algorithm. With the list of modules,
> > modpost sets sym->used if it is used by a module. modpost emits KSYMTAB
> > only for symbols with sym->used==true.
> >
> > BTW, 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
>
> I did achieve it using LTO with gcc back then. See the section called
> "The tree that hides the forest" of https://lwn.net/Articles/746780/ for
> example results.

Clang can do similar optimizations, but not in relocatable links where
the linker must obviously preserve all the globals. A while ago there
was a suggestion of adding an option to LLD that allows one to pass a
list of symbols to preserve in relocatable LTO links, which would
allow us to better optimize vmlinux.o. However, I haven't had a chance
to look into this deeper than this proof of concept:

https://reviews.llvm.org/D142163

> > If deeper trimming is required, we need to revisit this, but I guess
> > that is unlikely to happen.
>
> Would have been nicer to keep this possibility as an option. The code is
> already there and working as intended. The build cost is intrinsic to
> the approach of course. The actual bug is to impose that cost onto
> people who didn't explicitly ask for it.
>
> But I'm no longer fighting this battle.

I agree, this looks like a reasonable solution for now.

Sami

Re: [PATCH v5 21/21] kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion

[Masahiro Yamada]

On Tue, May 16, 2023 at 7:54 AM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Mon, May 15, 2023 at 2:39 PM Nicolas Pitre <nico@fluxnic.net> wrote:
> >
> > On Mon, 15 May 2023, Masahiro Yamada wrote:
> >
> > > When CONFIG_TRIM_UNUSED_KSYMS is enabled, Kbuild recursively traverses
> > > the directory tree to determine which EXPORT_SYMBOL to trim. If an
> > > EXPORT_SYMBOL turns out to be unused by anyone, Kbuild begins the
> > > second traverse, where some source files are recompiled with their
> > > EXPORT_SYMBOL() tuned into a no-op.
> > >
> > > Linus stated negative opinions about this slowness in commits:
> > >
> > >  - 5cf0fd591f2e ("Kbuild: disable TRIM_UNUSED_KSYMS option")
> > >  - a555bdd0c58c ("Kbuild: enable TRIM_UNUSED_KSYMS again, with some guarding")
> > >
> > > We can do this better now. The final data structures of EXPORT_SYMBOL
> > > are generated by the modpost stage, so modpost can selectively emit
> > > KSYMTAB entries that are really used by modules.
> > >
> > > Commit 2cce989f8461 ("kbuild: unify two modpost invocations") is another
> > > ground-work to do this in a one-pass algorithm. With the list of modules,
> > > modpost sets sym->used if it is used by a module. modpost emits KSYMTAB
> > > only for symbols with sym->used==true.
> > >
> > > BTW, 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
> >
> > I did achieve it using LTO with gcc back then. See the section called
> > "The tree that hides the forest" of https://lwn.net/Articles/746780/ for
> > example results.
>
> Clang can do similar optimizations, but not in relocatable links where
> the linker must obviously preserve all the globals.

Yeah, the issue is not in the compiler itself
but in the way CONFIG_LTO_CLANG was implemented.

If it had been implemented in the final link stage,
it would have required LTO running three times
with CONFIG_KALLSYMS=y.
But scripts/generate_initcall_order.pl would
have been unneeded. And, maybe we would get slightly
better vmlinux.

I think the help message of CONFIG_LTO_CLANG_FULL is
a misleading advertisement.

We did not achieve such deeper trimming
that is described in this link:

  https://llvm.org/docs/LinkTimeOptimization.html


If I remember correctly, GCC LTO was implemented
in the final link stage. So, trimming was depper
but it ran three times.





> A while ago there
> was a suggestion of adding an option to LLD that allows one to pass a
> list of symbols to preserve in relocatable LTO links, which would
> allow us to better optimize vmlinux.o. However, I haven't had a chance
> to look into this deeper than this proof of concept:
>
> https://reviews.llvm.org/D142163


Interesting.

But, scripts/generate_initcall_order.pl is still needed, right?

--lto-export-symbol-list is a list of symbols,
but it does not specify the correct order?



Nocolas explained the chain reaction of
compiling modules with LTO, but I suspect it
because modules are always relocatable ELF.


The LWN article (https://lwn.net/Articles/746780/) is awesome
but I think the benefit of LTO is for vmlinux,
not for modules.



>
> > > If deeper trimming is required, we need to revisit this, but I guess
> > > that is unlikely to happen.
> >
> > Would have been nicer to keep this possibility as an option. The code is
> > already there and working as intended. The build cost is intrinsic to
> > the approach of course. The actual bug is to impose that cost onto
> > people who didn't explicitly ask for it.
> >
> > But I'm no longer fighting this battle.
>
> I agree, this looks like a reasonable solution for now.
>
> Sami



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v5 21/21] kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion

[Sami Tolvanen]

On Mon, May 15, 2023 at 7:16 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Tue, May 16, 2023 at 7:54 AM Sami Tolvanen <samitolvanen@google.com> wrote:
> > A while ago there
> > was a suggestion of adding an option to LLD that allows one to pass a
> > list of symbols to preserve in relocatable LTO links, which would
> > allow us to better optimize vmlinux.o. However, I haven't had a chance
> > to look into this deeper than this proof of concept:
> >
> > https://reviews.llvm.org/D142163
>
>
> Interesting.
>
> But, scripts/generate_initcall_order.pl is still needed, right?
>
> --lto-export-symbol-list is a list of symbols,
> but it does not specify the correct order?

Correct, the patch doesn't take the order of the list into account.

Sami

Re: [PATCH v5 21/21] kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion

[Masahiro Yamada]

On Mon, May 15, 2023 at 12:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> When CONFIG_TRIM_UNUSED_KSYMS is enabled, Kbuild recursively traverses
> the directory tree to determine which EXPORT_SYMBOL to trim. If an
> EXPORT_SYMBOL turns out to be unused by anyone, Kbuild begins the
> second traverse, where some source files are recompiled with their
> EXPORT_SYMBOL() tuned into a no-op.
>
> Linus stated negative opinions about this slowness in commits:
>
>  - 5cf0fd591f2e ("Kbuild: disable TRIM_UNUSED_KSYMS option")
>  - a555bdd0c58c ("Kbuild: enable TRIM_UNUSED_KSYMS again, with some guarding")
>
> We can do this better now. The final data structures of EXPORT_SYMBOL
> are generated by the modpost stage, so modpost can selectively emit
> KSYMTAB entries that are really used by modules.
>
> Commit 2cce989f8461 ("kbuild: unify two modpost invocations") is another

The commit hash is wrong.

Commit f73edc8951b2 ("kbuild: unify two modpost invocations") is correct.






-- 
Best Regards
Masahiro Yamada

Re: [PATCH v5 00/21] Unify <linux/export.h> and <asm/export.h>, remove EXPORT_DATA_SYMBOL(), faster TRIM_UNUSED_KSYMS

[Masahiro Yamada]

On Mon, May 15, 2023 at 12:27 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
>
> This patch set refactors modpost first to make it easier to
> add new code.
>
> My goals:
>
>  - 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.
>    This removes EXPORT_DATA_SYMBOL().
>
>  - Re-implement TRIM_UNUSED_KSYMS in one-pass.
>    This makes the building faster.
>
>  - Move the static EXPORT_SYMBOL check to modpost.
>    This also makes the building faster.
>
> Previous version
> v4: https://lore.kernel.org/linux-kbuild/CAK7LNASDzy9RERN6+q6WgR4ROYZQue=SBqgbcoYuVePByHtk6Q@mail.gmail.com/T/#t
> v3: https://lore.kernel.org/all/20220928063947.299333-1-masahiroy@kernel.org/
>


01-10 applied to linux-kbuild.

I will send v6 for the rest.



>
> Masahiro Yamada (21):
>   modpost: remove broken calculation of exception_table_entry size
>   modpost: remove fromsym info in __ex_table section mismatch warning
>   modpost: remove get_prettyname()
>   modpost: squash report_extable_warnings() into
>     extable_mismatch_handler()
>   modpost: squash report_sec_mismatch() into default_mismatch_handler()
>   modpost: clean up is_executable_section()
>   modpost: squash extable_mismatch_handler() into
>     default_mismatch_handler()
>   modpost: pass 'tosec' down to default_mismatch_handler()
>   modpost: pass section index to find_elf_symbol2()
>   modpost: rename find_elf_symbol() and find_elf_symbol2()
>   modpost: modpost: refactor find_fromsym() and find_tosym()
>   modpost: unify 'sym' and 'to' in default_mismatch_handler()
>   modpost: replace r->r_offset, r->r_addend with faddr, taddr
>   modpost: remove is_shndx_special() check from section_rel(a)
>   modpost: pass struct module pointer to check_section_mismatch()
>   kbuild: generate KSYMTAB entries by modpost
>   ia64,export.h: replace EXPORT_DATA_SYMBOL* with EXPORT_SYMBOL*
>   modpost: check static EXPORT_SYMBOL* by modpost again
>   modpost: squash sym_update_namespace() into sym_add_exported()
>   modpost: use null string instead of NULL pointer for default namespace
>   kbuild: implement CONFIG_TRIM_UNUSED_KSYMS without recursion
>
>  .gitignore                        |   1 -
>  Makefile                          |  19 +-
>  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/asm-generic/vmlinux.lds.h |   1 +
>  include/linux/export-internal.h   |  49 +++
>  include/linux/export.h            | 119 ++-----
>  include/linux/pm.h                |   8 +-
>  kernel/module/internal.h          |  12 +
>  scripts/Makefile.build            |  19 +-
>  scripts/Makefile.modpost          |   7 +
>  scripts/adjust_autoksyms.sh       |  73 ----
>  scripts/basic/fixdep.c            |   3 +-
>  scripts/check-local-export        |  70 ----
>  scripts/gen_ksymdeps.sh           |  30 --
>  scripts/mod/modpost.c             | 561 ++++++++++++------------------
>  scripts/mod/modpost.h             |   1 +
>  scripts/remove-stale-files        |   2 +
>  21 files changed, 343 insertions(+), 723 deletions(-)
>  delete mode 100644 arch/ia64/include/asm/export.h
>  delete mode 100755 scripts/adjust_autoksyms.sh
>  delete mode 100755 scripts/check-local-export
>  delete mode 100755 scripts/gen_ksymdeps.sh
>
> --
> 2.39.2
>


-- 
Best Regards
Masahiro Yamada