[PATCH 1/6] modpost: add a helper to get data pointed by a symbol

[PATCH 1/6] modpost: add a helper to get data pointed by a symbol

[Masahiro Yamada]

When CONFIG_MODULE_REL_CRCS is enabled, the value of __crc_* is not
an absolute value, but the address to the CRC data embedded in the
.rodata section.

Getting the data pointed by the symbol value is somewhat complex.
Split it out into a new helper, sym_get_data().

I will reuse it to refactor namespace_from_kstrtabns() in the next
commit.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 46d7f695fe7f..cd885573daaf 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -308,6 +308,18 @@ static const char *sec_name(struct elf_info *elf, int secindex)
 	return sech_name(elf, &elf->sechdrs[secindex]);
 }
 
+static void *sym_get_data(const struct elf_info *info, const Elf_Sym *sym)
+{
+	Elf_Shdr *sechdr = &info->sechdrs[sym->st_shndx];
+	unsigned long offset;
+
+	offset = sym->st_value;
+	if (info->hdr->e_type != ET_REL)
+		offset -= sechdr->sh_addr;
+
+	return (void *)info->hdr + sechdr->sh_offset + offset;
+}
+
 #define strstarts(str, prefix) (strncmp(str, prefix, strlen(prefix)) == 0)
 
 static enum export export_from_secname(struct elf_info *elf, unsigned int sec)
@@ -697,10 +709,7 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
 			unsigned int *crcp;
 
 			/* symbol points to the CRC in the ELF object */
-			crcp = (void *)info->hdr + sym->st_value +
-			       info->sechdrs[sym->st_shndx].sh_offset -
-			       (info->hdr->e_type != ET_REL ?
-				info->sechdrs[sym->st_shndx].sh_addr : 0);
+			crcp = sym_get_data(info, sym);
 			crc = TO_NATIVE(*crcp);
 		}
 		sym_update_crc(symname + strlen("__crc_"), mod, crc,
-- 
2.17.1

[PATCH 2/6] modpost: refactor namespace_from_kstrtabns() to not hard-code section name

[Masahiro Yamada]

Currently, namespace_from_kstrtabns() relies on the fact that
namespace strings are recorded in the __ksymtab_strings section.
Actually, it is coded in include/linux/export.h, but modpost does
not need to hard-code the section name.

Elf_Sym::st_shndx holds the section number of the relevant section.
Using it is a more portable way to find the namespace string.

sym_get_value() takes care of it, so namespace_from_kstrtabns() can
simply wrap it. Delete the unneeded info->ksymtab_strings .

While I was here, I added more 'const' qualifiers to pointers.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 scripts/mod/modpost.c | 10 +++-------
 scripts/mod/modpost.h |  1 -
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index cd885573daaf..d9418c58a8c0 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -356,10 +356,10 @@ static enum export export_from_sec(struct elf_info *elf, unsigned int sec)
 		return export_unknown;
 }
 
-static const char *namespace_from_kstrtabns(struct elf_info *info,
-					    Elf_Sym *kstrtabns)
+static const char *namespace_from_kstrtabns(const struct elf_info *info,
+					    const Elf_Sym *sym)
 {
-	char *value = info->ksymtab_strings + kstrtabns->st_value;
+	const char *value = sym_get_data(info, sym);
 	return value[0] ? value : NULL;
 }
 
@@ -601,10 +601,6 @@ static int parse_elf(struct elf_info *info, const char *filename)
 			info->export_unused_gpl_sec = i;
 		else if (strcmp(secname, "__ksymtab_gpl_future") == 0)
 			info->export_gpl_future_sec = i;
