Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1720944 > unrolled thread

Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE Stefan Agner <stefan@agner.ch> - 2017-08-28 02:00 +0200
    Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE Kees Cook <keescook@chromium.org> - 2017-08-28 19:30 +0200
      Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE Nick Desaulniers <ndesaulniers@google.com> - 2017-08-28 19:40 +0200
        Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE Kees Cook <keescook@chromium.org> - 2017-08-28 19:50 +0200
          Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE Stefan Agner <stefan@agner.ch> - 2017-08-29 04:30 +0200

#1720944 — Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE

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)

[toc] | [next] | [standalone]

#1721844

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

[toc] | [prev] | [next] | [standalone]

#1721848

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

[toc] | [prev] | [next] | [standalone]

#1721852

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

[toc] | [prev] | [next] | [standalone]

#1722116

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web