Messages in this thread | | | Date | Mon, 30 Sep 2019 16:20:46 -0500 | From | Shaun Ruffell <> | Subject | Re: [PATCH] modpost: Copy namespace string into 'struct symbol' |
| |
On Fri, Sep 27, 2019 at 09:03:46AM +0100, Matthias Maennich wrote: > 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.
Thanks Matthias.
> > @@ -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.
Given the implementation of sym_extract_namespace below, I believe free((char *)name) is correct.
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; }
I agree that freeing name instead of namespace is a little surprising unless you know the implementation of sym_extract_namespace.
I thought about changing the the signature of sym_extract_namespace() to make it clear when the symname is used to return a new allocation or not, and given your comment, perhaps I should have.
| |