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

[Shaun Ruffell]

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.