-		else if (strcmp(secname, "__ksymtab_strings") == 0)
-			info->ksymtab_strings = (void *)hdr +
-						sechdrs[i].sh_offset -
-						sechdrs[i].sh_addr;
 
 		if (sechdrs[i].sh_type == SHT_SYMTAB) {
 			unsigned int sh_link_idx;
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index fe6652535e4b..64a82d2d85f6 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -143,7 +143,6 @@ struct elf_info {
 	Elf_Section  export_gpl_sec;
 	Elf_Section  export_unused_gpl_sec;
 	Elf_Section  export_gpl_future_sec;
-	char	     *ksymtab_strings;
 	char         *strtab;
 	char	     *modinfo;
 	unsigned int modinfo_len;
-- 
2.17.1

[PATCH 3/6] modpost: rename handle_modversions() to handle_symbol()

[Masahiro Yamada]

This function handles not only modversions, but also unresolved
symbols, export symbols, etc.

Rename it to a more proper function name.

While I was here, I also added the 'const' qualifier to *sym.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index d9418c58a8c0..6735ae3da4c2 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -683,8 +683,8 @@ static int ignore_undef_symbol(struct elf_info *info, const char *symname)
 	return 0;
 }
 
-static void handle_modversions(struct module *mod, struct elf_info *info,
-			       Elf_Sym *sym, const char *symname)
+static void handle_symbol(struct module *mod, struct elf_info *info,
+			  const Elf_Sym *sym, const char *symname)
 {
 	unsigned int crc;
 	enum export export;
@@ -2051,7 +2051,7 @@ static void read_symbols(const char *modname)
 	for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
 		symname = remove_dot(info.strtab + sym->st_name);
 
-		handle_modversions(mod, &info, sym, symname);
+		handle_symbol(mod, &info, sym, symname);
 		handle_moddevtable(mod, &info, sym, symname);
 	}
 
-- 
2.17.1

[PATCH 4/6] modpost: stop symbol preloading for modversion CRC

[Masahiro Yamada]

It is complicated to add mocked-up symbols to pre-handle CRC.
Handle CRC after all the export symbols in the relevant module
are registered.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 6735ae3da4c2..73bdf27c41fe 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -169,7 +169,7 @@ struct symbol {
 	unsigned int vmlinux:1;    /* 1 if symbol is defined in vmlinux */
 	unsigned int kernel:1;     /* 1 if symbol is from kernel
 				    *  (only for external modules) **/
-	unsigned int preloaded:1;  /* 1 if symbol from Module.symvers, or crc */
+	unsigned int preloaded:1;  /* 1 if symbol from Module.symvers */
 	unsigned int is_static:1;  /* 1 if symbol is not global */
 	enum export  export;       /* Type of export */
 	char name[0];
@@ -410,15 +410,15 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
 	return s;
 }
 
-static void sym_update_crc(const char *name, struct module *mod,
-			   unsigned int crc, enum export export)
+static void sym_set_crc(const char *name, const struct module *mod,
+			unsigned int crc)
 {
 	struct symbol *s = find_symbol(name);
 
 	if (!s) {
-		s = new_symbol(name, mod, export);
-		/* Don't complain when we find it later. */
-		s->preloaded = 1;
+		warn("%s: '__crc_%s' is invalid use. __crc_* is reserved for modversion\n",
+		     mod->name, name);
+		return;
 	}
 	s->crc = crc;
 	s->crc_valid = 1;
@@ -683,12 +683,34 @@ static int ignore_undef_symbol(struct elf_info *info, const char *symname)
 	return 0;
 }
 
+static void handle_modversion(const struct module *mod,
+			      const struct elf_info *info,
+			      const Elf_Sym *sym, const char *symname)
+{
+	unsigned int crc;
+
+	if (sym->st_shndx == SHN_UNDEF) {
+		warn("EXPORT symbol \"%s\" [%s%s] version generation failed, symbol will not be versioned.\n",
+		     symname, mod->name, is_vmlinux(mod->name) ? "":".ko");
+		return;
+	}
+
+	if (sym->st_shndx == SHN_ABS) {
+		crc = sym->st_value;
+	} else {
+		unsigned int *crcp;
+
+		/* symbol points to the CRC in the ELF object */
+		crcp = sym_get_data(info, sym);
+		crc = TO_NATIVE(*crcp);
+	}
+	sym_set_crc(symname, mod, crc);
+}
+
 static void handle_symbol(struct module *mod, struct elf_info *info,
 			  const Elf_Sym *sym, const char *symname)
 {
-	unsigned int crc;
 	enum export export;
-	bool is_crc = false;
 	const char *name;
 
 	if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
@@ -697,21 +719,6 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
 	else
 		export = export_from_sec(info, get_secindex(info, sym));
 
-	/* CRC'd symbol */
-	if (strstarts(symname, "__crc_")) {
-		is_crc = true;
-		crc = (unsigned int) sym->st_value;
-		if (sym->st_shndx != SHN_UNDEF && sym->st_shndx != SHN_ABS) {
-			unsigned int *crcp;
-
-			/* symbol points to the CRC in the ELF object */
-			crcp = sym_get_data(info, sym);
-			crc = TO_NATIVE(*crcp);
-		}
-		sym_update_crc(symname + strlen("__crc_"), mod, crc,
-				export);
-	}
-
 	switch (sym->st_shndx) {
 	case SHN_COMMON:
 		if (strstarts(symname, "__gnu_lto_")) {
@@ -746,11 +753,6 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
 		}
 #endif
 
-		if (is_crc) {
-			const char *e = is_vmlinux(mod->name) ?"":".ko";
-			warn("EXPORT symbol \"%s\" [%s%s] version generation failed, symbol will not be versioned.\n",
-			     symname + strlen("__crc_"), mod->name, e);
-		}
 		mod->unres = alloc_symbol(symname,
 					  ELF_ST_BIND(sym->st_info) == STB_WEAK,
 					  mod->unres);
@@ -2063,6 +2065,10 @@ static void read_symbols(const char *modname)
 			sym_update_namespace(symname + strlen("__kstrtabns_"),
 					     namespace_from_kstrtabns(&info,
 								      sym));
+
+		if (strstarts(symname, "__crc_"))
+			handle_modversion(mod, &info, sym,
+					  symname + strlen("__crc_"));
 	}
 
 	// check for static EXPORT_SYMBOL_* functions && global vars
@@ -2476,7 +2482,7 @@ static void read_dump(const char *fname, unsigned int kernel)
 		s->kernel    = kernel;
 		s->preloaded = 1;
 		s->is_static = 0;
-		sym_update_crc(symname, mod, crc, export_no(export));
+		sym_set_crc(symname, mod, crc);
 		sym_update_namespace(symname, namespace);
 	}
 	release_file(file, size);
-- 
2.17.1

[PATCH 5/6] modpost: do not set ->preloaded for symbols from Module.symvers

[Masahiro Yamada]

Now that there is no overwrap between symbols from ELF files and
ones from Module.symvers.

So, the 'exported twice' warning should be reported irrespective
of where the symbol in question came from.

The exceptional case is external module; in some cases, we build
an external module to provide a different version/variant of the
corresponding in-kernel module, overriding the same set of exported
symbols.

You can see this use-case in upstream; tools/testing/nvdimm/libnvdimm.ko
replaces drivers/nvdimm/libnvdimm.ko in order to link it against mocked
version of core kernel symbols.

So, let's relax the 'exported twice' warning when building external
modules. The multiple export from external modules is warned only
when the previous one is from vmlinux or itself.

With this refactoring, the ugly preloading goes away.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 73bdf27c41fe..06086105011f 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -169,7 +169,6 @@ struct symbol {
 	unsigned int vmlinux:1;    /* 1 if symbol is defined in vmlinux */
 	unsigned int kernel:1;     /* 1 if symbol is from kernel
 				    *  (only for external modules) **/
-	unsigned int preloaded:1;  /* 1 if symbol from Module.symvers */
 	unsigned int is_static:1;  /* 1 if symbol is not global */
 	enum export  export;       /* Type of export */
 	char name[0];
@@ -394,7 +393,8 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
 	if (!s) {
 		s = new_symbol(name, mod, export);
 	} else {
-		if (!s->preloaded) {
+		if (!external_module || is_vmlinux(s->module->name) ||
+		    s->module == mod) {
 			warn("%s: '%s' exported twice. Previous export was in %s%s\n",
 			     mod->name, name, s->module->name,
 			     is_vmlinux(s->module->name) ? "" : ".ko");
@@ -403,7 +403,6 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
 			s->module = mod;
 		}
 	}
-	s->preloaded = 0;
 	s->vmlinux   = is_vmlinux(mod->name);
 	s->kernel    = 0;
 	s->export    = export;
@@ -2480,7 +2479,6 @@ static void read_dump(const char *fname, unsigned int kernel)
 		}
 		s = sym_add_exported(symname, mod, export_no(export));
 		s->kernel    = kernel;
-		s->preloaded = 1;
 		s->is_static = 0;
 		sym_set_crc(symname, mod, crc);
 		sym_update_namespace(symname, namespace);
-- 
2.17.1

[PATCH 6/6] modpost: respect the previous export when 'exported twice' is warned

[Masahiro Yamada]

When 'exported twice' is warned, let sym_add_exported() return without
updating the symbol info. This respects the previous export, which is
ordered first in modules.order

This simplifies the code too.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 06086105011f..d9a92fbe1146 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -211,13 +211,11 @@ static struct symbol *new_symbol(const char *name, struct module *module,
 				 enum export export)
 {
 	unsigned int hash;
-	struct symbol *new;
 
 	hash = tdb_hash(name) % SYMBOL_HASH_SIZE;
-	new = symbolhash[hash] = alloc_symbol(name, 0, symbolhash[hash]);
-	new->module = module;
-	new->export = export;
-	return new;
+	symbolhash[hash] = alloc_symbol(name, 0, symbolhash[hash]);
+
+	return symbolhash[hash];
 }
 
 static struct symbol *find_symbol(const char *name)
@@ -392,17 +390,15 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
 
 	if (!s) {
 		s = new_symbol(name, mod, export);
-	} else {
-		if (!external_module || is_vmlinux(s->module->name) ||
-		    s->module == mod) {
-			warn("%s: '%s' exported twice. Previous export was in %s%s\n",
-			     mod->name, name, s->module->name,
-			     is_vmlinux(s->module->name) ? "" : ".ko");
-		} else {
-			/* In case Module.symvers was out of date */
-			s->module = mod;
-		}
+	} else if (!external_module || is_vmlinux(s->module->name) ||
+		   s->module == mod) {
+		warn("%s: '%s' exported twice. Previous export was in %s%s\n",
+		     mod->name, name, s->module->name,
+		     is_vmlinux(s->module->name) ? "" : ".ko");
+		return s;
 	}
+
+	s->module = mod;
 	s->vmlinux   = is_vmlinux(mod->name);
 	s->kernel    = 0;
 	s->export    = export;
-- 
2.17.1

Re: [PATCH 1/6] modpost: add a helper to get data pointed by a symbol

[Masahiro Yamada]

(+CC: Matthias, who might be interested)

On Fri, Nov 15, 2019 at 2:42 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> When CONFIG_MODULE_REL_CRCS is enabled, the value of __crc_* is not
> an absolute value, but the address to the CRC data embedded in the
> .rodata section.
>
> Getting the data pointed by the symbol value is somewhat complex.
> Split it out into a new helper, sym_get_data().
>
> I will reuse it to refactor namespace_from_kstrtabns() in the next
> commit.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  scripts/mod/modpost.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 46d7f695fe7f..cd885573daaf 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -308,6 +308,18 @@ static const char *sec_name(struct elf_info *elf, int secindex)
>         return sech_name(elf, &elf->sechdrs[secindex]);
>  }
>
> +static void *sym_get_data(const struct elf_info *info, const Elf_Sym *sym)
> +{
> +       Elf_Shdr *sechdr = &info->sechdrs[sym->st_shndx];
> +       unsigned long offset;
> +
> +       offset = sym->st_value;
> +       if (info->hdr->e_type != ET_REL)
> +               offset -= sechdr->sh_addr;
> +
> +       return (void *)info->hdr + sechdr->sh_offset + offset;
> +}
> +
>  #define strstarts(str, prefix) (strncmp(str, prefix, strlen(prefix)) == 0)
>
>  static enum export export_from_secname(struct elf_info *elf, unsigned int sec)
> @@ -697,10 +709,7 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
>                         unsigned int *crcp;
>
>                         /* symbol points to the CRC in the ELF object */
> -                       crcp = (void *)info->hdr + sym->st_value +
> -                              info->sechdrs[sym->st_shndx].sh_offset -
> -                              (info->hdr->e_type != ET_REL ?
> -                               info->sechdrs[sym->st_shndx].sh_addr : 0);
> +                       crcp = sym_get_data(info, sym);
>                         crc = TO_NATIVE(*crcp);
>                 }
>                 sym_update_crc(symname + strlen("__crc_"), mod, crc,
> --
> 2.17.1
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/6] modpost: refactor namespace_from_kstrtabns() to not hard-code section name

