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.
