Messages in this thread | | | Date | Fri, 27 Sep 2019 09:03:46 +0100 | From | Matthias Maennich <> | Subject | Re: [PATCH] modpost: Copy namespace string into 'struct symbol' |
| |
On Thu, Sep 26, 2019 at 05:24:46PM -0500, Shaun Ruffell wrote: >When building an out-of-tree module I was receiving many warnings from >modpost like: > > WARNING: module dahdi_vpmadt032_loader uses symbol __kmalloc from namespace ts/dahdi-linux/drivers/dahdi/dahdi-version.o: ..., but does not import it. > WARNING: module dahdi_vpmadt032_loader uses symbol vpmadtreg_register from namespace linux/drivers/dahdi/dahdi-version.o: ..., but does not import it. > WARNING: module dahdi_vpmadt032_loader uses symbol param_ops_int from namespace ahdi-linux/drivers/dahdi/dahdi-version.o: ..., but does not import it. > WARNING: module dahdi_vpmadt032_loader uses symbol __init_waitqueue_head from namespace ux/drivers/dahdi/dahdi-version.o: ..., but does not import it. > ... > >The fundamental issue appears to be that read_dump() is passing a >pointer to a statically allocated buffer for the namespace which is >reused as the file is parsed.
Hi Shaun,
Thanks for working on this. I think you are right about the root cause of this. I will have a closer look at your fix later today.
>This change makes it so that 'struct symbol' holds a copy of the >namespace string in the same way that it holds a copy of the symbol >string. Because a copy is being made, handle_modversion can now free the >temporary copy > >Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces") >Cc: Martijn Coenen <maco@android.com> >Cc: Joel Fernandes (Google) <joel@joelfernandes.org> >Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >Cc: Matthias Maennich <maennich@google.com> >Cc: Jessica Yu <jeyu@kernel.org> >Signed-off-by: Shaun Ruffell <sruffell@sruffell.net> >--- > >Hi, > >I didn't test that this change works with the namespaces, or investigate why >read_dump() is only called first while building out-of-tree modules, but it does >seem correct to me for the symbol to own the memory backing the namespace >string. > >I also realize I'm jumping the gun a bit by testing against master before >5.4-rc1 is tagged. > >Shaun > > scripts/mod/modpost.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > >diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c >index 3961941e8e7a..349832ead200 100644 >--- a/scripts/mod/modpost.c >+++ b/scripts/mod/modpost.c >@@ -364,6 +364,24 @@ static const char *sym_extract_namespace(const char **symname) > return NULL; > } > >+static const char *dup_namespace(const char *namespace) >+{ >+ if (!namespace || (namespace[0] == '\0')) >+ return NULL; >+ return NOFAIL(strdup(namespace)); >+} >+ >+static bool is_equal(const char *n1, const char *n2) >+{ >+ if (n1 && !n2) >+ return false; >+ if (!n1 && n2) >+ return false; >+ if (!n1 && !n2) >+ return true; >+ return strcmp(n1, n2) == 0; >+} >+ > /** > * Add an exported symbol - it may have already been added without a > * CRC, in this case just update the CRC >@@ -375,7 +393,7 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace, > > if (!s) { > s = new_symbol(name, mod, export); >- s->namespace = namespace; >+ s->namespace = dup_namespace(namespace); > } else { > if (!s->preloaded) { > warn("%s: '%s' exported twice. Previous export was in %s%s\n", >@@ -384,6 +402,12 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace, > } else { > /* In case Module.symvers was out of date */ > s->module = mod; >+ >+ /* In case the namespace was out of date */ >+ if (!is_equal(s->namespace, namespace)) { >+ free((char *)s->namespace); >+ s->namespace = dup_namespace(namespace); >+ } > } > } > s->preloaded = 0; >@@ -672,7 +696,6 @@ static void handle_modversions(struct module *mod, struct elf_info *info, > unsigned int crc; > enum export export; > bool is_crc = false; >- const char *name, *namespace; > > if ((!is_vmlinux(mod->name) || mod->is_dot_o) && > strstarts(symname, "__ksymtab")) >@@ -744,9 +767,13 @@ static void handle_modversions(struct module *mod, struct elf_info *info, > default: > /* All exported symbols */ > if (strstarts(symname, "__ksymtab_")) { >+ const char *name, *namespace; >+ > name = symname + strlen("__ksymtab_"); > namespace = sym_extract_namespace(&name); > sym_add_exported(name, namespace, mod, export); >+ if (namespace) >+ free((char *)name);
This probably should free namespace instead.
> } > if (strcmp(symname, "init_module") == 0) > mod->has_init = 1; >-- >2.17.1
Cheers, Matthias
| |