[Masahiro Yamada]

(+CC: Matthias, who might be interested)


On Fri, Nov 15, 2019 at 2:42 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> Currently, namespace_from_kstrtabns() relies on the fact that
> namespace strings are recorded in the __ksymtab_strings section.
> Actually, it is coded in include/linux/export.h, but modpost does
> not need to hard-code the section name.
>
> Elf_Sym::st_shndx holds the section number of the relevant section.
> Using it is a more portable way to find the namespace string.
>
> sym_get_value() takes care of it, so namespace_from_kstrtabns() can
> simply wrap it. Delete the unneeded info->ksymtab_strings .
>
> While I was here, I added more 'const' qualifiers to pointers.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  scripts/mod/modpost.c | 10 +++-------
>  scripts/mod/modpost.h |  1 -
>  2 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index cd885573daaf..d9418c58a8c0 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -356,10 +356,10 @@ static enum export export_from_sec(struct elf_info *elf, unsigned int sec)
>                 return export_unknown;
>  }
>
> -static const char *namespace_from_kstrtabns(struct elf_info *info,
> -                                           Elf_Sym *kstrtabns)
> +static const char *namespace_from_kstrtabns(const struct elf_info *info,
> +                                           const Elf_Sym *sym)
>  {
> -       char *value = info->ksymtab_strings + kstrtabns->st_value;
> +       const char *value = sym_get_data(info, sym);
>         return value[0] ? value : NULL;
>  }
>
> @@ -601,10 +601,6 @@ static int parse_elf(struct elf_info *info, const char *filename)
>                         info->export_unused_gpl_sec = i;
>                 else if (strcmp(secname, "__ksymtab_gpl_future") == 0)
>                         info->export_gpl_future_sec = i;
> -               else if (strcmp(secname, "__ksymtab_strings") == 0)
> -                       info->ksymtab_strings = (void *)hdr +
> -                                               sechdrs[i].sh_offset -
> -                                               sechdrs[i].sh_addr;
>
>                 if (sechdrs[i].sh_type == SHT_SYMTAB) {
>                         unsigned int sh_link_idx;
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index fe6652535e4b..64a82d2d85f6 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -143,7 +143,6 @@ struct elf_info {
>         Elf_Section  export_gpl_sec;
>         Elf_Section  export_unused_gpl_sec;
>         Elf_Section  export_gpl_future_sec;
> -       char         *ksymtab_strings;
>         char         *strtab;
>         char         *modinfo;
>         unsigned int modinfo_len;
> --
> 2.17.1
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 4/6] modpost: stop symbol preloading for modversion CRC

[Masahiro Yamada]

On Fri, Nov 15, 2019 at 2:42 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> It is complicated to add mocked-up symbols to pre-handle CRC.
> Handle CRC after all the export symbols in the relevant module
> are registered.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  scripts/mod/modpost.c | 64 +++++++++++++++++++++++--------------------
>  1 file changed, 35 insertions(+), 29 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 6735ae3da4c2..73bdf27c41fe 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -169,7 +169,7 @@ struct symbol {
>         unsigned int vmlinux:1;    /* 1 if symbol is defined in vmlinux */
>         unsigned int kernel:1;     /* 1 if symbol is from kernel
>                                     *  (only for external modules) **/
> -       unsigned int preloaded:1;  /* 1 if symbol from Module.symvers, or crc */
> +       unsigned int preloaded:1;  /* 1 if symbol from Module.symvers */
>         unsigned int is_static:1;  /* 1 if symbol is not global */
>         enum export  export;       /* Type of export */
>         char name[0];
> @@ -410,15 +410,15 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
>         return s;
>  }
>
> -static void sym_update_crc(const char *name, struct module *mod,
> -                          unsigned int crc, enum export export)
> +static void sym_set_crc(const char *name, const struct module *mod,
> +                       unsigned int crc)
>  {
>         struct symbol *s = find_symbol(name);
>
>         if (!s) {
> -               s = new_symbol(name, mod, export);
> -               /* Don't complain when we find it later. */
> -               s->preloaded = 1;
> +               warn("%s: '__crc_%s' is invalid use. __crc_* is reserved for modversion\n",
> +                    mod->name, name);

I notice this can produce false positive warnings.

ARCH=arm allyesconfig produces the following:

WARNING: vmlinux: '__crc_ccitt_veneer' is invalid use. __crc_* is
reserved for modversion
WARNING: vmlinux: '__crc_ccitt_veneer' is invalid use. __crc_* is
reserved for modversion
WARNING: vmlinux: '__crc_ccitt_veneer' is invalid use. __crc_* is
reserved for modversion
WARNING: vmlinux: '__crc_itu_t_veneer' is invalid use. __crc_* is
reserved for modversion
WARNING: vmlinux: '__crc_itu_t_veneer' is invalid use. __crc_* is
reserved for modversion
WARNING: vmlinux: '__crc_itu_t_veneer' is invalid use. __crc_* is
reserved for modversion


The ARM compiler inserts veneers automatically.

I will remove this warn(), and add commit messages as follows:

    In some cases, I see atand-alone __crc_* without __ksymtab_*.
    For example, ARCH=arm allyesconfig produces __crc_ccitt_veneer and
    __crc_itu_t_veneer. I guess they come from crc_ccitt, crc_itu_t,
    respectively. Since __*_veneer are auto-generated symbols, just
    ignore them.



> +               return;
>         }
>         s->crc = crc;
>         s->crc_valid = 1;
> @@ -683,12 +683,34 @@ static int ignore_undef_symbol(struct elf_info *info, const char *symname)
>         return 0;
>  }
>
> +static void handle_modversion(const struct module *mod,
> +                             const struct elf_info *info,
> +                             const Elf_Sym *sym, const char *symname)
> +{
> +       unsigned int crc;
> +
> +       if (sym->st_shndx == SHN_UNDEF) {
> +               warn("EXPORT symbol \"%s\" [%s%s] version generation failed, symbol will not be versioned.\n",
> +                    symname, mod->name, is_vmlinux(mod->name) ? "":".ko");
> +               return;
> +       }
> +
> +       if (sym->st_shndx == SHN_ABS) {
> +               crc = sym->st_value;
> +       } else {
> +               unsigned int *crcp;
> +
> +               /* symbol points to the CRC in the ELF object */
> +               crcp = sym_get_data(info, sym);
> +               crc = TO_NATIVE(*crcp);
> +       }
> +       sym_set_crc(symname, mod, crc);
> +}
> +
>  static void handle_symbol(struct module *mod, struct elf_info *info,
>                           const Elf_Sym *sym, const char *symname)
>  {
> -       unsigned int crc;
>         enum export export;
> -       bool is_crc = false;
>         const char *name;
>
>         if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
> @@ -697,21 +719,6 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
>         else
>                 export = export_from_sec(info, get_secindex(info, sym));
>
> -       /* CRC'd symbol */
> -       if (strstarts(symname, "__crc_")) {
> -               is_crc = true;
> -               crc = (unsigned int) sym->st_value;
> -               if (sym->st_shndx != SHN_UNDEF && sym->st_shndx != SHN_ABS) {
> -                       unsigned int *crcp;
> -
> -                       /* symbol points to the CRC in the ELF object */
> -                       crcp = sym_get_data(info, sym);
> -                       crc = TO_NATIVE(*crcp);
> -               }
> -               sym_update_crc(symname + strlen("__crc_"), mod, crc,
> -                               export);
> -       }
> -
>         switch (sym->st_shndx) {
>         case SHN_COMMON:
>                 if (strstarts(symname, "__gnu_lto_")) {
> @@ -746,11 +753,6 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
>                 }
>  #endif
>
> -               if (is_crc) {
> -                       const char *e = is_vmlinux(mod->name) ?"":".ko";
> -                       warn("EXPORT symbol \"%s\" [%s%s] version generation failed, symbol will not be versioned.\n",
> -                            symname + strlen("__crc_"), mod->name, e);
> -               }
>                 mod->unres = alloc_symbol(symname,
>                                           ELF_ST_BIND(sym->st_info) == STB_WEAK,
>                                           mod->unres);
> @@ -2063,6 +2065,10 @@ static void read_symbols(const char *modname)
>                         sym_update_namespace(symname + strlen("__kstrtabns_"),
>                                              namespace_from_kstrtabns(&info,
>                                                                       sym));
> +
> +               if (strstarts(symname, "__crc_"))
> +                       handle_modversion(mod, &info, sym,
> +                                         symname + strlen("__crc_"));
>         }
>
>         // check for static EXPORT_SYMBOL_* functions && global vars
> @@ -2476,7 +2482,7 @@ static void read_dump(const char *fname, unsigned int kernel)
>                 s->kernel    = kernel;
>                 s->preloaded = 1;
>                 s->is_static = 0;
> -               sym_update_crc(symname, mod, crc, export_no(export));
> +               sym_set_crc(symname, mod, crc);
>                 sym_update_namespace(symname, namespace);
>         }
>         release_file(file, size);
> --
> 2.17.1
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/6] modpost: add a helper to get data pointed by a symbol

