diff mbox series

[v3] modpost: check for static EXPORT_SYMBOL* functions

Message ID 20190729141801.31333-1-efremov@linux.com (mailing list archive)
State New, archived
Headers show
Series [v3] modpost: check for static EXPORT_SYMBOL* functions | expand

Commit Message

Denis Efremov (Oracle) July 29, 2019, 2:18 p.m. UTC
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.

Acked-by: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Denis Efremov <efremov@linux.com>
---
 scripts/mod/modpost.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Stephen Rothwell July 29, 2019, 10:26 p.m. UTC | #1
Hi Denis,

On Mon, 29 Jul 2019 17:18:01 +0300 Denis Efremov <efremov@linux.com> 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.
Denis Efremov (Oracle) July 30, 2019, 6:59 a.m. UTC | #2
On 30.07.2019 01:26, Stephen Rothwell wrote:
> Hi Denis,
> 
> On Mon, 29 Jul 2019 17:18:01 +0300 Denis Efremov <efremov@linux.com> 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 ...

This could be fixed either by adding the type, for example:
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -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:
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -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 July 30, 2019, 4:29 p.m. UTC | #3
On Tue, Jul 30, 2019 at 4:00 PM Denis Efremov <efremov@linux.com> wrote:
>
> On 30.07.2019 01:26, Stephen Rothwell wrote:
> > Hi Denis,
> >
> > On Mon, 29 Jul 2019 17:18:01 +0300 Denis Efremov <efremov@linux.com> 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 ...
>
> This could be fixed either by adding the type, for example:
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -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?


> Or by updating the check in the patch:
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -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"?

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;
}
Denis Efremov (Oracle) July 30, 2019, 4:44 p.m. UTC | #4
On 30.07.2019 19:29, Masahiro Yamada wrote:
> 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;
> }

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 July 30, 2019, 5:21 p.m. UTC | #5
On Wed, Jul 31, 2019 at 1:44 AM Denis Efremov <efremov@linux.com> wrote:
>
> On 30.07.2019 19:29, Masahiro Yamada wrote:
> > 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;
> > }
>
> 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;
         }
}
diff mbox series

Patch

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f277e116e0eb..332898d34e47 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -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;