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

[Jon Hunter <jonathanh@nvidia.com>]

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

-- 
nvpublic