modpost: check for static EXPORT_SYMBOL* functions

RFC PATCH In review Denis Efremov 4 participants 32 messages Jul 14, 2019 – Aug 13, 2019

Subsystems (2)

x86 Kbuild

lore

Share & tools

lore

Denis Efremov

author

2019-07-14 15:28 UTC #0 lore raw

RFC PATCH 1/1 modpost: check for static EXPORT_SYMBOL* functions

This patch adds a check to warn about static EXPORT_SYMBOL* functions during the modpost. In most of the cases, a static symbol marked for exporting is an odd combination that should be fixed either by deleting the exporting mark or by removing the static attribute and adding the appropriate declaration to headers.

If this check will be considered useful, I will resend the patch with review fixes.

Currently, this check emits the warnings on the following symbols, most of which are accepted to be fixed:
1. "sas_wait_eh" [drivers/scsi/libsas/libsas]
Patch: https://lkml.org/lkml/2019/7/8/970 (accepted)
2. "torture_onoff_cleanup" [kernel/torture] "torture_shuffle_cleanup" [kernel/torture]
Patch: https://lkml.org/lkml/2019/7/4/411 (accepted)
3. "LZ4HC_setExternalDict" [lib/lz4/lz4hc_compress]
Patch: https://lkml.org/lkml/2019/7/8/842
4. "drm_client_close" [drivers/gpu/drm/drm]
Patch: https://lkml.org/lkml/2019/7/3/758 (accepted)
5. "gve_probe" [drivers/net/ethernet/google/gve/gve]
Patch: https://lkml.org/lkml/2019/7/14/65
6. "i2c_new_client_device" [vmlinux]
"i2c_new_dummy_device" [vmlinux]
Patch: https://lkml.org/lkml/2019/7/7/226 (fixed in a different patch)
7. "ahci_em_messages" [drivers/ata/libahci]
Patch: https://lkml.org/lkml/2019/7/10/550 (reviwed)
8. "ftrace_set_clr_event" [vmlinux]
Patch: https://lkml.org/lkml/2019/7/4/609 (reviwed)
9. "rmi_2d_sensor_set_input_params" [drivers/input/rmi4/rmi_core]
Patch: https://lkml.org/lkml/2019/7/8/999
10. "empty_zero_page" [vmlinux]
11. "phys_base" [vmlinux]
12. "hypercall_page" [vmlinux]

Similar commits:
1. 54638c6eaf44 ("net: phy: make exported variables non-static")
2. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
3. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration")
4. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
5. e4e4730698c9 ("crypto: skcipher - remove the exporting of skcipher_walk_next")

Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x):

$ make mrproper; make allmodconfig; time make -j12; \
  git checkout HEAD~1; \
  make mrproper; make allmodconfig; time make -j12
