On Tue, Oct 01, 2019 at 05:19:23PM +0100, Matthias Maennich wrote:
> On Mon, Sep 30, 2019 at 04:20:46PM -0500, Shaun Ruffell wrote:
> > 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.
>
> In the meantime, Masahiro came up with an alternative approach to
> address this problem:
> https://lore.kernel.org/lkml/[email protected]/
> How do you think about it? It ignores the memory allocation problem that
> you addressed as modpost is a host tool after all. As part of the patch
> series, an alternative format for the namespace ksymtab entry is
> suggested that also changes the way modpost has to deal with it.
Masahiro's patch set looks good to me.
My only comment would be that I felt it preferable for
sym_add_exported() to treat the two string arguments passed to it the
same way. I feel the way it is currently violates the princple of least
surprise. However I accept this is just my personal opinion.
> > > > @@ -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.
>
> Yeah, you are right. I was just noticing the inconsistency and thought
> it was obviously wrong. So, I was wrong. Sorry and thanks for the
> explanation.
>
> >
> > 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.
>
> I would rather follow-up with Masahiro's approach for now. What do you
> think?
I agree that following-up with Masahiro's patch set is the better
option.
Cheers,
Shaun
