[PATCH] module: Remove const attribute from alias for MODULE_DEVICE_TABLE

[PATCH] module: Remove const attribute from alias for MODULE_DEVICE_TABLE

[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

Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE

[Jessica Yu]

+++ 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
>

Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE

[Stefan Agner]

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)

Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE

[Kees Cook]

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

Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE

[Nick Desaulniers]

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

Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE

[Kees Cook]

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

Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE

[Stefan Agner]

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