[Masahiro Yamada]

On Fri, Nov 15, 2019 at 2:42 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> When CONFIG_MODULE_REL_CRCS is enabled, the value of __crc_* is not
> an absolute value, but the address to the CRC data embedded in the
> .rodata section.
>
> Getting the data pointed by the symbol value is somewhat complex.
> Split it out into a new helper, sym_get_data().
>
> I will reuse it to refactor namespace_from_kstrtabns() in the next
> commit.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Series, applied to linux-kbuild.


>
>  scripts/mod/modpost.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 46d7f695fe7f..cd885573daaf 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -308,6 +308,18 @@ static const char *sec_name(struct elf_info *elf, int secindex)
>         return sech_name(elf, &elf->sechdrs[secindex]);
>  }
>
> +static void *sym_get_data(const struct elf_info *info, const Elf_Sym *sym)
> +{
> +       Elf_Shdr *sechdr = &info->sechdrs[sym->st_shndx];
> +       unsigned long offset;
> +
> +       offset = sym->st_value;
> +       if (info->hdr->e_type != ET_REL)
> +               offset -= sechdr->sh_addr;
> +
> +       return (void *)info->hdr + sechdr->sh_offset + offset;
> +}
> +
>  #define strstarts(str, prefix) (strncmp(str, prefix, strlen(prefix)) == 0)
>
>  static enum export export_from_secname(struct elf_info *elf, unsigned int sec)
> @@ -697,10 +709,7 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
>                         unsigned int *crcp;
>
>                         /* symbol points to the CRC in the ELF object */
> -                       crcp = (void *)info->hdr + sym->st_value +
> -                              info->sechdrs[sym->st_shndx].sh_offset -
> -                              (info->hdr->e_type != ET_REL ?
> -                               info->sechdrs[sym->st_shndx].sh_addr : 0);
> +                       crcp = sym_get_data(info, sym);
>                         crc = TO_NATIVE(*crcp);
>                 }
>                 sym_update_crc(symname + strlen("__crc_"), mod, crc,
> --
> 2.17.1
>


-- 
Best Regards
Masahiro Yamada