[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 <m...@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

[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 <m...@chromium.org>

Applied, thanks!

Jessica

[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 <ste...@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

[Kees Cook]

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

[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.

--
Thanks,
~Nick Desaulniers

[Kees Cook]

On Mon, Aug 28, 2017 at 10:38 AM, Nick Desaulniers
<ndesau...@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...

[Stefan Agner]

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