| Message ID | 20241212154619.2235767-1-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | modpost: distinguish same module paths from different dump files | expand |
On Fri, Dec 13, 2024 at 12:46 AM Masahiro Yamada <masahiroy@kernel.org> 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 is -> are > 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@nvidia.com> > Closes: https://lore.kernel.org/all/eb21a546-a19c-40df-b821-bbba80f19a3d@nvidia.com/ > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > ---
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@nvidia.com> > Closes: https://lore.kernel.org/all/eb21a546-a19c-40df-b821-bbba80f19a3d@nvidia.com/ > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > --- > > 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@nvidia.com> Jon
On 12/12/2024 20:17, Jon Hunter wrote: > > 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@nvidia.com> >> Closes: https://lore.kernel.org/all/eb21a546-a19c-40df-b821- >> bbba80f19a3d@nvidia.com/ >> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> >> --- >> >> 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@nvidia.com> I have not seen this land in -next yet. Would be great to get this applied. Thanks! Jon
On Thu, Dec 19, 2024 at 7:48 PM Jon Hunter <jonathanh@nvidia.com> wrote: > > > On 12/12/2024 20:17, Jon Hunter wrote: > > > > 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@nvidia.com> > >> Closes: https://lore.kernel.org/all/eb21a546-a19c-40df-b821- > >> bbba80f19a3d@nvidia.com/ > >> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > >> --- > >> > >> 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@nvidia.com> > > > I have not seen this land in -next yet. Would be great to get this applied. > Now applied to linux-kbuild/fixes.
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;
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@nvidia.com> Closes: https://lore.kernel.org/all/eb21a546-a19c-40df-b821-bbba80f19a3d@nvidia.com/ Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> --- scripts/mod/modpost.c | 17 +++++++++-------- scripts/mod/modpost.h | 3 ++- 2 files changed, 11 insertions(+), 9 deletions(-)