Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 27 Sep 2019 09:03:46 +0100
From:   Matthias Maennich <maennich@google.com>
To:     Shaun Ruffell <sruffell@sruffell.net>
Cc:     linux-kernel@vger.kernel.org, Martijn Coenen <maco@android.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jessica Yu <jeyu@kernel.org>
Subject: Re: [PATCH] modpost: Copy namespace string into 'struct symbol'
Message-ID: <20190927080346.GC90796@google.com>
References: <20190906103235.197072-5-maennich@google.com>
 <20190926222446.30510-1-sruffell@sruffell.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20190926222446.30510-1-sruffell@sruffell.net>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@android.com>
>Cc: Joel Fernandes (Google) <joel@joelfernandes.org>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Cc: Matthias Maennich <maennich@google.com>
>Cc: Jessica Yu <jeyu@kernel.org>
>Signed-off-by: Shaun Ruffell <sruffell@sruffell.net>
>---
>
>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