Re: module: Remove const attribute from alias for MODULE_DEVICE_TABLE

[Nick Desaulniers <ndesaulniers@google.com>]

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