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

[Matthias Maennich]

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.

> This change makes it so that 'struct symbol' holds a copy of the
> namespace string in the same way that it holds a copy of the symbol
> string. Because a copy is being made, handle_modversion can now free the
> temporary copy
>
> Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
> Cc: Martijn Coenen <maco@xxxxxxxxxxx>
> Cc: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Matthias Maennich <maennich@xxxxxxxxxx>
> Cc: Jessica Yu <jeyu@xxxxxxxxxx>
> Signed-off-by: Shaun Ruffell <sruffell@xxxxxxxxxxxx>
> ---
>
> Hi,
>
> I didn't test that this change works with the namespaces, or investigate why
> read_dump() is only called first while building out-of-tree modules, but it does
> seem correct to me for the symbol to own the memory backing the namespace
> string.
>
> I also realize I'm jumping the gun a bit by testing against master before
> 5.4-rc1 is tagged.
>
> Shaun
>
> scripts/mod/modpost.c | 31 +++++++++++++++++++++++++++++--
> 1 file changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 3961941e8e7a..349832ead200 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -364,6 +364,24 @@ static const char *sym_extract_namespace(const char **symname)
> 	return NULL;
> }
>
> +static const char *dup_namespace(const char *namespace)
> +{
> +	if (!namespace || (namespace[0] == '\0'))
> +		return NULL;
> +	return NOFAIL(strdup(namespace));
> +}
> +
> +static bool is_equal(const char *n1, const char *n2)
> +{
> +	if (n1 && !n2)
> +		return false;
> +	if (!n1 && n2)
> +		return false;
> +	if (!n1 && !n2)
> +		return true;
> +	return strcmp(n1, n2) == 0;
> +}
> +
> /**
>  * Add an exported symbol - it may have already been added without a
>  * CRC, in this case just update the CRC
> @@ -375,7 +393,7 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
>
> 	if (!s) {
> 		s = new_symbol(name, mod, export);
> -		s->namespace = namespace;
> +		s->namespace = dup_namespace(namespace);
> 	} else {
> 		if (!s->preloaded) {
> 			warn("%s: '%s' exported twice. Previous export was in %s%s\n",
> @@ -384,6 +402,12 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
> 		} else {
> 			/* In case Module.symvers was out of date */
> 			s->module = mod;
> +
> +			/* In case the namespace was out of date */
> +			if (!is_equal(s->namespace, namespace)) {
> +				free((char *)s->namespace);
> +				s->namespace = dup_namespace(namespace);
> +			}
> 		}
> 	}
> 	s->preloaded = 0;
> @@ -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.

> 		}
> 		if (strcmp(symname, "init_module") == 0)
> 			mod->has_init = 1;
> --
> 2.17.1

Cheers,
Matthias