Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 1 Oct 2019 14:37:04 -0500 | From | Shaun Ruffell <> | Subject | Re: [PATCH] modpost: Copy namespace string into 'struct symbol' |
| |
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/20190927093603.9140-2-yamada.masahiro@socionext.com/ > 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
| ![\](/images/icornerr.gif) |