Re: [PATCH v3 01/15] modpost: mitigate false-negatives for static EXPORT_SYMBOL checks

[Nicolas Schier]

On Thu, May 05, 2022 at 04:22:30PM +0900 Masahiro Yamada wrote:
> The 'static' specifier and EXPORT_SYMBOL() are an odd combination.
> 
> Since commit 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL*
> functions"), modpost tries to detect it, but there are false negatives.
> 
> Here is the sample code.
> 
> [Sample 1]
> 
>   Makefile:
> 
>     obj-m += mymod1.o mymod2.o
> 
>   mymod1.c:
> 
>     #include <linux/export.h>
>     #include <linux/module.h>
>     static void foo(void) {}
>     EXPORT_SYMBOL(foo);
>     MODULE_LICENSE("GPL");
> 
>   mymod2.c:
> 
>     #include <linux/module.h>
>     void foo(void) {}
>     MODULE_LICENSE("GPL");
> 
> mymod1 exports the static symbol 'foo', but modpost cannot catch it
> because it is fooled by the same name symbol in another module, mymod2.
> (Without mymod2, modpost can detect the error in mymod1)
> 
> find_symbol() returns the first symbol found in the hash table with the
> given name. This hash table is global, so it may return a symbol from
> an unrelated module. So, a global symbol in a module may unset the
> 'is_static' flag of another module.
> 
> To mitigate this issue, add sym_find_with_module(), which receives the
> module pointer as the second argument. If non-NULL pointer is passed, it
> returns the symbol in the specified module. If NULL is passed, it is
> equivalent to find_module().
> 
> Please note there are still false positives in the composite module,
> like below (or when both are built-in). I have no idea how to do this
> correctly.
> 
> [Sample 2]  (not fixed by this commit)
> 
>   Makefile:
>     obj-m += mymod.o
>     mymod-objs := mymod1.o mymod2.o
> 
> Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> ---

I like the detailed commit description!

Reviewed-by: Nicolas Schier <nicolas@xxxxxxxxx>

> 
> (no changes since v2)
> 
> Changes in v2:
>   - Rename the new func to sym_find_with_module()
> 
>  scripts/mod/modpost.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index b605f4a58759..a55fa2b88a9a 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -272,7 +272,7 @@ static void sym_add_unresolved(const char *name, struct module *mod, bool weak)
>  	list_add_tail(&sym->list, &mod->unresolved_symbols);
>  }
>  
> -static struct symbol *find_symbol(const char *name)
> +static struct symbol *sym_find_with_module(const char *name, struct module *mod)
>  {
>  	struct symbol *s;
>  
> @@ -281,12 +281,17 @@ static struct symbol *find_symbol(const char *name)
>  		name++;
>  
>  	for (s = symbolhash[tdb_hash(name) % SYMBOL_HASH_SIZE]; s; s = s->next) {
> -		if (strcmp(s->name, name) == 0)
> +		if (strcmp(s->name, name) == 0 && (!mod || s->module == mod))
>  			return s;
>  	}
>  	return NULL;
>  }
>  
> +static struct symbol *find_symbol(const char *name)
> +{
> +	return sym_find_with_module(name, NULL);
> +}
> +
>  struct namespace_list {
>  	struct list_head list;
>  	char namespace[];
> @@ -2063,8 +2068,9 @@ static void read_symbols(const char *modname)
>  
>  		if (bind == STB_GLOBAL || bind == STB_WEAK) {
>  			struct symbol *s =
> -				find_symbol(remove_dot(info.strtab +
> -						       sym->st_name));
> +				sym_find_with_module(remove_dot(info.strtab +
> +								sym->st_name),
> +						     mod);
>  
>  			if (s)
>  				s->is_static = false;
> -- 
> 2.32.0

-- 
epost|xmpp: nicolas@xxxxxxxxx          irc://oftc.net/nsc
↳ gpg: 18ed 52db e34f 860e e9fb  c82b 7d97 0932 55a0 ce7f
     -- frykten for herren er opphav til kunnskap --