Re: [PATCH] modpost: Copy namespace string into 'struct symbol'

[Shaun Ruffell]

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@xxxxxxxxxxxxx/
> 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