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:   Tue, 1 Oct 2019 17:19:23 +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>,
        Masahiro Yamada <yamada.masahiro@socionext.com>
Subject: Re: [PATCH] modpost: Copy namespace string into 'struct symbol'
Message-ID: <20191001161923.GH90796@google.com>
References: <20190906103235.197072-5-maennich@google.com>
 <20190926222446.30510-1-sruffell@sruffell.net>
 <20190927080346.GC90796@google.com>
 <20190930212046.rhtqmnueoffe7dnr@sruffell.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20190930212046.rhtqmnueoffe7dnr@sruffell.net>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@socionext.com/
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.

>> > @@ -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?

Cheers,
Matthias