Re: [PATCH] modpost: distinguish same module paths from different dump files

[Jon Hunter]

On 12/12/2024 15:46, Masahiro Yamada wrote:
> Since commit 13b25489b6f8 ("kbuild: change working directory to external
> module directory with M="), module paths are always relative to the top
> of the external module tree.
>
> The module paths recorded in Module.symvers is no longer globally unique
> when they are passed via KBUILD_EXTRA_SYMBOLS for building other external
> modules, which may result in false positive "exported twice" errors.
> Such errors should not occur because external modules should be able to
> override in-tree modules.
>
> To address this, record the dump file path in struct module and check it
> when searching for a module.
>
> Fixes: 13b25489b6f8 ("kbuild: change working directory to external module directory with M=")
> Reported-by: Jon Hunter <jonathanh@xxxxxxxxxx>
> Closes: https://lore.kernel.org/all/eb21a546-a19c-40df-b821-bbba80f19a3d@xxxxxxxxxx/
> Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> ---
>
>   scripts/mod/modpost.c | 17 +++++++++--------
>   scripts/mod/modpost.h |  3 ++-
>   2 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index fb787a5715f5..94ee49207a45 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -155,12 +155,13 @@ char *get_line(char **stringp)
>   /* A list of all modules we processed */
>   LIST_HEAD(modules);
>   
> -static struct module *find_module(const char *modname)
> +static struct module *find_module(const char *filename, const char *modname)
>   {
>   	struct module *mod;
>   
>   	list_for_each_entry(mod, &modules, list) {
> -		if (strcmp(mod->name, modname) == 0)
> +		if (!strcmp(mod->dump_file, filename) &&
> +		    !strcmp(mod->name, modname))
>   			return mod;
>   	}
>   	return NULL;
> @@ -2030,10 +2031,10 @@ static void read_dump(const char *fname)
>   			continue;
>   		}
>   
> -		mod = find_module(modname);
> +		mod = find_module(fname, modname);
>   		if (!mod) {
>   			mod = new_module(modname, strlen(modname));
> -			mod->from_dump = true;
> +			mod->dump_file = fname;
>   		}
>   		s = sym_add_exported(symname, mod, gpl_only, namespace);
>   		sym_set_crc(s, crc);
> @@ -2052,7 +2053,7 @@ static void write_dump(const char *fname)
>   	struct symbol *sym;
>   
>   	list_for_each_entry(mod, &modules, list) {
> -		if (mod->from_dump)
> +		if (mod->dump_file)
>   			continue;
>   		list_for_each_entry(sym, &mod->exported_symbols, list) {
>   			if (trim_unused_exports && !sym->used)
> @@ -2076,7 +2077,7 @@ static void write_namespace_deps_files(const char *fname)
>   
>   	list_for_each_entry(mod, &modules, list) {
>   
> -		if (mod->from_dump || list_empty(&mod->missing_namespaces))
> +		if (mod->dump_file || list_empty(&mod->missing_namespaces))
>   			continue;
>   
>   		buf_printf(&ns_deps_buf, "%s.ko:", mod->name);
> @@ -2194,7 +2195,7 @@ int main(int argc, char **argv)
>   		read_symbols_from_files(files_source);
>   
>   	list_for_each_entry(mod, &modules, list) {
> -		if (mod->from_dump || mod->is_vmlinux)
> +		if (mod->dump_file || mod->is_vmlinux)
>   			continue;
>   
>   		check_modname_len(mod);
> @@ -2205,7 +2206,7 @@ int main(int argc, char **argv)
>   		handle_white_list_exports(unused_exports_white_list);
>   
>   	list_for_each_entry(mod, &modules, list) {
> -		if (mod->from_dump)
> +		if (mod->dump_file)
>   			continue;
>   
>   		if (mod->is_vmlinux)
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index 49848fcbe2a1..8b72c227ebf4 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -95,14 +95,15 @@ struct module_alias {
>   /**
>    * struct module - represent a module (vmlinux or *.ko)
>    *
> + * @dump_file: path to the .symvers file if loaded from a file
>    * @aliases: list head for module_aliases
>    */
>   struct module {
>   	struct list_head list;
>   	struct list_head exported_symbols;
>   	struct list_head unresolved_symbols;
> +	const char *dump_file;
>   	bool is_gpl_compatible;
> -	bool from_dump;		/* true if module was loaded from *.symvers */
>   	bool is_vmlinux;
>   	bool seen;
>   	bool has_init;


Thanks for fixing!

Tested-by: Jon Hunter <jonathanh@xxxxxxxxxx>

Jon

--
nvpublic