1.
   (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total
   (w/o  patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total
2.
   (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total
   (w/o  patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total
3.
   (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total
   (w/o  patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total

Thus, the current implementation adds approx. 1 min for allmodconfig. However, it's possible to do the check in a more optimal way if it will be considered useful.

Also, this kind of check could be implemented as a separate script instead. Here is the implementation:
https://gist.github.com/evdenis/bf2322d094f0c02c0f60fe0a225848b2

Signed-off-by Denis Efremov <[email protected]>

---
 scripts/mod/modpost.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

scripts/mod/modpost.c +33 -2

index f277e116e0eb..c51eef357721 100644
@@ -169,6 +169,7 @@ struct symbol {
 	unsigned int kernel:1;     /* 1 if symbol is from kernel
 				    *  (only for external modules) **/
 	unsigned int preloaded:1;  /* 1 if symbol from Module.symvers, or crc */
+	unsigned int is_static:1;  /* 1 if symbol is not global */
 	enum export  export;       /* Type of export */
 	char name[0];
 };

@@ -199,8 +200,9 @@ static struct symbol *alloc_symbol(const char *name, unsigned int weak,
 
 	memset(s, 0, sizeof(*s));
 	strcpy(s->name, name);
-	s->weak = weak;
-	s->next = next;
+	s->weak      = weak;
+	s->next      = next;
+	s->is_static = 1;
 	return s;
 }

@@ -1980,6 +1982,21 @@ static void read_symbols(const char *modname)
 		handle_modversions(mod, &info, sym, symname);
 		handle_moddevtable(mod, &info, sym, symname);
 	}
+
+	// check for static EXPORT_SYMBOL_* functions && global vars
+	for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
+		unsigned char bind = ELF_ST_BIND(sym->st_info);
+		unsigned char type = ELF_ST_TYPE(sym->st_info);
+
+		if (type == STT_OBJECT || type == STT_FUNC) {
+			struct symbol *s =
+			    find_symbol(remove_dot(info.strtab + sym->st_name));
+
+			if (s && (bind == STB_GLOBAL || bind == STB_WEAK))
+				s->is_static = 0;
+		}
+	}
+
 	if (!is_vmlinux(modname) || vmlinux_section_warnings)
 		check_sec_ref(mod, modname, &info);

@@ -2425,6 +2442,7 @@ int main(int argc, char **argv)
 	char *dump_write = NULL, *files_source = NULL;
 	int opt;
 	int err;
+	size_t n;
 	struct ext_sym_list *extsym_iter;
 	struct ext_sym_list *extsym_start = NULL;

@@ -2520,6 +2538,19 @@ int main(int argc, char **argv)
 	if (sec_mismatch_count && sec_mismatch_fatal)
 		fatal("modpost: Section mismatches detected.\n"
 		      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
+	for (n = 0; n < SYMBOL_HASH_SIZE ; n++) {
+		struct symbol *s = symbolhash[n];
+
+		while (s) {
+			if (s->is_static)
+				warn("\"%s\" [%s] is a static %s symbol\n",
+					s->name, s->module->name,
+						export_str(s->export));
+
+			s = s->next;
+		}
+	}
+
 	free(buf.p);
 
 	return err;

Signature · 1 line

2.21.0

Emil Velikov

Acked-by on v1 1/1

2019-07-15 14:43 UTC #1 lore raw

Hi Denis,

Quoted 56 lines · Denis Efremov

This patch adds a check to warn about static EXPORT_SYMBOL* functions during the modpost. In most of the cases, a static symbol marked for exporting is an odd combination that should be fixed either by deleting the exporting mark or by removing the static attribute and adding the appropriate declaration to headers.
If this check will be considered useful, I will resend the patch with review fixes.
Currently, this check emits the warnings on the following symbols, most of which are accepted to be fixed:
1. "sas_wait_eh" [drivers/scsi/libsas/libsas]
Patch: https://lkml.org/lkml/2019/7/8/970 (accepted)
2. "torture_onoff_cleanup" [kernel/torture] "torture_shuffle_cleanup" [kernel/torture]
Patch: https://lkml.org/lkml/2019/7/4/411 (accepted)
3. "LZ4HC_setExternalDict" [lib/lz4/lz4hc_compress]
Patch: https://lkml.org/lkml/2019/7/8/842
4. "drm_client_close" [drivers/gpu/drm/drm]
Patch: https://lkml.org/lkml/2019/7/3/758 (accepted)
5. "gve_probe" [drivers/net/ethernet/google/gve/gve]
Patch: https://lkml.org/lkml/2019/7/14/65
6. "i2c_new_client_device" [vmlinux]
"i2c_new_dummy_device" [vmlinux]
Patch: https://lkml.org/lkml/2019/7/7/226 (fixed in a different patch)
7. "ahci_em_messages" [drivers/ata/libahci]
Patch: https://lkml.org/lkml/2019/7/10/550 (reviwed)
8. "ftrace_set_clr_event" [vmlinux]
Patch: https://lkml.org/lkml/2019/7/4/609 (reviwed)
9. "rmi_2d_sensor_set_input_params" [drivers/input/rmi4/rmi_core]
Patch: https://lkml.org/lkml/2019/7/8/999
10. "empty_zero_page" [vmlinux]
11. "phys_base" [vmlinux]
12. "hypercall_page" [vmlinux]
Similar commits:
1. 54638c6eaf44 ("net: phy: make exported variables non-static")
2. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
3. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration")
4. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
5. e4e4730698c9 ("crypto: skcipher - remove the exporting of skcipher_walk_next")
Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x):
$ make mrproper; make allmodconfig; time make -j12; \
  git checkout HEAD~1; \
  make mrproper; make allmodconfig; time make -j12
1.
   (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total
   (w/o  patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total
2.
   (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total
   (w/o  patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total
3.
   (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total
   (w/o  patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total

Quoted 8 lines

Thus, the current implementation adds approx. 1 min for allmodconfig. However, it's possible to do the check in a more optimal way if it will be considered useful.
Also, this kind of check could be implemented as a separate script instead. Here is the implementation:
https://gist.github.com/evdenis/bf2322d094f0c02c0f60fe0a225848b2

Personally I think this is a pretty good feature.

If I did my numbers correctly, the above numbers show ~2% increase. Although one should be able to reduce that if people feel too strongly.

That said, the patch is:

Acked-by Emil Velikov <[email protected]>

Can we make sure that patches for all issues are out (on the respective mailing lists, or merged) before this lands.

HTH
Emil

Masahiro Yamada

2019-07-27 12:56 UTC #2 lore raw

In reply to Denis Efremov

Hi.

Thanks, this patch is very nice.

Quoted 27 lines · Denis Efremov

This patch adds a check to warn about static EXPORT_SYMBOL* functions during the modpost. In most of the cases, a static symbol marked for exporting is an odd combination that should be fixed either by deleting the exporting mark or by removing the static attribute and adding the appropriate declaration to headers.
If this check will be considered useful, I will resend the patch with review fixes.
Currently, this check emits the warnings on the following symbols, most of which are accepted to be fixed:
1. "sas_wait_eh" [drivers/scsi/libsas/libsas]
Patch: https://lkml.org/lkml/2019/7/8/970 (accepted)
2. "torture_onoff_cleanup" [kernel/torture] "torture_shuffle_cleanup" [kernel/torture]
Patch: https://lkml.org/lkml/2019/7/4/411 (accepted)
3. "LZ4HC_setExternalDict" [lib/lz4/lz4hc_compress]
Patch: https://lkml.org/lkml/2019/7/8/842
4. "drm_client_close" [drivers/gpu/drm/drm]
Patch: https://lkml.org/lkml/2019/7/3/758 (accepted)
5. "gve_probe" [drivers/net/ethernet/google/gve/gve]
Patch: https://lkml.org/lkml/2019/7/14/65
6. "i2c_new_client_device" [vmlinux]
"i2c_new_dummy_device" [vmlinux]
Patch: https://lkml.org/lkml/2019/7/7/226 (fixed in a different patch)
7. "ahci_em_messages" [drivers/ata/libahci]

Quoted 8 lines

Patch: https://lkml.org/lkml/2019/7/10/550 (reviwed)
8. "ftrace_set_clr_event" [vmlinux]
Patch: https://lkml.org/lkml/2019/7/4/609 (reviwed)
9. "rmi_2d_sensor_set_input_params" [drivers/input/rmi4/rmi_core]
Patch: https://lkml.org/lkml/2019/7/8/999
10. "empty_zero_page" [vmlinux]
11. "phys_base" [vmlinux]
12. "hypercall_page" [vmlinux]

Could you drop the solved ones from the list?

I still see the following at least, but I will apply this patch anyway.

WARNING: "phys_base" [vmlinux] is a static EXPORT_SYMBOL symbol
WARNING: "drm_client_close" [vmlinux] is a static EXPORT_SYMBOL symbol
WARNING: "ahci_em_messages" [vmlinux] is a static EXPORT_SYMBOL_GPL symbol
WARNING: "ftrace_set_clr_event" [vmlinux] is a static EXPORT_SYMBOL_GPL symbol
WARNING: "empty_zero_page" [vmlinux] is a static EXPORT_SYMBOL symbol

The code looks good to me.
I commented on some minor style issues.

Quoted 34 lines

Similar commits:
1. 54638c6eaf44 ("net: phy: make exported variables non-static")
2. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
3. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration")
4. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
5. e4e4730698c9 ("crypto: skcipher - remove the exporting of skcipher_walk_next")
Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x):
$ make mrproper; make allmodconfig; time make -j12; \
  git checkout HEAD~1; \
  make mrproper; make allmodconfig; time make -j12
1.
   (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total
   (w/o  patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total
2.
   (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total
   (w/o  patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total
3.
   (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total
   (w/o  patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total
Thus, the current implementation adds approx. 1 min for allmodconfig. However, it's possible to do the check in a more optimal way if it will be considered useful.
Also, this kind of check could be implemented as a separate script instead. Here is the implementation:
https://gist.github.com/evdenis/bf2322d094f0c02c0f60fe0a225848b2
Signed-off-by Denis Efremov <[email protected]>

scripts/mod/modpost.c static struct symbol *alloc_symbol(const char *name, unsigned int weak, · line 204

Quoted 7 lines

scripts/mod/modpost.c +2 -2

@@ -199,8 +200,9 @@ static struct symbol *alloc_symbol(const char *name, unsigned int weak,
 
        memset(s, 0, sizeof(*s));
        strcpy(s->name, name);
-       s->weak = weak;
-       s->next = next;
+       s->weak      = weak;
+       s->next      = next;

Could you drop this change?
I do not think we need to align the '=' operator.

Quoted 36 lines

scripts/mod/modpost.c +17 -0

+       s->is_static = 1;
        return s;
 }

@@ -1980,6 +1982,21 @@ static void read_symbols(const char *modname)
                handle_modversions(mod, &info, sym, symname);
                handle_moddevtable(mod, &info, sym, symname);
        }
+
+       // check for static EXPORT_SYMBOL_* functions && global vars
+       for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
+               unsigned char bind = ELF_ST_BIND(sym->st_info);
+               unsigned char type = ELF_ST_TYPE(sym->st_info);
+
+               if (type == STT_OBJECT || type == STT_FUNC) {
+                       struct symbol *s =
+                           find_symbol(remove_dot(info.strtab + sym->st_name));
+
+                       if (s && (bind == STB_GLOBAL || bind == STB_WEAK))
+                               s->is_static = 0;
+               }
+       }
+
        if (!is_vmlinux(modname) || vmlinux_section_warnings)
                check_sec_ref(mod, modname, &info);

@@ -2425,6 +2442,7 @@ int main(int argc, char **argv)
        char *dump_write = NULL, *files_source = NULL;
        int opt;
        int err;
+       size_t n;
        struct ext_sym_list *extsym_iter;
        struct ext_sym_list *extsym_start = NULL;

@@ -2520,6 +2538,19 @@ int main(int argc, char **argv)
        if (sec_mismatch_count && sec_mismatch_fatal)

scripts/mod/modpost.c int main(int argc, char **argv) · line 2548

Quoted 10 lines

scripts/mod/modpost.c +8 -0

                fatal("modpost: Section mismatches detected.\n"
                      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
+       for (n = 0; n < SYMBOL_HASH_SIZE ; n++) {
+               struct symbol *s = symbolhash[n];
+
+               while (s) {
+                       if (s->is_static)
+                               warn("\"%s\" [%s] is a static %s symbol\n",
+                                       s->name, s->module->name,
+                                               export_str(s->export));

Could you follow the checkpatch.pl suggestion?

CHECK: Alignment should match open parenthesis
#2550: FILE: scripts/mod/modpost.c:2550:
+ warn("\"%s\" [%s] is a static %s symbol\n",
+ s->name, s->module->name,

(This file already has tons of checkpatch warnings, but I want to try my best to not add new ones.)

Thanks.

Signature · 2 lines

Best Regards
Masahiro Yamada

Denis Efremov

author

2019-07-27 19:14 UTC #3 lore raw

Hi.

Quoted 1 line

Could you drop the solved ones from the list?

Yes, of course. Do you want me to remove all symbols fixed with patches or only those are in-tree now?

Should it be like this:
1. "torture_onoff_cleanup" [kernel/torture]

    "torture_shuffle_cleanup" [kernel/torture]
    Patch: https://lkml.org/lkml/2019/7/4/411 (accepted)
 2. "LZ4HC_setExternalDict" [lib/lz4/lz4hc_compress]
    Patch: https://lkml.org/lkml/2019/7/8/842
 3. "drm_client_close" [drivers/gpu/drm/drm]
    Patch: https://lkml.org/lkml/2019/7/3/758 (accepted)
 4. "ahci_em_messages" [drivers/ata/libahci]
    Patch: https://lkml.org/lkml/2019/7/10/550 (reviewed)
 5. "ftrace_set_clr_event" [vmlinux]
    Patch: https://lkml.org/lkml/2019/7/4/609 (reviewed)
 6. "rmi_2d_sensor_set_input_params" [drivers/input/rmi4/rmi_core]
    Patch: https://lkml.org/lkml/2019/7/8/999 (accepted)

10. "empty_zero_page" [vmlinux]
11. "phys_base" [vmlinux]
12. "hypercall_page" [vmlinux]

Or like this:
1. "empty_zero_page" [vmlinux]
2. "phys_base" [vmlinux]
3. "hypercall_page" [vmlinux]

Well, I could remove this list at all. It seems like the following list with existing commits is enough to show the problem of static exported functions.

I will resend the patch shortly after.

Regards,
Denis

Masahiro Yamada

2019-07-28 02:00 UTC #4 lore raw

Hi.

Quoted 26 lines · Denis Efremov

Hi.
Quoted 1 line
Could you drop the solved ones from the list?
Yes, of course. Do you want me to remove all symbols fixed with patches or only those are in-tree now?
Should it be like this:
1. "torture_onoff_cleanup" [kernel/torture]
     "torture_shuffle_cleanup" [kernel/torture]
     Patch: https://lkml.org/lkml/2019/7/4/411 (accepted)
  2. "LZ4HC_setExternalDict" [lib/lz4/lz4hc_compress]
     Patch: https://lkml.org/lkml/2019/7/8/842
  3. "drm_client_close" [drivers/gpu/drm/drm]
     Patch: https://lkml.org/lkml/2019/7/3/758 (accepted)
  4. "ahci_em_messages" [drivers/ata/libahci]
     Patch: https://lkml.org/lkml/2019/7/10/550 (reviewed)
  5. "ftrace_set_clr_event" [vmlinux]
     Patch: https://lkml.org/lkml/2019/7/4/609 (reviewed)
  6. "rmi_2d_sensor_set_input_params" [drivers/input/rmi4/rmi_core]
     Patch: https://lkml.org/lkml/2019/7/8/999 (accepted)
10. "empty_zero_page" [vmlinux]
11. "phys_base" [vmlinux]
12. "hypercall_page" [vmlinux]

Quoted 8 lines

Or like this:
1. "empty_zero_page" [vmlinux]
2. "phys_base" [vmlinux]
3. "hypercall_page" [vmlinux]
Well, I could remove this list at all. It seems like the following list with existing commits is enough to show the problem of static exported functions.

Yeah, I agree.
Showing some existing commits is enough.

Quoted 4 lines

I will resend the patch shortly after.
Regards,
Denis

Signature · 2 lines

Best Regards
Masahiro Yamada

Denis Efremov

author

2019-07-28 10:09 UTC #5 lore raw

PATCH 1/1 modpost: check for static EXPORT_SYMBOL* functions

In reply to Denis Efremov

This check could help to detect the following problems:
1. 550113d4e9f5 ("i2c: add newly exported functions to the header, too")
2. 54638c6eaf44 ("net: phy: make exported variables non-static")
3. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
4. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration")
5. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
6. e4e4730698c9 ("crypto: skcipher - remove the exporting of skcipher_walk_next")
7. 14b4c48bb1ce ("gve: Remove the exporting of gve_probe")
8. 9b79ee9773a8 ("scsi: libsas: remove the exporting of sas_wait_eh")
9. ...

Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x):

$ make mrproper; make allmodconfig; time make -j12; \
  git checkout HEAD~1; \
  make mrproper; make allmodconfig; time make -j12
1.
   (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total
   (w/o  patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total
2.
   (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total
   (w/o  patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total
3.
   (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total
   (w/o  patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total

Thus, the current implementation adds approx. 1 min for allmodconfig.

Acked-by Emil Velikov <[email protected]>

Signed-off-by Denis Efremov <[email protected]>

---
 scripts/mod/modpost.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

scripts/mod/modpost.c +32 -0

index f277e116e0eb..85e885235c96 100644
@@ -169,6 +169,7 @@ struct symbol {
 	unsigned int kernel:1;     /* 1 if symbol is from kernel
 				    *  (only for external modules) **/
 	unsigned int preloaded:1;  /* 1 if symbol from Module.symvers, or crc */
+	unsigned int is_static:1;  /* 1 if symbol is not global */
 	enum export  export;       /* Type of export */
 	char name[0];
 };

@@ -201,6 +202,7 @@ static struct symbol *alloc_symbol(const char *name, unsigned int weak,
 	strcpy(s->name, name);
 	s->weak = weak;
 	s->next = next;
+	s->is_static = 1;
 	return s;
 }

@@ -1980,6 +1982,22 @@ static void read_symbols(const char *modname)
 		handle_modversions(mod, &info, sym, symname);
 		handle_moddevtable(mod, &info, sym, symname);
 	}
+
+	// check for static EXPORT_SYMBOL_* functions && global vars
+	for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
+		unsigned char bind = ELF_ST_BIND(sym->st_info);
+		unsigned char type = ELF_ST_TYPE(sym->st_info);
+
+		if (type == STT_OBJECT || type == STT_FUNC) {
+			struct symbol *s =
+				find_symbol(remove_dot(info.strtab +
+						       sym->st_name));
+
+			if (s && (bind == STB_GLOBAL || bind == STB_WEAK))
+				s->is_static = 0;
+		}
+	}
+
 	if (!is_vmlinux(modname) || vmlinux_section_warnings)
 		check_sec_ref(mod, modname, &info);

@@ -2425,6 +2443,7 @@ int main(int argc, char **argv)
 	char *dump_write = NULL, *files_source = NULL;
 	int opt;
 	int err;
+	size_t n;
 	struct ext_sym_list *extsym_iter;
 	struct ext_sym_list *extsym_start = NULL;

@@ -2520,6 +2539,19 @@ int main(int argc, char **argv)
 	if (sec_mismatch_count && sec_mismatch_fatal)
 		fatal("modpost: Section mismatches detected.\n"
 		      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
+	for (n = 0; n < SYMBOL_HASH_SIZE; n++) {
+		struct symbol *s = symbolhash[n];
+
+		while (s) {
+			if (s->is_static)
+				warn("\"%s\" [%s] is the static %s\n",
+				     s->name, s->module->name,
+				     export_str(s->export));
+
+			s = s->next;
+		}
+	}
+
 	free(buf.p);
 
 	return err;

Signature · 1 line

2.21.0

Masahiro Yamada

2019-07-29 03:29 UTC #6 lore raw

Quoted 43 lines · Denis Efremov

This patch adds a check to warn about static EXPORT_SYMBOL* functions during the modpost. In most of the cases, a static symbol marked for exporting is an odd combination that should be fixed either by deleting the exporting mark or by removing the static attribute and adding the appropriate declaration to headers.
This check could help to detect the following problems:
1. 550113d4e9f5 ("i2c: add newly exported functions to the header, too")
2. 54638c6eaf44 ("net: phy: make exported variables non-static")
3. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
4. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration")
5. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
6. e4e4730698c9 ("crypto: skcipher - remove the exporting of skcipher_walk_next")
7. 14b4c48bb1ce ("gve: Remove the exporting of gve_probe")
8. 9b79ee9773a8 ("scsi: libsas: remove the exporting of sas_wait_eh")
9. ...
Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x):
$ make mrproper; make allmodconfig; time make -j12; \
  git checkout HEAD~1; \
  make mrproper; make allmodconfig; time make -j12
1.
   (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total
   (w/o  patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total
2.
   (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total
   (w/o  patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total
3.
   (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total
   (w/o  patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total
Thus, the current implementation adds approx. 1 min for allmodconfig.
Acked-by Emil Velikov <[email protected]>
Signed-off-by Denis Efremov <[email protected]>
---
 scripts/mod/modpost.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

scripts/mod/modpost.c int main(int argc, char **argv) · line 2446

Quoted 4 lines

scripts/mod/modpost.c +1 -0

@@ -2425,6 +2443,7 @@ int main(int argc, char **argv)
        char *dump_write = NULL, *files_source = NULL;
        int opt;
        int err;
+       size_t n;

Sorry, I missed to ask this in the previous version.

If there is not a particular reason,
may I ask you to use 'int' instead of 'size_t' here?

SYMBOL_HASH_SIZE (= 1024) is small enough, and it will keep consistency with
the write_dump() function in this file.

If it is tedious to send a new version, may I fix-up 'size_t' -> 'int' ?

Thanks.

Quoted 26 lines

scripts/mod/modpost.c +13 -1

        struct ext_sym_list *extsym_iter;
        struct ext_sym_list *extsym_start = NULL;

@@ -2520,6 +2539,19 @@ int main(int argc, char **argv)
        if (sec_mismatch_count && sec_mismatch_fatal)
                fatal("modpost: Section mismatches detected.\n"
                      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
+       for (n = 0; n < SYMBOL_HASH_SIZE; n++) {
+               struct symbol *s = symbolhash[n];
+
+               while (s) {
+                       if (s->is_static)
+                               warn("\"%s\" [%s] is the static %s\n",
+                                    s->name, s->module->name,
+                                    export_str(s->export));
+
+                       s = s->next;
+               }
+       }
+
        free(buf.p);
 
        return err;
--

2.21.0

Signature · 2 lines

Best Regards
Masahiro Yamada

Denis Efremov

author

2019-07-29 09:51 UTC #7 lore raw

Quoted 1 line · Masahiro Yamada

may I ask you to use 'int' instead of 'size_t' here?

Fixed in v2.

Regards,
Denis

Stephen Rothwell

2019-07-29 05:13 UTC #8 lore raw

In reply to Denis Efremov

Hi Denis,

Quoted 2 lines · On Sun, 28 Jul 2019 13:09:06 +0300 Denis Efremov <[email protected]> wrote:

Thus, the current implementation adds approx. 1 min for allmodconfig.

Just a reminder that some of us (just me?) do well over 100+ builds per day ... if this can be optimised some what that would be good.

Signature · 2 lines

Cheers,
Stephen Rothwell

Denis Efremov

author

2019-07-29 09:16 UTC #9 lore raw

Quoted 2 lines

Just a reminder that some of us (just me?) do well over 100+ builds per day ... if this can be optimised some what that would be good.

These measurements for the worst case (allmodconfig). Is it possible to measure the slowdown in your case? How it will perform on your typical workflow?

Looks like it is possible to optimize it, but I need some hints from Masahiro on how to do it properly. Because I don't know how to match __ksymtab_<symbol> with the <symbol> without an additional loop. Introduce another hash table?

The first loop from this patch could traverse only the exported symbols instead of all symbols. But in this case, I don't know how to break early from the loop because there can be many symbols with the same name but with the different scope (static/non-static).

For example, ring_buffer_size:
kernel/trace/ring_buffer.c
4334:unsigned long ring_buffer_size(struct ring_buffer *buffer, int cpu) 4347:EXPORT_SYMBOL_GPL(ring_buffer_size);
And
drivers/usb/misc/ldusb.c
125:static int ring_buffer_size = 128;

Or for, nfs4_disable_idmapping:
fs/nfs/super.c
2920:bool nfs4_disable_idmapping = true;
2930:EXPORT_SYMBOL_GPL(nfs4_disable_idmapping);
fs/nfsd/nfs4idmap.c
48:static bool nfs4_disable_idmapping = true;

Regards,
Denis

Masahiro Yamada

2019-07-29 09:33 UTC #10 lore raw

Quoted 11 lines · Denis Efremov

Quoted 3 lines
Just a reminder that some of us (just me?) do well over 100+ builds per day ... if this can be optimised some what that would be good.
These measurements for the worst case (allmodconfig). Is it possible to measure the slowdown in your case? How it will perform on your typical workflow?
Looks like it is possible to optimize it, but I need some hints from Masahiro on how to do it properly. Because I don't know how to match __ksymtab_<symbol> with the <symbol> without an additional loop.

Right.
This is not feasible without an additional loop since we put only exported symbols into the hash table.

Perhaps, we could put every symbol into the hash table so that we can quickly look-up <symbol> from __ksymtab_<symbol>, but it would consume lots of memory.

So, I think the implementation is this patch is good enough.

Quoted 21 lines

Introduce another hash table?
The first loop from this patch could traverse only the exported symbols instead of all symbols. But in this case, I don't know how to break early from the loop because there can be many symbols with the same name but with the different scope (static/non-static).
For example, ring_buffer_size:
kernel/trace/ring_buffer.c
4334:unsigned long ring_buffer_size(struct ring_buffer *buffer, int cpu) 4347:EXPORT_SYMBOL_GPL(ring_buffer_size);
And
drivers/usb/misc/ldusb.c
125:static int ring_buffer_size = 128;
Or for, nfs4_disable_idmapping:
fs/nfs/super.c
2920:bool nfs4_disable_idmapping = true;
2930:EXPORT_SYMBOL_GPL(nfs4_disable_idmapping);
fs/nfsd/nfs4idmap.c
48:static bool nfs4_disable_idmapping = true;

Signature · 2 lines

Best Regards
Masahiro Yamada

Stephen Rothwell

2019-07-29 12:40 UTC #11 lore raw

In reply to Denis Efremov

Hi Denis,

Quoted 7 lines · On Mon, 29 Jul 2019 12:16:29 +0300 Denis Efremov <[email protected]> wrote:

Quoted 3 lines
Just a reminder that some of us (just me?) do well over 100+ builds per day ... if this can be optimised some what that would be good.
These measurements for the worst case (allmodconfig). Is it possible to measure the slowdown in your case? How it will perform on your typical workflow?

I did 3 x86_64 allmodconfig builds without and with the patch (I do -j 80 powerpc64 le hosted cross builds) and it doesn't look like the patch has much impact at all.

Without the patch:

real 8m41.390s user 587m25.249s sys 22m0.411s
real 8m40.100s user 587m32.148s sys 21m58.419s
real 8m40.084s user 587m25.311s sys 22m2.794s

With the patch:

real 8m40.351s user 587m21.819s sys 21m57.389s
real 8m40.868s user 587m23.730s sys 21m58.737s
real 8m40.970s user 587m22.525s sys 22m2.467s

I do other builds as well, but that is the biggest, so actually looks ok.

Signature · 2 lines

Cheers,
Stephen Rothwell

Denis Efremov

author

2019-07-29 12:52 UTC #12 lore raw

Quoted 21 lines · Stephen Rothwell

Hi Denis,
Quoted 7 lines · On Mon, 29 Jul 2019 12:16:29 +0300 Denis Efremov <[email protected]> wrote:
Quoted 3 lines
Just a reminder that some of us (just me?) do well over 100+ builds per day ... if this can be optimised some what that would be good.
These measurements for the worst case (allmodconfig). Is it possible to measure the slowdown in your case? How it will perform on your typical workflow?
I did 3 x86_64 allmodconfig builds without and with the patch (I do -j 80 powerpc64 le hosted cross builds) and it doesn't look like the patch has much impact at all.
Without the patch:
real 8m41.390s user 587m25.249s sys 22m0.411s
real 8m40.100s user 587m32.148s sys 21m58.419s
real 8m40.084s user 587m25.311s sys 22m2.794s

Quoted 9 lines

With the patch:
real 8m40.351s user 587m21.819s sys 21m57.389s
real 8m40.868s user 587m23.730s sys 21m58.737s
real 8m40.970s user 587m22.525s sys 22m2.467s
I do other builds as well, but that is the biggest, so actually looks ok.

Is it worth to include your measurements instead of mine in the commit description? Maybe the note about performance downgrade could be omitted at all in this case?

Denis

Stephen Rothwell

2019-07-29 13:07 UTC #13 lore raw

Hi Denis,

Quoted 4 lines · On Mon, 29 Jul 2019 15:52:15 +0300 Denis Efremov <[email protected]> wrote:

Is it worth to include your measurements instead of mine in the commit description? Maybe the note about performance downgrade could be omitted at all in this case?

Just leave your measurements (they are yours after all), but maybe say "less than a minute" rather than "approx. 1 min".

Signature · 2 lines

Cheers,
Stephen Rothwell

Denis Efremov

author

2019-07-29 09:23 UTC #14 lore raw

PATCH v2 1/1 modpost: check for static EXPORT_SYMBOL* functions

In reply to Denis Efremov

Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x):

$ make mrproper; make allmodconfig; time make -j12; \
  git checkout HEAD~1; \
  make mrproper; make allmodconfig; time make -j12
1.
   (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total
   (w/o  patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total
2.
   (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total
   (w/o  patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total
3.
   (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total
   (w/o  patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total

Thus, the current implementation adds approx. 1 min for allmodconfig.

Acked-by Emil Velikov <[email protected]>

Signed-off-by Denis Efremov <[email protected]>

---
 scripts/mod/modpost.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

scripts/mod/modpost.c +32 -0

index f277e116e0eb..e138d763bcf2 100644
@@ -169,6 +169,7 @@ struct symbol {
 	unsigned int kernel:1;     /* 1 if symbol is from kernel
 				    *  (only for external modules) **/
 	unsigned int preloaded:1;  /* 1 if symbol from Module.symvers, or crc */
+	unsigned int is_static:1;  /* 1 if symbol is not global */
 	enum export  export;       /* Type of export */
 	char name[0];
 };

@@ -201,6 +202,7 @@ static struct symbol *alloc_symbol(const char *name, unsigned int weak,
 	strcpy(s->name, name);
 	s->weak = weak;
 	s->next = next;
+	s->is_static = 1;
 	return s;
 }

@@ -1980,6 +1982,22 @@ static void read_symbols(const char *modname)
 		handle_modversions(mod, &info, sym, symname);
 		handle_moddevtable(mod, &info, sym, symname);
 	}
+
+	// check for static EXPORT_SYMBOL_* functions && global vars
+	for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
+		unsigned char bind = ELF_ST_BIND(sym->st_info);
+		unsigned char type = ELF_ST_TYPE(sym->st_info);
+
+		if (type == STT_OBJECT || type == STT_FUNC) {
+			struct symbol *s =
+				find_symbol(remove_dot(info.strtab +
+						       sym->st_name));
+
+			if (s && (bind == STB_GLOBAL || bind == STB_WEAK))
+				s->is_static = 0;
+		}
+	}
+
 	if (!is_vmlinux(modname) || vmlinux_section_warnings)
 		check_sec_ref(mod, modname, &info);

@@ -2425,6 +2443,7 @@ int main(int argc, char **argv)
 	char *dump_write = NULL, *files_source = NULL;
 	int opt;
 	int err;
+	int n;
 	struct ext_sym_list *extsym_iter;
 	struct ext_sym_list *extsym_start = NULL;

@@ -2520,6 +2539,19 @@ int main(int argc, char **argv)
 	if (sec_mismatch_count && sec_mismatch_fatal)
 		fatal("modpost: Section mismatches detected.\n"
 		      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
+	for (n = 0; n < SYMBOL_HASH_SIZE; n++) {
+		struct symbol *s = symbolhash[n];
+
+		while (s) {
+			if (s->is_static)
+				warn("\"%s\" [%s] is the static %s\n",
+				     s->name, s->module->name,
+				     export_str(s->export));
+
+			s = s->next;
+		}
+	}
+
 	free(buf.p);
 
 	return err;

Signature · 1 line

2.21.0

Stephen Rothwell

2019-07-29 12:20 UTC #15 lore raw

Hi Denis,

Just a small note (nit? :-)):

scripts/mod/modpost.c int main(int argc, char **argv) · line 2547

Quoted 3 lines · On Mon, 29 Jul 2019 12:22:50 +0300 Denis Efremov <[email protected]> wrote:

scripts/mod/modpost.c +2 -0

+			if (s->is_static)
+				warn("\"%s\" [%s] is the static %s\n",

This read a little wrong to me, maybe "is a static"?

Signature · 2 lines

Cheers,
Stephen Rothwell

Denis Efremov

author

2019-07-29 12:47 UTC #16 lore raw

Quoted 11 lines · Stephen Rothwell

Hi Denis,
Just a small note (nit? :-)):
scripts/mod/modpost.c int main(int argc, char **argv) · line 2547
Quoted 3 lines · On Mon, 29 Jul 2019 12:22:50 +0300 Denis Efremov <[email protected]> wrote:
scripts/mod/modpost.c +2 -0
+			if (s->is_static)
+				warn("\"%s\" [%s] is the static %s\n",
This read a little wrong to me, maybe "is a static"?

Thank you! I will fix this in v3 in an hour.

Denis

Denis Efremov

author

2019-07-29 14:19 UTC #17 lore raw

PATCH v3 1/1 modpost: check for static EXPORT_SYMBOL* functions

In reply to Denis Efremov

Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x):

$ make mrproper; make allmodconfig; time make -j12; \
  git checkout HEAD~1; \
  make mrproper; make allmodconfig; time make -j12
1.
   (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total
   (w/o  patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total
2.
   (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total
   (w/o  patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total
3.
   (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total
   (w/o  patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total

The check adds less than a minute to a usual build.

Acked-by Emil Velikov <[email protected]>

Signed-off-by Denis Efremov <[email protected]>

---
 scripts/mod/modpost.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

scripts/mod/modpost.c +32 -0

index f277e116e0eb..332898d34e47 100644
@@ -169,6 +169,7 @@ struct symbol {
 	unsigned int kernel:1;     /* 1 if symbol is from kernel
 				    *  (only for external modules) **/
 	unsigned int preloaded:1;  /* 1 if symbol from Module.symvers, or crc */
+	unsigned int is_static:1;  /* 1 if symbol is not global */
 	enum export  export;       /* Type of export */
 	char name[0];
 };

@@ -201,6 +202,7 @@ static struct symbol *alloc_symbol(const char *name, unsigned int weak,
 	strcpy(s->name, name);
 	s->weak = weak;
 	s->next = next;
+	s->is_static = 1;
 	return s;
 }

@@ -1980,6 +1982,22 @@ static void read_symbols(const char *modname)
 		handle_modversions(mod, &info, sym, symname);
 		handle_moddevtable(mod, &info, sym, symname);
 	}
+
+	// check for static EXPORT_SYMBOL_* functions && global vars
+	for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
+		unsigned char bind = ELF_ST_BIND(sym->st_info);
+		unsigned char type = ELF_ST_TYPE(sym->st_info);
+
+		if (type == STT_OBJECT || type == STT_FUNC) {
+			struct symbol *s =
+				find_symbol(remove_dot(info.strtab +
+						       sym->st_name));
+
+			if (s && (bind == STB_GLOBAL || bind == STB_WEAK))
+				s->is_static = 0;
+		}
+	}
+
 	if (!is_vmlinux(modname) || vmlinux_section_warnings)
 		check_sec_ref(mod, modname, &info);

@@ -2425,6 +2443,7 @@ int main(int argc, char **argv)
 	char *dump_write = NULL, *files_source = NULL;
 	int opt;
 	int err;
+	int n;
 	struct ext_sym_list *extsym_iter;
 	struct ext_sym_list *extsym_start = NULL;

@@ -2520,6 +2539,19 @@ int main(int argc, char **argv)
 	if (sec_mismatch_count && sec_mismatch_fatal)
 		fatal("modpost: Section mismatches detected.\n"
 		      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
+	for (n = 0; n < SYMBOL_HASH_SIZE; n++) {
+		struct symbol *s = symbolhash[n];
+
+		while (s) {
+			if (s->is_static)
+				warn("\"%s\" [%s] is a static %s\n",
+				     s->name, s->module->name,
+				     export_str(s->export));
+
+			s = s->next;
+		}
+	}
+
 	free(buf.p);
 
 	return err;

Signature · 1 line

2.21.0

Stephen Rothwell

2019-07-29 22:26 UTC #18 lore raw

Hi Denis,

Quoted 6 lines · On Mon, 29 Jul 2019 17:18:01 +0300 Denis Efremov <[email protected]> wrote:

This patch adds a check to warn about static EXPORT_SYMBOL* functions during the modpost. In most of the cases, a static symbol marked for exporting is an odd combination that should be fixed either by deleting the exporting mark or by removing the static attribute and adding the appropriate declaration to headers.

OK, this is now in linux-next and I am getting what look like false positives :-(

My powerpc builds produce these:

WARNING: "ahci_em_messages" [vmlinux] is the static EXPORT_SYMBOL_GPL
WARNING: "ftrace_set_clr_event" [vmlinux] is the static EXPORT_SYMBOL_GPL
WARNING: "empty_zero_page" [vmlinux] is the static EXPORT_SYMBOL
WARNING: "jiffies" [vmlinux] is the static EXPORT_SYMBOL

empty_zero_page (at least) is not static. It is defined in assembler ...

jiffies is defined in arch/powerpc/kernel/vmlinux.lds.S as an alias for (part of) jiffies_64 which is not static (defined in kernel/time/timer.c).

The other 2 were OK.

Signature · 2 lines

Cheers,
Stephen Rothwell

Denis Efremov

author

2019-07-30 07:00 UTC #19 lore raw

Quoted 13 lines · Stephen Rothwell

Hi Denis,
Quoted 6 lines · On Mon, 29 Jul 2019 17:18:01 +0300 Denis Efremov <[email protected]> wrote:
This patch adds a check to warn about static EXPORT_SYMBOL* functions during the modpost. In most of the cases, a static symbol marked for exporting is an odd combination that should be fixed either by deleting the exporting mark or by removing the static attribute and adding the appropriate declaration to headers.
OK, this is now in linux-next and I am getting what look like false positives :-(

Quoted 8 lines

My powerpc builds produce these:
WARNING: "ahci_em_messages" [vmlinux] is the static EXPORT_SYMBOL_GPL
WARNING: "ftrace_set_clr_event" [vmlinux] is the static EXPORT_SYMBOL_GPL
WARNING: "empty_zero_page" [vmlinux] is the static EXPORT_SYMBOL
WARNING: "jiffies" [vmlinux] is the static EXPORT_SYMBOL
empty_zero_page (at least) is not static. It is defined in assembler ...

This could be fixed either by adding the type, for example:

arch/x86/kernel/head_64.S +1 -0

@@ -478,6 +478,7 @@ EXPORT_SYMBOL(phys_base)
 
        __PAGE_ALIGNED_BSS
 NEXT_PAGE(empty_zero_page)
+.type empty_zero_page, STT_OBJECT
        .skip PAGE_SIZE
 EXPORT_SYMBOL(empty_zero_page)

Or by updating the check in the patch:

scripts/mod/modpost.c +3 -1

@@ -1988,7 +1988,9 @@ static void read_symbols(const char *modname)
                unsigned char bind = ELF_ST_BIND(sym->st_info);
                unsigned char type = ELF_ST_TYPE(sym->st_info);
 
-               if (type == STT_OBJECT || type == STT_FUNC) {
+               if (type == STT_OBJECT ||
+                   type == STT_FUNC ||
+                   type == STT_NOTYPE) {

Do I need to resend the whole patch or create new "patch-on-patch"?

Denis

Masahiro Yamada

2019-07-30 16:30 UTC #20 lore raw

Quoted 28 lines · Denis Efremov

Quoted 13 lines · Stephen Rothwell
Hi Denis,
Quoted 6 lines · On Mon, 29 Jul 2019 17:18:01 +0300 Denis Efremov <[email protected]> wrote:
This patch adds a check to warn about static EXPORT_SYMBOL* functions during the modpost. In most of the cases, a static symbol marked for exporting is an odd combination that should be fixed either by deleting the exporting mark or by removing the static attribute and adding the appropriate declaration to headers.
OK, this is now in linux-next and I am getting what look like false positives :-(
Quoted 8 lines
My powerpc builds produce these:
WARNING: "ahci_em_messages" [vmlinux] is the static EXPORT_SYMBOL_GPL
WARNING: "ftrace_set_clr_event" [vmlinux] is the static EXPORT_SYMBOL_GPL
WARNING: "empty_zero_page" [vmlinux] is the static EXPORT_SYMBOL
WARNING: "jiffies" [vmlinux] is the static EXPORT_SYMBOL
empty_zero_page (at least) is not static. It is defined in assembler ...
This could be fixed either by adding the type, for example:

arch/x86/kernel/head_64.S EXPORT_SYMBOL(phys_base) · line 483

Quoted 6 lines

arch/x86/kernel/head_64.S +1 -0

@@ -478,6 +478,7 @@ EXPORT_SYMBOL(phys_base)
 
         __PAGE_ALIGNED_BSS
  NEXT_PAGE(empty_zero_page)
+.type empty_zero_page, STT_OBJECT
         .skip PAGE_SIZE
  EXPORT_SYMBOL(empty_zero_page)

This would require us to fix-up
all assembly files, wouldn't it?

Quoted 12 lines

Or by updating the check in the patch:

scripts/mod/modpost.c +3 -1

@@ -1988,7 +1988,9 @@ static void read_symbols(const char *modname)
                 unsigned char bind = ELF_ST_BIND(sym->st_info);
                 unsigned char type = ELF_ST_TYPE(sym->st_info);
 
-               if (type == STT_OBJECT || type == STT_FUNC) {
+               if (type == STT_OBJECT ||
+                   type == STT_FUNC ||
+                   type == STT_NOTYPE) {

Quoted 1 line

Do I need to resend the whole patch or create new "patch-on-patch"?

I prefer this, but why do you need to check type?

Doesn't this work?

for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
        unsigned char bind = ELF_ST_BIND(sym->st_info);

        struct symbol *s = find_symbol(remove_dot(info.strtab +
                                                  sym->st_name));

        if (s && (bind == STB_GLOBAL || bind == STB_WEAK))
                s->is_static = 0;
}

Signature · 2 lines

Best Regards
Masahiro Yamada

Denis Efremov

author

2019-07-30 16:44 UTC #21 lore raw

Quoted 4 lines · Masahiro Yamada

I prefer this, but why do you need to check type?
Doesn't this work?

Quoted 9 lines

for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
         unsigned char bind = ELF_ST_BIND(sym->st_info);

         struct symbol *s = find_symbol(remove_dot(info.strtab +
                                                   sym->st_name));

         if (s && (bind == STB_GLOBAL || bind == STB_WEAK))
                 s->is_static = 0;
}

This works. However, I thought it will be too costly to call find_symbol on each symbol. Hence, 'type == STT_OBJECT || type == STT_FUNC || type == STT_NOTYPE' is a small performance optimization because we need to check only variables and functions. Is it worth to remove it in v4?

Denis

Masahiro Yamada

2019-07-30 17:22 UTC #22 lore raw

Quoted 16 lines · Denis Efremov

Quoted 4 lines · Masahiro Yamada

I prefer this, but why do you need to check type?
Doesn't this work?

Quoted 9 lines

for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
         unsigned char bind = ELF_ST_BIND(sym->st_info);

         struct symbol *s = find_symbol(remove_dot(info.strtab +
                                                   sym->st_name));

         if (s && (bind == STB_GLOBAL || bind == STB_WEAK))
                 s->is_static = 0;
}

Quoted 6 lines

This works. However, I thought it will be too costly to call find_symbol on each symbol. Hence, 'type == STT_OBJECT || type == STT_FUNC || type == STT_NOTYPE' is a small performance optimization because we need to check only variables and functions. Is it worth to remove it in v4?
Denis

I checked the symbol table for ppc64_defconfig.

The following is the number of entries
for each combination of type and bind.

[1]  type: STT_NOTYPE,  bind: STB_LOCAL   -> 39502 entries
[2]  type: STT_NOTYPE,  bind: STB_GLOBAL  -> 30161 entries
[3]  type: STT_NOTYPE,  bind: STB_WEAK    -> 5 entries
[4]  type: STT_OBJECT,  bind: STB_LOCAL   -> 60326 entries
[5]  type: STT_OBJECT,  bind: STB_GLOBAL  -> 4126 entries
[6]  type: STT_OBJECT,  bind: STB_WEAK    -> 11 entries
[7]  type: STT_FUNC,    bind: STB_LOCAL   -> 38816 entries
[8]  type: STT_FUNC,    bind: STB_GLOBAL  -> 56196 entries
[9]  type: STT_FUNC,    bind: STB_WEAK    -> 350 entries
[10] type: STT_SECTION, bind: STB_LOCAL   -> 9027 entries
[11] type: STT_FILE,    bind: STB_LOCAL   -> 2918 entries

Checking 'type' beforehand
saves only 11945 look-ups ( [10] + [11]).

You can check 'bind' before the look-up, not after. If bind == STB_LOCAL, you do not need to lookup the hash-table, since you do not do anything.
This saves [1], [4], [7], [10], [11].

I think the following is simpler, and works more efficiently.

for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
        unsigned char bind = ELF_ST_BIND(sym->st_info);

        if (bind == STB_GLOBAL || bind == STB_WEAK) {
                  struct symbol *s =
                               find_symbol(remove_dot(info.strtab +
                                                      sym->st_name));

                  if (s)
                            s->is_static = 0;
         }
}

Signature · 2 lines

Best Regards
Masahiro Yamada

Denis Efremov

author

2019-07-30 18:12 UTC #23 lore raw

PATCH v4 1/1 modpost: check for static EXPORT_SYMBOL* functions

In reply to Denis Efremov

Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x):

$ make mrproper; make allmodconfig; time make -j12; \
  git checkout HEAD~1; \
  make mrproper; make allmodconfig; time make -j12
1.
   (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total
   (w/o  patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total
2.
   (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total
   (w/o  patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total
3.
   (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total
   (w/o  patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total

The check adds less than a minute to a usual build.

Acked-by Emil Velikov <[email protected]>

Signed-off-by Denis Efremov <[email protected]>

---
 scripts/mod/modpost.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

scripts/mod/modpost.c +31 -0

index f277e116e0eb..3e6d36ddfcdf 100644
@@ -169,6 +169,7 @@ struct symbol {
 	unsigned int kernel:1;     /* 1 if symbol is from kernel
 				    *  (only for external modules) **/
 	unsigned int preloaded:1;  /* 1 if symbol from Module.symvers, or crc */
+	unsigned int is_static:1;  /* 1 if symbol is not global */
 	enum export  export;       /* Type of export */
 	char name[0];
 };

@@ -201,6 +202,7 @@ static struct symbol *alloc_symbol(const char *name, unsigned int weak,
 	strcpy(s->name, name);
 	s->weak = weak;
 	s->next = next;
+	s->is_static = 1;
 	return s;
 }

@@ -1980,6 +1982,21 @@ static void read_symbols(const char *modname)
 		handle_modversions(mod, &info, sym, symname);
 		handle_moddevtable(mod, &info, sym, symname);
 	}
+
+	// check for static EXPORT_SYMBOL_* functions && global vars
+	for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
+		unsigned char bind = ELF_ST_BIND(sym->st_info);
+
+		if (bind == STB_GLOBAL || bind == STB_WEAK) {
+			struct symbol *s =
+				find_symbol(remove_dot(info.strtab +
+						       sym->st_name));
+
+			if (s)
+				s->is_static = 0;
+		}
+	}
+
 	if (!is_vmlinux(modname) || vmlinux_section_warnings)
 		check_sec_ref(mod, modname, &info);

@@ -2425,6 +2442,7 @@ int main(int argc, char **argv)
 	char *dump_write = NULL, *files_source = NULL;
 	int opt;
 	int err;
+	int n;
 	struct ext_sym_list *extsym_iter;
 	struct ext_sym_list *extsym_start = NULL;

@@ -2520,6 +2538,19 @@ int main(int argc, char **argv)
 	if (sec_mismatch_count && sec_mismatch_fatal)
 		fatal("modpost: Section mismatches detected.\n"
 		      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
+	for (n = 0; n < SYMBOL_HASH_SIZE; n++) {
+		struct symbol *s = symbolhash[n];
+
+		while (s) {
+			if (s->is_static)
+				warn("\"%s\" [%s] is a static %s\n",
+				     s->name, s->module->name,
+				     export_str(s->export));
+
+			s = s->next;
+		}
+	}
+
 	free(buf.p);
 
 	return err;

Signature · 1 line

2.21.0

Denis Efremov

author

2019-07-30 18:15 UTC #24 lore raw

Changes in v1:
1. Fixed indentations.
2. Removed lkml links from the description of the commit. Changes in v2:
1. Changed the 'n' variable type in the main function from size_t to int.
Changes in v3:
1. Changed warning message from "%s is the static EXPORT_*" to "%s a static ..."
2. Improved the commit description from "approx. 1 min" to "less than a minute". Thanks Stephen Rothwell for additional measurements.
Changes in v4:
1. Dropped ELF_ST_TYPE check. This fixes false-positives detected by Stephen Rothwell.
2. Moved ELF_ST_BIND check before the call to find_symbol. Thanks Masahiro Yamada for suggesting it.

Denis

Quoted 110 lines · Denis Efremov

Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x):

$ make mrproper; make allmodconfig; time make -j12; \
   git checkout HEAD~1; \
   make mrproper; make allmodconfig; time make -j12
1.
    (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total
    (w/o  patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total
2.
    (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total
    (w/o  patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total
3.
    (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total
    (w/o  patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total

The check adds less than a minute to a usual build.

Acked-by Emil Velikov <[email protected]>

Signed-off-by Denis Efremov <[email protected]>

---
  scripts/mod/modpost.c | 31 +++++++++++++++++++++++++++++++
  1 file changed, 31 insertions(+)

scripts/mod/modpost.c +31 -0

index f277e116e0eb..3e6d36ddfcdf 100644
@@ -169,6 +169,7 @@ struct symbol {
  	unsigned int kernel:1;     /* 1 if symbol is from kernel
  				    *  (only for external modules) **/
  	unsigned int preloaded:1;  /* 1 if symbol from Module.symvers, or crc */
+	unsigned int is_static:1;  /* 1 if symbol is not global */
  	enum export  export;       /* Type of export */
  	char name[0];
  };

@@ -201,6 +202,7 @@ static struct symbol *alloc_symbol(const char *name, unsigned int weak,
  	strcpy(s->name, name);
  	s->weak = weak;
  	s->next = next;
+	s->is_static = 1;
  	return s;
  }

@@ -1980,6 +1982,21 @@ static void read_symbols(const char *modname)
  		handle_modversions(mod, &info, sym, symname);
  		handle_moddevtable(mod, &info, sym, symname);
  	}
+
+	// check for static EXPORT_SYMBOL_* functions && global vars
+	for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
+		unsigned char bind = ELF_ST_BIND(sym->st_info);
+
+		if (bind == STB_GLOBAL || bind == STB_WEAK) {
+			struct symbol *s =
+				find_symbol(remove_dot(info.strtab +
+						       sym->st_name));
+
+			if (s)
+				s->is_static = 0;
+		}
+	}
+
  	if (!is_vmlinux(modname) || vmlinux_section_warnings)
  		check_sec_ref(mod, modname, &info);

@@ -2425,6 +2442,7 @@ int main(int argc, char **argv)
  	char *dump_write = NULL, *files_source = NULL;
  	int opt;
  	int err;
+	int n;
  	struct ext_sym_list *extsym_iter;
  	struct ext_sym_list *extsym_start = NULL;

@@ -2520,6 +2538,19 @@ int main(int argc, char **argv)
  	if (sec_mismatch_count && sec_mismatch_fatal)
  		fatal("modpost: Section mismatches detected.\n"
  		      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
+	for (n = 0; n < SYMBOL_HASH_SIZE; n++) {
+		struct symbol *s = symbolhash[n];
+
+		while (s) {
+			if (s->is_static)
+				warn("\"%s\" [%s] is a static %s\n",
+				     s->name, s->module->name,
+				     export_str(s->export));
+
+			s = s->next;
+		}
+	}
+
  	free(buf.p);
 
  	return err;

Masahiro Yamada

2019-07-31 08:55 UTC #25 lore raw

In reply to Denis Efremov

Hi.

Quoted 32 lines · Denis Efremov

This patch adds a check to warn about static EXPORT_SYMBOL* functions during the modpost. In most of the cases, a static symbol marked for exporting is an odd combination that should be fixed either by deleting the exporting mark or by removing the static attribute and adding the appropriate declaration to headers.
This check could help to detect the following problems:
1. 550113d4e9f5 ("i2c: add newly exported functions to the header, too")
2. 54638c6eaf44 ("net: phy: make exported variables non-static")
3. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
4. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration")
5. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
6. e4e4730698c9 ("crypto: skcipher - remove the exporting of skcipher_walk_next")
7. 14b4c48bb1ce ("gve: Remove the exporting of gve_probe")
8. 9b79ee9773a8 ("scsi: libsas: remove the exporting of sas_wait_eh")
9. ...
Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x):
$ make mrproper; make allmodconfig; time make -j12; \
  git checkout HEAD~1; \
  make mrproper; make allmodconfig; time make -j12
1.
   (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total
   (w/o  patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total
2.
   (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total
   (w/o  patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total
3.
   (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total
   (w/o  patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total

Quoted 1 line

The check adds less than a minute to a usual build.

I am not convinced with this analysis.

Since commit b7dca6dd1e591ad19,
scripts/mod/modpost is only invoked from scripts/Makefile.modpost.

So, it is much easier to to measure the elapsed time of modpost directly.

I applied your patch on top next-20190731,
and the following:

scripts/Makefile.modpost +1 -2

index 92ed02d7cd5e..e0db1a626e50 100644
@@ -98,7 +98,7 @@ MODPOST += $(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
-s -T - $(wildcard vmlinux)
 
 # We can go over command line length here, so be careful.
 quiet_cmd_modpost = MODPOST $(words $(modules)) modules
-      cmd_modpost = sed 's/ko$$/o/' $(modorder) | $(MODPOST)
+      cmd_modpost = sed 's/ko$$/o/' $(modorder) | time -o

modpost-time.txt $(MODPOST)

PHONY += modules-modpost
modules-modpost:

The difference is less than 1 sec (not 1 min!) even for allmodconfig. So, the performance regression is almost unnoticeable level.

Signature · 2 lines

Best Regards
Masahiro Yamada

Masahiro Yamada

2019-08-01 02:21 UTC #26 lore raw

Quoted 39 lines · Masahiro Yamada

Hi.
Quoted 33 lines · Denis Efremov
This patch adds a check to warn about static EXPORT_SYMBOL* functions during the modpost. In most of the cases, a static symbol marked for exporting is an odd combination that should be fixed either by deleting the exporting mark or by removing the static attribute and adding the appropriate declaration to headers.
This check could help to detect the following problems:
1. 550113d4e9f5 ("i2c: add newly exported functions to the header, too")
2. 54638c6eaf44 ("net: phy: make exported variables non-static")
3. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
4. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration")
5. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
6. e4e4730698c9 ("crypto: skcipher - remove the exporting of skcipher_walk_next")
7. 14b4c48bb1ce ("gve: Remove the exporting of gve_probe")
8. 9b79ee9773a8 ("scsi: libsas: remove the exporting of sas_wait_eh")
9. ...
Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x):
$ make mrproper; make allmodconfig; time make -j12; \
  git checkout HEAD~1; \
  make mrproper; make allmodconfig; time make -j12
1.
   (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total
   (w/o  patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total
2.
   (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total
   (w/o  patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total
3.
   (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total
   (w/o  patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total
The check adds less than a minute to a usual build.

The build time impact is very limited.
I guess this measurement is unnecessary in the commit log.

Signature · 2 lines

Best Regards
Masahiro Yamada

Denis Efremov

author

2019-08-01 06:17 UTC #27 lore raw

Quoted 4 lines · Masahiro Yamada

The build time impact is very limited.
I guess this measurement is unnecessary in the commit log.

Thank you for your observations! It was not easy for me to guess to do it this way because of my limited knowledge about kbuild && modpost work. In v5 I've only updated commit log: measurements removed, "The build time impact is very limited and is almost at the unnoticeable level (< 1 sec)" added instead. Fell free to edit it if you want.

Denis

Denis Efremov

author

2019-08-01 06:07 UTC #28 lore raw

PATCH v5 1/1 modpost: check for static EXPORT_SYMBOL* functions

In reply to Denis Efremov

The build time impact is very limited and is almost at the unnoticeable level (< 1 sec).

Acked-by Emil Velikov <[email protected]>

Signed-off-by Denis Efremov <[email protected]>

---
 scripts/mod/modpost.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

scripts/mod/modpost.c +31 -0

index f277e116e0eb..3e6d36ddfcdf 100644
@@ -169,6 +169,7 @@ struct symbol {
 	unsigned int kernel:1;     /* 1 if symbol is from kernel
 				    *  (only for external modules) **/
 	unsigned int preloaded:1;  /* 1 if symbol from Module.symvers, or crc */
+	unsigned int is_static:1;  /* 1 if symbol is not global */
 	enum export  export;       /* Type of export */
 	char name[0];
 };

@@ -201,6 +202,7 @@ static struct symbol *alloc_symbol(const char *name, unsigned int weak,
 	strcpy(s->name, name);
 	s->weak = weak;
 	s->next = next;
+	s->is_static = 1;
 	return s;
 }

@@ -1980,6 +1982,21 @@ static void read_symbols(const char *modname)
 		handle_modversions(mod, &info, sym, symname);
 		handle_moddevtable(mod, &info, sym, symname);
 	}
+
+	// check for static EXPORT_SYMBOL_* functions && global vars
+	for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
+		unsigned char bind = ELF_ST_BIND(sym->st_info);
+
+		if (bind == STB_GLOBAL || bind == STB_WEAK) {
+			struct symbol *s =
+				find_symbol(remove_dot(info.strtab +
+						       sym->st_name));
+
+			if (s)
+				s->is_static = 0;
+		}
+	}
+
 	if (!is_vmlinux(modname) || vmlinux_section_warnings)
 		check_sec_ref(mod, modname, &info);

@@ -2425,6 +2442,7 @@ int main(int argc, char **argv)
 	char *dump_write = NULL, *files_source = NULL;
 	int opt;
 	int err;
+	int n;
 	struct ext_sym_list *extsym_iter;
 	struct ext_sym_list *extsym_start = NULL;

@@ -2520,6 +2538,19 @@ int main(int argc, char **argv)
 	if (sec_mismatch_count && sec_mismatch_fatal)
 		fatal("modpost: Section mismatches detected.\n"
 		      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
+	for (n = 0; n < SYMBOL_HASH_SIZE; n++) {
+		struct symbol *s = symbolhash[n];
+
+		while (s) {
+			if (s->is_static)
+				warn("\"%s\" [%s] is a static %s\n",
+				     s->name, s->module->name,
+				     export_str(s->export));
+
+			s = s->next;
+		}
+	}
+
 	free(buf.p);
 
 	return err;

Signature · 1 line

2.21.0

Masahiro Yamada

2019-08-07 15:13 UTC #29 lore raw

Quoted 18 lines · Denis Efremov

This patch adds a check to warn about static EXPORT_SYMBOL* functions during the modpost. In most of the cases, a static symbol marked for exporting is an odd combination that should be fixed either by deleting the exporting mark or by removing the static attribute and adding the appropriate declaration to headers.
This check could help to detect the following problems:
1. 550113d4e9f5 ("i2c: add newly exported functions to the header, too")
2. 54638c6eaf44 ("net: phy: make exported variables non-static")
3. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
4. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration")
5. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
6. e4e4730698c9 ("crypto: skcipher - remove the exporting of skcipher_walk_next")
7. 14b4c48bb1ce ("gve: Remove the exporting of gve_probe")
8. 9b79ee9773a8 ("scsi: libsas: remove the exporting of sas_wait_eh")
9. ...

Quoted 6 lines

The build time impact is very limited and is almost at the unnoticeable level (< 1 sec).
Acked-by Emil Velikov <[email protected]>
Signed-off-by Denis Efremov <[email protected]>
---

Applied to linux-kbuild. Thanks.

Signature · 2 lines

Best Regards
Masahiro Yamada

Masahiro Yamada

2019-08-13 16:08 UTC #30 lore raw

Hi Denis,

Quoted 20 lines · Masahiro Yamada

Quoted 18 lines · Denis Efremov
This patch adds a check to warn about static EXPORT_SYMBOL* functions during the modpost. In most of the cases, a static symbol marked for exporting is an odd combination that should be fixed either by deleting the exporting mark or by removing the static attribute and adding the appropriate declaration to headers.
This check could help to detect the following problems:
1. 550113d4e9f5 ("i2c: add newly exported functions to the header, too")
2. 54638c6eaf44 ("net: phy: make exported variables non-static")
3. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
4. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration")
5. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
6. e4e4730698c9 ("crypto: skcipher - remove the exporting of skcipher_walk_next")
7. 14b4c48bb1ce ("gve: Remove the exporting of gve_probe")
8. 9b79ee9773a8 ("scsi: libsas: remove the exporting of sas_wait_eh")
9. ...

Quoted 8 lines

Quoted 6 lines
The build time impact is very limited and is almost at the unnoticeable level (< 1 sec).
Acked-by Emil Velikov <[email protected]>
Signed-off-by Denis Efremov <[email protected]>
---
Applied to linux-kbuild. Thanks.

I noticed this patch produces false-positive warnings for external module build.

When I built my external module, it showed the following, the last five are false positives.

make[1]: Entering directory '/home/masahiro/workspace/linux-kbuild'
Building modules, stage 2.
MODPOST 2 modules
WARNING: "drm_client_close" [vmlinux] is a static EXPORT_SYMBOL
WARNING: "ahci_em_messages" [vmlinux] is a static EXPORT_SYMBOL_GPL
WARNING: "ftrace_set_clr_event" [vmlinux] is a static EXPORT_SYMBOL_GPL
WARNING: "nf_log_dump_packet_common" [net/netfilter/nf_log_common] is a static EXPORT_SYMBOL_GPL
WARNING: "nf_log_l2packet" [net/netfilter/nf_log_common] is a static EXPORT_SYMBOL_GPL
WARNING: "nf_log_dump_sk_uid_gid" [net/netfilter/nf_log_common] is a static EXPORT_SYMBOL_GPL
WARNING: "nf_log_dump_udp_header" [net/netfilter/nf_log_common] is a static EXPORT_SYMBOL_GPL
WARNING: "nf_log_dump_tcp_header" [net/netfilter/nf_log_common] is a static EXPORT_SYMBOL_GPL
make[1]: Leaving directory '/home/masahiro/workspace/linux-kbuild'

I squashed the following fix-up.

scripts/mod/modpost.c +0 -0

index 3e6d36ddfcdf..2773f9f9bae2 100644
@@ -2386,6 +2386,7 @@ static void read_dump(const char *fname,

unsigned int kernel)

                s = sym_add_exported(symname, mod, export_no(export));
                s->kernel    = kernel;
                s->preloaded = 1;
+               s->is_static = 0;
                sym_update_crc(symname, mod, crc, export_no(export));
        }
        release_file(file, size);

Signature · 2 lines

Best Regards
Masahiro Yamada

Denis Efremov

author

2019-08-13 21:11 UTC #31 lore raw

Quoted 17 lines · Masahiro Yamada

Hi Denis,

I squashed the following fix-up.

scripts/mod/modpost.c +1 -0

index 3e6d36ddfcdf..2773f9f9bae2 100644
@@ -2386,6 +2386,7 @@ static void read_dump(const char *fname,
 unsigned int kernel)
                s = sym_add_exported(symname, mod, export_no(export));
                s->kernel    = kernel;
                s->preloaded = 1;
+               s->is_static = 0;
                sym_update_crc(symname, mod, crc, export_no(export));
        }

Quoted 1 line

        release_file(file, size);

Hi!

Thank you very much indeed.

Best regards,
Denis