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:   Thu, 3 Oct 2019 01:29:27 -0500
From:   Shaun Ruffell <sruffell@sruffell.net>
To:     Masahiro Yamada <yamada.masahiro@socionext.com>
Cc:     Jessica Yu <jeyu@kernel.org>,
        Matthias Maennich <maennich@google.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kbuild@vger.kernel.org,
        Michal Marek <michal.lkml@markovi.net>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/6] modpost: fix broken sym->namespace for external
 module builds
Message-ID: <20191003062927.4pusu3sfjyfayigy@sruffell.net>
References: <20191003075826.7478-1-yamada.masahiro@socionext.com>
 <20191003075826.7478-3-yamada.masahiro@socionext.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191003075826.7478-3-yamada.masahiro@socionext.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Oct 03, 2019 at 04:58:22PM +0900, Masahiro Yamada wrote:
> Currently, external module builds produce tons of false-positives:
> 
>   WARNING: module <mod> uses symbol <sym> from namespace <ns>, but does not import it.
> 
> Here, the <ns> part shows a random string.
> 
> When you build external modules, the symbol info of vmlinux and
> in-kernel modules are read from $(objtree)/Module.symvers, but
> read_dump() is buggy in multiple ways:
> 
> [1] When the modpost is run for vmlinux and in-kernel modules,
> sym_extract_namespace() allocates memory for the namespace. On the
> other hand, read_dump() does not, then sym->namespace will point to
> somewhere in the line buffer of get_next_line(). The data in the
> buffer will be replaced soon, and sym->namespace will end up with
> pointing to unrelated data. As a result, check_exports() will show
> random strings in the warning messages.
> 
> [2] When there is no namespace, sym_extract_namespace() returns NULL.
> On the other hand, read_dump() sets namespace to an empty string "".
> (but, it will be later replaced with unrelated data due to bug [1].)
> The check_exports() shows a warning unless exp->namespace is NULL,
> so every symbol read from read_dump() emits the warning, which is
> mostly false positive.
> 
> To address [1], sym_add_exported() calls strdup() for s->namespace.
> The namespace from sym_extract_namespace() must be freed to avoid
> memory leak.
> 
> For [2], I changed the if-conditional in check_exports().
> 
> This commit also fixes sym_add_exported() to set s->namespace correctly
> when the symbol is preloaded.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Reviewed-by: Matthias Maennich <maennich@google.com>
> ---
> 
> Changes in v2:
>   - Change the approach to deal with ->preloaded
> 
>  scripts/mod/modpost.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 2c644086c412..936d3ad23c83 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -166,7 +166,7 @@ struct symbol {
>  	struct module *module;
>  	unsigned int crc;
>  	int crc_valid;
> -	const char *namespace;
> +	char *namespace;
>  	unsigned int weak:1;
>  	unsigned int vmlinux:1;    /* 1 if symbol is defined in vmlinux */
>  	unsigned int kernel:1;     /* 1 if symbol is from kernel
> @@ -348,7 +348,7 @@ static enum export export_from_sec(struct elf_info *elf, unsigned int sec)
>  		return export_unknown;
>  }
>  
> -static const char *sym_extract_namespace(const char **symname)
> +static char *sym_extract_namespace(const char **symname)
>  {
>  	char *namespace = NULL;
>  	char *ns_separator;
> @@ -373,7 +373,6 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
>  
>  	if (!s) {
>  		s = new_symbol(name, mod, export);
> -		s->namespace = namespace;
>  	} else {
>  		if (!s->preloaded) {
>  			warn("%s: '%s' exported twice. Previous export was in %s%s\n",
> @@ -384,6 +383,8 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
>  			s->module = mod;
>  		}
>  	}
> +	free(s->namespace);
> +	s->namespace = namespace ? strdup(namespace) : NULL;
>  	s->preloaded = 0;
>  	s->vmlinux   = is_vmlinux(mod->name);
>  	s->kernel    = 0;
> @@ -670,7 +671,8 @@ 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;
> +	const char *name;
> +	char *namespace;
>  
>  	if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
>  	    strstarts(symname, "__ksymtab"))
> @@ -745,6 +747,7 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
>  			name = symname + strlen("__ksymtab_");
>  			namespace = sym_extract_namespace(&name);
>  			sym_add_exported(name, namespace, mod, export);
> +			free(namespace);
>  		}
>  		if (strcmp(symname, "init_module") == 0)
>  			mod->has_init = 1;
> @@ -2193,7 +2196,7 @@ static int check_exports(struct module *mod)
>  		else
>  			basename = mod->name;
>  
> -		if (exp->namespace) {
> +		if (exp->namespace && exp->namespace[0]) {
>  			add_namespace(&mod->required_namespaces,
>  				      exp->namespace);
>  

This looks good to me and is better than what I had originally proposed.
I confirmed that I can still build an external module without any
warnings. (But I did have to convince myself that it was OK to store
empty namespace strings in the symbol structure and that check_exports()
would cover it sufficiently)

If you would like, feel free to add my

Reviewed-by: Shaun Ruffell <sruffell@sruffell.net>
  or
Tested-by: Shaun Ruffell <sruffell@sruffell.net>