diff mbox series

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

Message ID 20220505072244.1155033-2-masahiroy@kernel.org (mailing list archive)
State New
Headers show
Series kbuild: yet another series of cleanups (modpost, LTO, MODULE_REL_CRCS) | expand

Commit Message

Masahiro Yamada May 5, 2022, 7:22 a.m. UTC
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@kernel.org>
---

(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(-)

Comments

Nicolas Schier May 5, 2022, 7:25 p.m. UTC | #1
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@kernel.org>
> ---

I like the detailed commit description!

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>

> 
> (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
diff mbox series

Patch

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;