* [PATCH] module: Remove const attribute from alias for MODULE_DEVICE_TABLE
@ 2017-07-25 1:27 Matthias Kaehlcke
2017-07-29 21:48 ` Jessica Yu
2017-08-27 23:52 ` Stefan Agner
0 siblings, 2 replies; 7+ messages in thread
From: Matthias Kaehlcke @ 2017-07-25 1:27 UTC (permalink / raw)
To: Jessica Yu, Rusty Russell
Cc: linux-kernel, Masahiro Yamada, Michal Marek, Kees Cook,
Arnd Bergmann, Doug Anderson, Grant Grundler, Greg Hackmann,
Michael Davidson, Nick Desaulniers, Alexander Potapenko,
Bernhard.Rosenkranzer, Matthias Kaehlcke
MODULE_DEVICE_TABLE(type, name) creates an alias of type 'extern const
typeof(name)'. If 'name' is already constant the 'const' attribute is
specified twice, which is not allowed in C89 (see discussion at
https://lkml.org/lkml/2017/5/23/1440). Since the kernel is built with
-std=gnu89 clang generates warnings like this:
drivers/thermal/x86_pkg_temp_thermal.c:509:1: warning: duplicate 'const'
declaration specifier
[-Wduplicate-decl-specifier]
MODULE_DEVICE_TABLE(x86cpu, pkg_temp_thermal_ids);
^
./include/linux/module.h:212:8: note: expanded from macro 'MODULE_DEVICE_TABLE'
extern const typeof(name) __mod_##type##__##name##_device_table
Remove the const attribute from the alias to avoid the duplicate
specifier. After all it is only an alias and the attribute shouldn't
have any effect.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
include/linux/module.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index e7bdd549e527..fe5aa3736707 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -209,7 +209,7 @@ extern void cleanup_module(void);
#ifdef MODULE
/* Creates an alias so file2alias.c can find device table. */
#define MODULE_DEVICE_TABLE(type, name) \
-extern const typeof(name) __mod_##type##__##name##_device_table \
+extern typeof(name) __mod_##type##__##name##_device_table \
__attribute__ ((unused, alias(__stringify(name))))
#else /* !MODULE */
#define MODULE_DEVICE_TABLE(type, name)
--
2.14.0.rc0.284.gd933b75aa4-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE 2017-07-25 1:27 [PATCH] module: Remove const attribute from alias for MODULE_DEVICE_TABLE Matthias Kaehlcke @ 2017-07-29 21:48 ` Jessica Yu 2017-08-27 23:52 ` Stefan Agner 1 sibling, 0 replies; 7+ messages in thread From: Jessica Yu @ 2017-07-29 21:48 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Rusty Russell, linux-kernel, Masahiro Yamada, Michal Marek, Kees Cook, Arnd Bergmann, Doug Anderson, Grant Grundler, Greg Hackmann, Michael Davidson, Nick Desaulniers, Alexander Potapenko, Bernhard.Rosenkranzer +++ Matthias Kaehlcke [24/07/17 18:27 -0700]: >MODULE_DEVICE_TABLE(type, name) creates an alias of type 'extern const >typeof(name)'. If 'name' is already constant the 'const' attribute is >specified twice, which is not allowed in C89 (see discussion at >https://lkml.org/lkml/2017/5/23/1440). Since the kernel is built with >-std=gnu89 clang generates warnings like this: > >drivers/thermal/x86_pkg_temp_thermal.c:509:1: warning: duplicate 'const' > declaration specifier > [-Wduplicate-decl-specifier] >MODULE_DEVICE_TABLE(x86cpu, pkg_temp_thermal_ids); >^ >./include/linux/module.h:212:8: note: expanded from macro 'MODULE_DEVICE_TABLE' >extern const typeof(name) __mod_##type##__##name##_device_table > >Remove the const attribute from the alias to avoid the duplicate >specifier. After all it is only an alias and the attribute shouldn't >have any effect. > >Signed-off-by: Matthias Kaehlcke <mka@chromium.org> Applied, thanks! Jessica > include/linux/module.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/include/linux/module.h b/include/linux/module.h >index e7bdd549e527..fe5aa3736707 100644 >--- a/include/linux/module.h >+++ b/include/linux/module.h >@@ -209,7 +209,7 @@ extern void cleanup_module(void); > #ifdef MODULE > /* Creates an alias so file2alias.c can find device table. */ > #define MODULE_DEVICE_TABLE(type, name) \ >-extern const typeof(name) __mod_##type##__##name##_device_table \ >+extern typeof(name) __mod_##type##__##name##_device_table \ > __attribute__ ((unused, alias(__stringify(name)))) > #else /* !MODULE */ > #define MODULE_DEVICE_TABLE(type, name) >-- >2.14.0.rc0.284.gd933b75aa4-goog > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE 2017-07-25 1:27 [PATCH] module: Remove const attribute from alias for MODULE_DEVICE_TABLE Matthias Kaehlcke 2017-07-29 21:48 ` Jessica Yu @ 2017-08-27 23:52 ` Stefan Agner 2017-08-28 17:20 ` Kees Cook 1 sibling, 1 reply; 7+ messages in thread From: Stefan Agner @ 2017-08-27 23:52 UTC (permalink / raw) To: Jessica Yu, Matthias Kaehlcke Cc: Rusty Russell, linux-kernel, Masahiro Yamada, Michal Marek, Kees Cook, Arnd Bergmann, Doug Anderson, Grant Grundler, Greg Hackmann, Michael Davidson, Nick Desaulniers, Alexander Potapenko, Bernhard.Rosenkranzer On 2017-07-24 18:27, Matthias Kaehlcke wrote: > MODULE_DEVICE_TABLE(type, name) creates an alias of type 'extern const > typeof(name)'. If 'name' is already constant the 'const' attribute is > specified twice, which is not allowed in C89 (see discussion at > https://lkml.org/lkml/2017/5/23/1440). Since the kernel is built with > -std=gnu89 clang generates warnings like this: > > drivers/thermal/x86_pkg_temp_thermal.c:509:1: warning: duplicate 'const' > declaration specifier > [-Wduplicate-decl-specifier] > MODULE_DEVICE_TABLE(x86cpu, pkg_temp_thermal_ids); > ^ > ./include/linux/module.h:212:8: note: expanded from macro 'MODULE_DEVICE_TABLE' > extern const typeof(name) __mod_##type##__##name##_device_table > > Remove the const attribute from the alias to avoid the duplicate > specifier. After all it is only an alias and the attribute shouldn't > have any effect. Unfortunately, it has effect where const is missing in the original variable declaration: Before this patch: 13:10 $ size drivers/net/wireless/ath/ath6kl/ath6kl_usb.ko text data bss dec hex filename 8825 728 40 9593 2579 drivers/net/wireless/ath/ath6kl/ath6kl_usb.ko After this patch: 13:12 $ size drivers/net/wireless/ath/ath6kl/ath6kl_usb.ko text data bss dec hex filename 8747 800 40 9587 2573 drivers/net/wireless/ath/ath6kl/ath6kl_usb.ko Ideally we would fix all MODULE_DEVICE_TABLE usage sites. This would also made it clearly visible that the device tables are const. I created a semantic patch, it turns out that 620 sites are affected (out of 4499)... // // Cocinelle Semantic Patch to constify module device tables // // Author: Stefan Agner <stefan@agner.ch> // @ module_device_table @ declarer name MODULE_DEVICE_TABLE; identifier moduletype; identifier name; @@ MODULE_DEVICE_TABLE(moduletype, name); @ add_const depends on module_device_table disable optional_qualifier @ identifier module_device_table.name; type T; @@ +const T name[] = { ... }; Thoughts? -- Stefan > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > include/linux/module.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/module.h b/include/linux/module.h > index e7bdd549e527..fe5aa3736707 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -209,7 +209,7 @@ extern void cleanup_module(void); > #ifdef MODULE > /* Creates an alias so file2alias.c can find device table. */ > #define MODULE_DEVICE_TABLE(type, name) \ > -extern const typeof(name) __mod_##type##__##name##_device_table \ > +extern typeof(name) __mod_##type##__##name##_device_table \ > __attribute__ ((unused, alias(__stringify(name)))) > #else /* !MODULE */ > #define MODULE_DEVICE_TABLE(type, name) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE 2017-08-27 23:52 ` Stefan Agner @ 2017-08-28 17:20 ` Kees Cook 2017-08-28 17:38 ` Nick Desaulniers 0 siblings, 1 reply; 7+ messages in thread From: Kees Cook @ 2017-08-28 17:20 UTC (permalink / raw) To: Stefan Agner Cc: Jessica Yu, Matthias Kaehlcke, Rusty Russell, LKML, Masahiro Yamada, Michal Marek, Arnd Bergmann, Doug Anderson, Grant Grundler, Greg Hackmann, Michael Davidson, Nick Desaulniers, Alexander Potapenko, Bernhard.Rosenkranzer On Sun, Aug 27, 2017 at 4:52 PM, Stefan Agner <stefan@agner.ch> wrote: > On 2017-07-24 18:27, Matthias Kaehlcke wrote: >> MODULE_DEVICE_TABLE(type, name) creates an alias of type 'extern const >> typeof(name)'. If 'name' is already constant the 'const' attribute is >> specified twice, which is not allowed in C89 (see discussion at >> https://lkml.org/lkml/2017/5/23/1440). Since the kernel is built with >> -std=gnu89 clang generates warnings like this: >> >> drivers/thermal/x86_pkg_temp_thermal.c:509:1: warning: duplicate 'const' >> declaration specifier >> [-Wduplicate-decl-specifier] >> MODULE_DEVICE_TABLE(x86cpu, pkg_temp_thermal_ids); >> ^ >> ./include/linux/module.h:212:8: note: expanded from macro 'MODULE_DEVICE_TABLE' >> extern const typeof(name) __mod_##type##__##name##_device_table >> >> Remove the const attribute from the alias to avoid the duplicate >> specifier. After all it is only an alias and the attribute shouldn't >> have any effect. > > Unfortunately, it has effect where const is missing in the original > variable declaration: > > Before this patch: > 13:10 $ size drivers/net/wireless/ath/ath6kl/ath6kl_usb.ko > text data bss dec hex filename > 8825 728 40 9593 2579 > drivers/net/wireless/ath/ath6kl/ath6kl_usb.ko > > After this patch: > 13:12 $ size drivers/net/wireless/ath/ath6kl/ath6kl_usb.ko > text data bss dec hex filename > 8747 800 40 9587 2573 > drivers/net/wireless/ath/ath6kl/ath6kl_usb.ko > > > Ideally we would fix all MODULE_DEVICE_TABLE usage sites. This would > also made it clearly visible that the device tables are const. > > I created a semantic patch, it turns out that 620 sites are affected > (out of 4499)... > > // > // Cocinelle Semantic Patch to constify module device tables > // > // Author: Stefan Agner <stefan@agner.ch> > // > @ module_device_table @ > declarer name MODULE_DEVICE_TABLE; > identifier moduletype; > identifier name; > @@ > MODULE_DEVICE_TABLE(moduletype, name); > > @ add_const depends on module_device_table disable optional_qualifier @ > identifier module_device_table.name; > type T; > @@ > +const > T name[] = { > ... > }; > > Thoughts? > > -- > Stefan > > >> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> >> --- >> include/linux/module.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/linux/module.h b/include/linux/module.h >> index e7bdd549e527..fe5aa3736707 100644 >> --- a/include/linux/module.h >> +++ b/include/linux/module.h >> @@ -209,7 +209,7 @@ extern void cleanup_module(void); >> #ifdef MODULE >> /* Creates an alias so file2alias.c can find device table. */ >> #define MODULE_DEVICE_TABLE(type, name) \ >> -extern const typeof(name) __mod_##type##__##name##_device_table \ >> +extern typeof(name) __mod_##type##__##name##_device_table \ >> __attribute__ ((unused, alias(__stringify(name)))) >> #else /* !MODULE */ >> #define MODULE_DEVICE_TABLE(type, name) Perhaps the reverse is the better solution? Leave "const" in MODULE_DEVICE_TABLE and remove the redundant usage. This means new cases of missing the const will never happen (which was the intent originally of putting const into the MODULE_DEVICE_TABLE macro, I assume). -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE 2017-08-28 17:20 ` Kees Cook @ 2017-08-28 17:38 ` Nick Desaulniers 2017-08-28 17:41 ` Kees Cook 0 siblings, 1 reply; 7+ messages in thread From: Nick Desaulniers @ 2017-08-28 17:38 UTC (permalink / raw) To: Kees Cook Cc: Stefan Agner, Jessica Yu, Matthias Kaehlcke, Rusty Russell, LKML, Masahiro Yamada, Michal Marek, Arnd Bergmann, Doug Anderson, Grant Grundler, Greg Hackmann, Michael Davidson, Alexander Potapenko, Bernhard.Rosenkranzer I think Kees' proposal is a better solution; rather than require all usage of device table to remember to add const, have the macro add it for all users. Otherwise if you require caller's to add it, they may forget. On Mon, Aug 28, 2017 at 10:20 AM, Kees Cook <keescook@chromium.org> wrote: > On Sun, Aug 27, 2017 at 4:52 PM, Stefan Agner <stefan@agner.ch> wrote: >> On 2017-07-24 18:27, Matthias Kaehlcke wrote: >>> MODULE_DEVICE_TABLE(type, name) creates an alias of type 'extern const >>> typeof(name)'. If 'name' is already constant the 'const' attribute is >>> specified twice, which is not allowed in C89 (see discussion at >>> https://lkml.org/lkml/2017/5/23/1440). Since the kernel is built with >>> -std=gnu89 clang generates warnings like this: >>> >>> drivers/thermal/x86_pkg_temp_thermal.c:509:1: warning: duplicate 'const' >>> declaration specifier >>> [-Wduplicate-decl-specifier] >>> MODULE_DEVICE_TABLE(x86cpu, pkg_temp_thermal_ids); >>> ^ >>> ./include/linux/module.h:212:8: note: expanded from macro 'MODULE_DEVICE_TABLE' >>> extern const typeof(name) __mod_##type##__##name##_device_table >>> >>> Remove the const attribute from the alias to avoid the duplicate >>> specifier. After all it is only an alias and the attribute shouldn't >>> have any effect. >> >> Unfortunately, it has effect where const is missing in the original >> variable declaration: >> >> Before this patch: >> 13:10 $ size drivers/net/wireless/ath/ath6kl/ath6kl_usb.ko >> text data bss dec hex filename >> 8825 728 40 9593 2579 >> drivers/net/wireless/ath/ath6kl/ath6kl_usb.ko >> >> After this patch: >> 13:12 $ size drivers/net/wireless/ath/ath6kl/ath6kl_usb.ko >> text data bss dec hex filename >> 8747 800 40 9587 2573 >> drivers/net/wireless/ath/ath6kl/ath6kl_usb.ko >> >> >> Ideally we would fix all MODULE_DEVICE_TABLE usage sites. This would >> also made it clearly visible that the device tables are const. >> >> I created a semantic patch, it turns out that 620 sites are affected >> (out of 4499)... >> >> // >> // Cocinelle Semantic Patch to constify module device tables >> // >> // Author: Stefan Agner <stefan@agner.ch> >> // >> @ module_device_table @ >> declarer name MODULE_DEVICE_TABLE; >> identifier moduletype; >> identifier name; >> @@ >> MODULE_DEVICE_TABLE(moduletype, name); >> >> @ add_const depends on module_device_table disable optional_qualifier @ >> identifier module_device_table.name; >> type T; >> @@ >> +const >> T name[] = { >> ... >> }; >> >> Thoughts? >> >> -- >> Stefan >> >> >>> >>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> >>> --- >>> include/linux/module.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/include/linux/module.h b/include/linux/module.h >>> index e7bdd549e527..fe5aa3736707 100644 >>> --- a/include/linux/module.h >>> +++ b/include/linux/module.h >>> @@ -209,7 +209,7 @@ extern void cleanup_module(void); >>> #ifdef MODULE >>> /* Creates an alias so file2alias.c can find device table. */ >>> #define MODULE_DEVICE_TABLE(type, name) \ >>> -extern const typeof(name) __mod_##type##__##name##_device_table \ >>> +extern typeof(name) __mod_##type##__##name##_device_table \ >>> __attribute__ ((unused, alias(__stringify(name)))) >>> #else /* !MODULE */ >>> #define MODULE_DEVICE_TABLE(type, name) > > Perhaps the reverse is the better solution? Leave "const" in > MODULE_DEVICE_TABLE and remove the redundant usage. This means new > cases of missing the const will never happen (which was the intent > originally of putting const into the MODULE_DEVICE_TABLE macro, I > assume). > > -Kees > > -- > Kees Cook > Pixel Security -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE 2017-08-28 17:38 ` Nick Desaulniers @ 2017-08-28 17:41 ` Kees Cook 2017-08-29 2:24 ` Stefan Agner 0 siblings, 1 reply; 7+ messages in thread From: Kees Cook @ 2017-08-28 17:41 UTC (permalink / raw) To: Nick Desaulniers Cc: Stefan Agner, Jessica Yu, Matthias Kaehlcke, Rusty Russell, LKML, Masahiro Yamada, Michal Marek, Arnd Bergmann, Doug Anderson, Grant Grundler, Greg Hackmann, Michael Davidson, Alexander Potapenko, Bernhard.Rosenkranzer On Mon, Aug 28, 2017 at 10:38 AM, Nick Desaulniers <ndesaulniers@google.com> wrote: > I think Kees' proposal is a better solution; rather than require all > usage of device table to remember to add const, have the macro add it > for all users. Otherwise if you require caller's to add it, they may > forget. And with the coccinelle script, it should be easy to invert the logic and remove const from the callers... -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE 2017-08-28 17:41 ` Kees Cook @ 2017-08-29 2:24 ` Stefan Agner 0 siblings, 0 replies; 7+ messages in thread From: Stefan Agner @ 2017-08-29 2:24 UTC (permalink / raw) To: Kees Cook, Nick Desaulniers Cc: Jessica Yu, Matthias Kaehlcke, Rusty Russell, LKML, Masahiro Yamada, Michal Marek, Arnd Bergmann, Doug Anderson, Grant Grundler, Greg Hackmann, Michael Davidson, Alexander Potapenko, Bernhard.Rosenkranzer, keescook On 2017-08-28 10:41, Kees Cook wrote: > On Mon, Aug 28, 2017 at 10:38 AM, Nick Desaulniers > <ndesaulniers@google.com> wrote: >> I think Kees' proposal is a better solution; rather than require all >> usage of device table to remember to add const, have the macro add it >> for all users. Otherwise if you require caller's to add it, they may >> forget. > > And with the coccinelle script, it should be easy to invert the logic > and remove const from the callers... > I tried to reproduce my findings again but was not successful :-( I must have changed .config or something in between and draw wrong conclusions... So removing the const in the module.h alias actually did not change anything... It did not help for drivers which forget to constify... I think even the alias in module.h was actually illegal according to C standard: (C89, 6.2.7p2) "All declarations that refer to the same object or function shall have compatible type; otherwise the behavior is undefined." I guess it would still make sense to constify the structs for most of the 620 drivers which do not have it const currently. I found some driver actually change the table at runtime, e.g. drivers/net/usb/pegasus.c, so we would have to exclude them. -- Stefan ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-08-29 2:26 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-25 1:27 [PATCH] module: Remove const attribute from alias for MODULE_DEVICE_TABLE Matthias Kaehlcke 2017-07-29 21:48 ` Jessica Yu 2017-08-27 23:52 ` Stefan Agner 2017-08-28 17:20 ` Kees Cook 2017-08-28 17:38 ` Nick Desaulniers 2017-08-28 17:41 ` Kees Cook 2017-08-29 2:24 ` Stefan Agner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.