Messages in this thread | | | Date | Tue, 27 Aug 2019 15:41:17 +0100 | From | Matthias Maennich <> | Subject | Re: [PATCH v3 04/11] modpost: add support for symbol namespaces |
| |
On Mon, Aug 26, 2019 at 06:21:38PM +0200, Jessica Yu wrote: >+++ Matthias Maennich [21/08/19 12:49 +0100]: >>Add support for symbols that are exported into namespaces. For that, >>extract any namespace suffix from the symbol name. In addition, emit a >>warning whenever a module refers to an exported symbol without >>explicitly importing the namespace that it is defined in. This patch >>consistently adds the namespace suffix to symbol names exported into >>Module.symvers. >> >>Example warning emitted by modpost in case of the above violation: >> >>WARNING: module ums-usbat uses symbol usb_stor_resume from namespace >>USB_STORAGE, but does not import it. >> >>Co-developed-by: Martijn Coenen <maco@android.com> >>Signed-off-by: Martijn Coenen <maco@android.com> >>Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> >>Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>Signed-off-by: Matthias Maennich <maennich@google.com> >>--- >>scripts/mod/modpost.c | 91 +++++++++++++++++++++++++++++++++++++------ >>scripts/mod/modpost.h | 7 ++++ >>2 files changed, 87 insertions(+), 11 deletions(-) >> >>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c >>index f277e116e0eb..538bb24ffee3 100644 >>--- a/scripts/mod/modpost.c >>+++ b/scripts/mod/modpost.c >>@@ -164,6 +164,7 @@ struct symbol { >> struct module *module; >> unsigned int crc; >> int crc_valid; >>+ const char *namespace; >> unsigned int weak:1; >> unsigned int vmlinux:1; /* 1 if symbol is defined in vmlinux */ >> unsigned int kernel:1; /* 1 if symbol is from kernel >>@@ -233,6 +234,37 @@ static struct symbol *find_symbol(const char *name) >> return NULL; >>} >> >>+static bool contains_namespace(struct namespace_list *list, >>+ const char *namespace) >>+{ >>+ struct namespace_list *ns_entry; >>+ >>+ for (ns_entry = list; ns_entry != NULL; ns_entry = ns_entry->next) >>+ if (strcmp(ns_entry->namespace, namespace) == 0) >>+ return true; >>+ >>+ return false; >>+} >>+ >>+static void add_namespace(struct namespace_list **list, const char *namespace) >>+{ >>+ struct namespace_list *ns_entry; >>+ >>+ if (!contains_namespace(*list, namespace)) { >>+ ns_entry = NOFAIL(malloc(sizeof(struct namespace_list) + >>+ strlen(namespace) + 1)); >>+ strcpy(ns_entry->namespace, namespace); >>+ ns_entry->next = *list; >>+ *list = ns_entry; >>+ } >>+} >>+ >>+static bool module_imports_namespace(struct module *module, >>+ const char *namespace) >>+{ >>+ return contains_namespace(module->imported_namespaces, namespace); >>+} >>+ >>static const struct { >> const char *str; >> enum export export; >>@@ -312,6 +344,22 @@ static enum export export_from_sec(struct elf_info *elf, unsigned int sec) >> return export_unknown; >>} >> >>+static const char *sym_extract_namespace(const char **symname) >>+{ >>+ size_t n; >>+ char *dupsymname; >>+ >>+ n = strcspn(*symname, "."); >>+ if (n < strlen(*symname) - 1) { >>+ dupsymname = NOFAIL(strdup(*symname)); >>+ dupsymname[n] = '\0'; >>+ *symname = dupsymname; >>+ return dupsymname + n + 1; >>+ } >>+ >>+ return NULL; >>+} >>+ >>/** >> * Add an exported symbol - it may have already been added without a >> * CRC, in this case just update the CRC >>@@ -319,16 +367,18 @@ static enum export export_from_sec(struct elf_info *elf, unsigned int sec) >>static struct symbol *sym_add_exported(const char *name, struct module *mod, >> enum export export) >>{ >>- struct symbol *s = find_symbol(name); >>+ const char *symbol_name = name; >>+ const char *namespace = sym_extract_namespace(&symbol_name); >>+ struct symbol *s = find_symbol(symbol_name); >> >> if (!s) { >>- s = new_symbol(name, mod, export); >>+ s = new_symbol(symbol_name, mod, export); >>+ s->namespace = namespace; >> } else { >> if (!s->preloaded) { >>- warn("%s: '%s' exported twice. Previous export " >>- "was in %s%s\n", mod->name, name, >>- s->module->name, >>- is_vmlinux(s->module->name) ?"":".ko"); >>+ warn("%s: '%s' exported twice. Previous export was in %s%s\n", >>+ mod->name, symbol_name, s->module->name, >>+ is_vmlinux(s->module->name) ? "" : ".ko"); >> } else { >> /* In case Module.symvers was out of date */ >> s->module = mod; >>@@ -1943,6 +1993,7 @@ static void read_symbols(const char *modname) >> const char *symname; >> char *version; >> char *license; >>+ char *namespace; >> struct module *mod; >> struct elf_info info = { }; >> Elf_Sym *sym; >>@@ -1974,6 +2025,12 @@ static void read_symbols(const char *modname) >> license = get_next_modinfo(&info, "license", license); >> } >> >>+ namespace = get_modinfo(&info, "import_ns"); >>+ while (namespace) { >>+ add_namespace(&mod->imported_namespaces, namespace); >>+ namespace = get_next_modinfo(&info, "import_ns", namespace); >>+ } >>+ >> for (sym = info.symtab_start; sym < info.symtab_stop; sym++) { >> symname = remove_dot(info.strtab + sym->st_name); >> >>@@ -2118,6 +2175,13 @@ static int check_exports(struct module *mod) >> basename++; >> else >> basename = mod->name; >>+ >>+ if (exp->namespace && >>+ !module_imports_namespace(mod, exp->namespace)) { >>+ warn("module %s uses symbol %s from namespace %s, but does not import it.\n", >>+ basename, exp->name, exp->namespace); >>+ } >>+ >> if (!mod->gpl_compatible) >> check_for_gpl_usage(exp->export, basename, exp->name); >> check_for_unused(exp->export, basename, exp->name); >>@@ -2395,16 +2459,21 @@ static void write_dump(const char *fname) >>{ >> struct buffer buf = { }; >> struct symbol *symbol; >>+ const char *namespace; >> int n; >> >> for (n = 0; n < SYMBOL_HASH_SIZE ; n++) { >> symbol = symbolhash[n]; >> while (symbol) { >>- if (dump_sym(symbol)) >>- buf_printf(&buf, "0x%08x\t%s\t%s\t%s\n", >>- symbol->crc, symbol->name, >>- symbol->module->name, >>- export_str(symbol->export)); >>+ if (dump_sym(symbol)) { >>+ namespace = symbol->namespace; >>+ buf_printf(&buf, "0x%08x\t%s%s%s\t%s\t%s\n", >>+ symbol->crc, symbol->name, >>+ namespace ? "." : "", >>+ namespace ? namespace : "", > >I think it might be cleaner to just have namespace be a separate >field in Module.symvers, rather than appending a dot and the >namespace at the end of a symbol name. Maybe something like > > <crc> <symbol_name> <namespace> <module> > >For symbols without a namespace, we could just have "", with all >fields delimited by tabs. This is just a stylistic suggestion, what do >you think?
I thought of something like that initially, but did not do it to not break users of this file. But as I am anyway breaking users by changing the symbol name into symbol.NS, I might as well do it as you suggested. Since read_dump() also knew already how to extract the namespaces from symbol.NS, it had already worked without a change to the reading code of modpost. Are there any other consumers of Module.symvers that we should be aware of?
>Regardless of the chosen format, I think we need to document how >namespaces are represented in Documentation/kbuild/modules.rst, where >it describes the Module.symvers format.
Agreed. I will update the documentation. It anyway needs an update.
Cheers, Matthias
>>+ symbol->module->name, >>+ export_str(symbol->export)); >>+ } >> symbol = symbol->next; >> } >> } >>diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h >>index 8453d6ac2f77..9626bf3e7424 100644 >>--- a/scripts/mod/modpost.h >>+++ b/scripts/mod/modpost.h >>@@ -109,6 +109,11 @@ buf_printf(struct buffer *buf, const char *fmt, ...); >>void >>buf_write(struct buffer *buf, const char *s, int len); >> >>+struct namespace_list { >>+ struct namespace_list *next; >>+ char namespace[0]; >>+}; >>+ >>struct module { >> struct module *next; >> const char *name; >>@@ -121,6 +126,8 @@ struct module { >> struct buffer dev_table_buf; >> char srcversion[25]; >> int is_dot_o; >>+ // Actual imported namespaces >>+ struct namespace_list *imported_namespaces; >>}; >> >>struct elf_info { >>-- >>2.23.0.rc1.153.gdeed80330f-goog >>
| |