linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/8] linker-section array fix and clean ups
@ 2020-11-23 10:23 Johan Hovold
  2020-11-23 10:23 ` [PATCH v2 1/8] of: fix linker-section match-table corruption Johan Hovold
                   ` (7 more replies)
  0 siblings, 8 replies; 12+ messages in thread
From: Johan Hovold @ 2020-11-23 10:23 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Jessica Yu, Linus Torvalds
  Cc: Frank Rowand, Arnd Bergmann, Geert Uytterhoeven, Dmitry Torokhov,
	David Miller, Jakub Jelinek, Peter Zijlstra, Thomas Gleixner,
	Steven Rostedt, Daniel Kurtz, linux-arch, linux-m68k,
	linux-kernel, Johan Hovold

We rely on the linker to create arrays for a number of things including
kernel parameters and device-tree-match entries.

The stride of these linker-section arrays obviously needs to match the
expectations of the code accessing them or bad things will happen.

One thing to watch out for is that gcc is known to increase the
alignment of larger objects with static extent as an optimisation (on
x86), but this can be suppressed by using the aligned attribute when
declaring entries.

We've been relying on this behaviour for 16 years for kernel parameters
(and other structures) and it indeed hasn't changed since the
introduction of the aligned attribute in gcc 3.1 (see align_variable()
in [1]).

Occasionally this gcc optimisation do cause problems which have instead
been worked around in various creative ways including using indirection
through an array of pointers. This was originally done for tracepoints
[2] after a number of failed attempts to create properly aligned arrays,
and the approach was later reused for module-version attributes [3] and
earlycon entries.

This series reverts the latter two workarounds in favour of the one-line
fix of aligning the entries according to the requirement of the type.

In principle, there shouldn't be anything preventing us from doing the
same for tracepoints.

The key observation here is that the arrays should be constructed using
the alignment of the type in question (as given by __alignof__()) rather
than some specific alignment such as sizeof(void *). This allows the
structures to be stored efficiently, but more importantly prevents
breakage on architectures like m68k where pointers are 2-byte aligned
should the size or alignment of the type change (e.g. so that the size
is no longer divisible by four).

As a preventive measure in case the kernel-parameter structures are ever
amended (or the code pattern is reused elsewhere), the final patches
switches the parameter declarations to also use type alignment.

The series has been tested using gcc 4.9 and 9.3 on x86_32 and
x86_64 and using gcc 7.2 on arm; and has been compile-tested and
verified using gcc 4.9 and 10.1 on aarch64, sparc and m68k.

Note that the patches are mostly independent and can be merged through
different subsystem trees. I decided to post them as a series to provide
a common background and have a single thread for any general discussion.

Rob and Greg, can you take patches 1 and 2 through your trees,
respectively?

Jessica, you said you could take the module and params patches through
your tree?

Who picks up the init.h one? Linus?

Johan

[1] https://github.com/gcc-mirror/gcc/blob/master/gcc/varasm.c
[2] https://lore.kernel.org/lkml/20110126222622.GA10794@Krystal/
[3] https://lore.kernel.org/lkml/1297123347-2170-1-git-send-email-dtor@vmware.com/


Changes in v2
 - amend commit messages of patches 1, 2, 4, 5 and 7 slightly in order
   to make it more clear that the gcc optimisation is suppressed by
   specifying alignment when declaring variables

v1
 - https://lore.kernel.org/r/20201103175711.10731-1-johan@kernel.org


Johan Hovold (8):
  of: fix linker-section match-table corruption
  earlycon: simplify earlycon-table implementation
  module: drop version-attribute alignment
  module: simplify version-attribute handling
  init: use type alignment for kernel parameters
  params: drop redundant "unused" attributes
  params: use type alignment for kernel parameters
  params: clean up module-param macros

 drivers/of/fdt.c              |  7 ++-----
 drivers/tty/serial/earlycon.c |  6 ++----
 include/linux/init.h          |  2 +-
 include/linux/module.h        | 28 ++++++++++++++--------------
 include/linux/moduleparam.h   | 12 ++++++------
 include/linux/of.h            |  1 +
 include/linux/serial_core.h   | 20 +++++++-------------
 kernel/params.c               | 10 ++++------
 8 files changed, 37 insertions(+), 49 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/8] of: fix linker-section match-table corruption
  2020-11-23 10:23 [PATCH v2 0/8] linker-section array fix and clean ups Johan Hovold
@ 2020-11-23 10:23 ` Johan Hovold
  2020-12-04 14:02   ` Johan Hovold
  2020-11-23 10:23 ` [PATCH v2 2/8] earlycon: simplify earlycon-table implementation Johan Hovold
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 12+ messages in thread
From: Johan Hovold @ 2020-11-23 10:23 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Jessica Yu, Linus Torvalds
  Cc: Frank Rowand, Arnd Bergmann, Geert Uytterhoeven, Dmitry Torokhov,
	David Miller, Jakub Jelinek, Peter Zijlstra, Thomas Gleixner,
	Steven Rostedt, Daniel Kurtz, linux-arch, linux-m68k,
	linux-kernel, Johan Hovold, stable

Specify type alignment when declaring linker-section match-table entries
to prevent gcc from increasing alignment and corrupting the various
tables with padding (e.g. timers, irqchips, clocks, reserved memory).

This is specifically needed on x86 where gcc (typically) aligns larger
objects like struct of_device_id with static extent on 32-byte
boundaries which at best prevents matching on anything but the first
entry. Specifying alignment when declaring variables suppresses this
optimisation.

Here's a 64-bit example where all entries are corrupt as 16 bytes of
padding has been inserted before the first entry:

	ffffffff8266b4b0 D __clk_of_table
	ffffffff8266b4c0 d __of_table_fixed_factor_clk
	ffffffff8266b5a0 d __of_table_fixed_clk
	ffffffff8266b680 d __clk_of_table_sentinel

And here's a 32-bit example where the 8-byte-aligned table happens to be
placed on a 32-byte boundary so that all but the first entry are corrupt
due to the 28 bytes of padding inserted between entries:

	812b3ec0 D __irqchip_of_table
	812b3ec0 d __of_table_irqchip1
	812b3fa0 d __of_table_irqchip2
	812b4080 d __of_table_irqchip3
	812b4160 d irqchip_of_match_end

Verified on x86 using gcc-9.3 and gcc-4.9 (which uses 64-byte
alignment), and on arm using gcc-7.2.

Note that there are no in-tree users of these tables on x86 currently
(even if they are included in the image).

Fixes: 54196ccbe0ba ("of: consolidate linker section OF match table declarations")
Fixes: f6e916b82022 ("irqchip: add basic infrastructure")
Cc: stable <stable@vger.kernel.org>     # 3.9
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 include/linux/of.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index 5d51891cbf1a..af655d264f10 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1300,6 +1300,7 @@ static inline int of_get_available_child_count(const struct device_node *np)
 #define _OF_DECLARE(table, name, compat, fn, fn_type)			\
 	static const struct of_device_id __of_table_##name		\
 		__used __section("__" #table "_of_table")		\
+		__aligned(__alignof__(struct of_device_id))		\
 		 = { .compatible = compat,				\
 		     .data = (fn == (fn_type)NULL) ? fn : fn  }
 #else
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/8] earlycon: simplify earlycon-table implementation
  2020-11-23 10:23 [PATCH v2 0/8] linker-section array fix and clean ups Johan Hovold
  2020-11-23 10:23 ` [PATCH v2 1/8] of: fix linker-section match-table corruption Johan Hovold
@ 2020-11-23 10:23 ` Johan Hovold
  2020-12-04 14:03   ` Johan Hovold
  2020-11-23 10:23 ` [PATCH v2 3/8] module: drop version-attribute alignment Johan Hovold
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 12+ messages in thread
From: Johan Hovold @ 2020-11-23 10:23 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Jessica Yu, Linus Torvalds
  Cc: Frank Rowand, Arnd Bergmann, Geert Uytterhoeven, Dmitry Torokhov,
	David Miller, Jakub Jelinek, Peter Zijlstra, Thomas Gleixner,
	Steven Rostedt, Daniel Kurtz, linux-arch, linux-m68k,
	linux-kernel, Johan Hovold

Instead of using the array-of-pointers trick to avoid having gcc mess up
the earlycon array stride, specify type alignment when declaring entries
to prevent gcc from increasing alignment.

This is essentially an alternative (one-line) fix to the problem
addressed by commit dd709e72cb93 ("earlycon: Use a pointer table to fix
__earlycon_table stride").

gcc can increase the alignment of larger objects with static extent as
an optimisation, but this can be suppressed by using the aligned
attribute when declaring variables.

Note that we have been relying on this behaviour for kernel parameters
for 16 years and it indeed hasn't changed since the introduction of the
aligned attribute in gcc-3.1.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/of/fdt.c              |  7 ++-----
 drivers/tty/serial/earlycon.c |  6 ++----
 include/linux/serial_core.h   | 20 +++++++-------------
 3 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 4602e467ca8b..feb0f2d67fc5 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -906,7 +906,7 @@ int __init early_init_dt_scan_chosen_stdout(void)
 	int offset;
 	const char *p, *q, *options = NULL;
 	int l;
-	const struct earlycon_id **p_match;
+	const struct earlycon_id *match;
 	const void *fdt = initial_boot_params;
 
 	offset = fdt_path_offset(fdt, "/chosen");
@@ -933,10 +933,7 @@ int __init early_init_dt_scan_chosen_stdout(void)
 		return 0;
 	}
 
-	for (p_match = __earlycon_table; p_match < __earlycon_table_end;
-	     p_match++) {
-		const struct earlycon_id *match = *p_match;
-
+	for (match = __earlycon_table; match < __earlycon_table_end; match++) {
 		if (!match->compatible[0])
 			continue;
 
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index b70877932d47..57c70851f22a 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -175,7 +175,7 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
  */
 int __init setup_earlycon(char *buf)
 {
-	const struct earlycon_id **p_match;
+	const struct earlycon_id *match;
 	bool empty_compatible = true;
 
 	if (!buf || !buf[0])
@@ -185,9 +185,7 @@ int __init setup_earlycon(char *buf)
 		return -EALREADY;
 
 again:
-	for (p_match = __earlycon_table; p_match < __earlycon_table_end;
-	     p_match++) {
-		const struct earlycon_id *match = *p_match;
+	for (match = __earlycon_table; match < __earlycon_table_end; match++) {
 		size_t len = strlen(match->name);
 
 		if (strncmp(buf, match->name, len))
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index ff63c2963359..3e32b788c28d 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -357,8 +357,8 @@ struct earlycon_id {
 	int	(*setup)(struct earlycon_device *, const char *options);
 };
 
-extern const struct earlycon_id *__earlycon_table[];
-extern const struct earlycon_id *__earlycon_table_end[];
+extern const struct earlycon_id __earlycon_table[];
+extern const struct earlycon_id __earlycon_table_end[];
 
 #if defined(CONFIG_SERIAL_EARLYCON) && !defined(MODULE)
 #define EARLYCON_USED_OR_UNUSED	__used
@@ -366,19 +366,13 @@ extern const struct earlycon_id *__earlycon_table_end[];
 #define EARLYCON_USED_OR_UNUSED	__maybe_unused
 #endif
 
-#define _OF_EARLYCON_DECLARE(_name, compat, fn, unique_id)		\
-	static const struct earlycon_id unique_id			\
-	     EARLYCON_USED_OR_UNUSED __initconst			\
+#define OF_EARLYCON_DECLARE(_name, compat, fn)				\
+	static const struct earlycon_id __UNIQUE_ID(__earlycon_##_name) \
+		EARLYCON_USED_OR_UNUSED  __section("__earlycon_table")  \
+		__aligned(__alignof__(struct earlycon_id))		\
 		= { .name = __stringify(_name),				\
 		    .compatible = compat,				\
-		    .setup = fn  };					\
-	static const struct earlycon_id EARLYCON_USED_OR_UNUSED		\
-		__section("__earlycon_table")				\
-		* const __PASTE(__p, unique_id) = &unique_id
-
-#define OF_EARLYCON_DECLARE(_name, compat, fn)				\
-	_OF_EARLYCON_DECLARE(_name, compat, fn,				\
-			     __UNIQUE_ID(__earlycon_##_name))
+		    .setup = fn };
 
 #define EARLYCON_DECLARE(_name, fn)	OF_EARLYCON_DECLARE(_name, "", fn)
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 3/8] module: drop version-attribute alignment
  2020-11-23 10:23 [PATCH v2 0/8] linker-section array fix and clean ups Johan Hovold
  2020-11-23 10:23 ` [PATCH v2 1/8] of: fix linker-section match-table corruption Johan Hovold
  2020-11-23 10:23 ` [PATCH v2 2/8] earlycon: simplify earlycon-table implementation Johan Hovold
@ 2020-11-23 10:23 ` Johan Hovold
  2020-11-23 10:23 ` [PATCH v2 4/8] module: simplify version-attribute handling Johan Hovold
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 12+ messages in thread
From: Johan Hovold @ 2020-11-23 10:23 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Jessica Yu, Linus Torvalds
  Cc: Frank Rowand, Arnd Bergmann, Geert Uytterhoeven, Dmitry Torokhov,
	David Miller, Jakub Jelinek, Peter Zijlstra, Thomas Gleixner,
	Steven Rostedt, Daniel Kurtz, linux-arch, linux-m68k,
	linux-kernel, Johan Hovold

Commit 98562ad8cb03 ("module: explicitly align module_version_attribute
structure") added an alignment attribute to the struct
module_version_attribute type in order to fix an alignment issue on m68k
where the structure is 2-byte aligned while MODULE_VERSION() forced the
__modver section entries to be 4-byte aligned (sizeof(void *)).

This was essentially an alternative fix to the problem addressed by
b4bc842802db ("module: deal with alignment issues in built-in module
versions") which used the array-of-pointer trick to prevent gcc from
increasing alignment of the version attribute entries. And with the
pointer indirection in place there's no need to increase the alignment
of the type.

Signed-off-by: Johan Hovold <johan@kernel.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 6264617bab4d..735f2931ea47 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -66,7 +66,7 @@ struct module_version_attribute {
 	struct module_attribute mattr;
 	const char *module_name;
 	const char *version;
-} __attribute__ ((__aligned__(sizeof(void *))));
+};
 
 extern ssize_t __modver_version_show(struct module_attribute *,
 				     struct module_kobject *, char *);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 4/8] module: simplify version-attribute handling
  2020-11-23 10:23 [PATCH v2 0/8] linker-section array fix and clean ups Johan Hovold
                   ` (2 preceding siblings ...)
  2020-11-23 10:23 ` [PATCH v2 3/8] module: drop version-attribute alignment Johan Hovold
@ 2020-11-23 10:23 ` Johan Hovold
  2020-11-23 10:23 ` [PATCH v2 5/8] init: use type alignment for kernel parameters Johan Hovold
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 12+ messages in thread
From: Johan Hovold @ 2020-11-23 10:23 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Jessica Yu, Linus Torvalds
  Cc: Frank Rowand, Arnd Bergmann, Geert Uytterhoeven, Dmitry Torokhov,
	David Miller, Jakub Jelinek, Peter Zijlstra, Thomas Gleixner,
	Steven Rostedt, Daniel Kurtz, linux-arch, linux-m68k,
	linux-kernel, Johan Hovold

Instead of using the array-of-pointers trick to avoid having gcc mess up
the built-in module-version array stride, specify type alignment when
declaring entries to prevent gcc from increasing alignment.

This is essentially an alternative (one-line) fix to the problem
addressed by commit b4bc842802db ("module: deal with alignment issues in
built-in module versions").

gcc can increase the alignment of larger objects with static extent as
an optimisation, but this can be suppressed by using the aligned
attribute when declaring variables.

Note that we have been relying on this behaviour for kernel parameters
for 16 years and it indeed hasn't changed since the introduction of the
aligned attribute in gcc-3.1.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 include/linux/module.h | 26 +++++++++++++-------------
 kernel/params.c        | 10 ++++------
 2 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 735f2931ea47..ebe2641d7b0b 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -266,20 +266,20 @@ extern typeof(name) __mod_##type##__##name##_device_table		\
 #else
 #define MODULE_VERSION(_version)					\
 	MODULE_INFO(version, _version);					\
-	static struct module_version_attribute ___modver_attr = {	\
-		.mattr	= {						\
-			.attr	= {					\
-				.name	= "version",			\
-				.mode	= S_IRUGO,			\
+	static struct module_version_attribute __modver_attr		\
+		__used __section("__modver")				\
+		__aligned(__alignof__(struct module_version_attribute)) \
+		= {							\
+			.mattr	= {					\
+				.attr	= {				\
+					.name	= "version",		\
+					.mode	= S_IRUGO,		\
+				},					\
+				.show	= __modver_version_show,	\
 			},						\
-			.show	= __modver_version_show,		\
-		},							\
-		.module_name	= KBUILD_MODNAME,			\
-		.version	= _version,				\
-	};								\
-	static const struct module_version_attribute			\
-	__used __section("__modver")					\
-	* __moduleparam_const __modver_attr = &___modver_attr
+			.module_name	= KBUILD_MODNAME,		\
+			.version	= _version,			\
+		};
 #endif
 
 /* Optional firmware file (or files) needed by the module
diff --git a/kernel/params.c b/kernel/params.c
index 164d79330849..2daa2780a92c 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -843,18 +843,16 @@ ssize_t __modver_version_show(struct module_attribute *mattr,
 	return scnprintf(buf, PAGE_SIZE, "%s\n", vattr->version);
 }
 
-extern const struct module_version_attribute *__start___modver[];
-extern const struct module_version_attribute *__stop___modver[];
+extern const struct module_version_attribute __start___modver[];
+extern const struct module_version_attribute __stop___modver[];
 
 static void __init version_sysfs_builtin(void)
 {
-	const struct module_version_attribute **p;
+	const struct module_version_attribute *vattr;
 	struct module_kobject *mk;
 	int err;
 
-	for (p = __start___modver; p < __stop___modver; p++) {
-		const struct module_version_attribute *vattr = *p;
-
+	for (vattr = __start___modver; vattr < __stop___modver; vattr++) {
 		mk = locate_module_kobject(vattr->module_name);
 		if (mk) {
 			err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 5/8] init: use type alignment for kernel parameters
  2020-11-23 10:23 [PATCH v2 0/8] linker-section array fix and clean ups Johan Hovold
                   ` (3 preceding siblings ...)
  2020-11-23 10:23 ` [PATCH v2 4/8] module: simplify version-attribute handling Johan Hovold
@ 2020-11-23 10:23 ` Johan Hovold
  2020-11-23 10:23 ` [PATCH v2 6/8] params: drop redundant "unused" attributes Johan Hovold
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 12+ messages in thread
From: Johan Hovold @ 2020-11-23 10:23 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Jessica Yu, Linus Torvalds
  Cc: Frank Rowand, Arnd Bergmann, Geert Uytterhoeven, Dmitry Torokhov,
	David Miller, Jakub Jelinek, Peter Zijlstra, Thomas Gleixner,
	Steven Rostedt, Daniel Kurtz, linux-arch, linux-m68k,
	linux-kernel, Johan Hovold

Specify type alignment for kernel parameters instead of sizeof(long).

The alignment attribute is used to prevent gcc from increasing the
alignment of objects with static extent as an optimisation, something
which would mess up the __setup array stride.

Using __alignof__(struct obs_kernel_param) rather than sizeof(long) is
preferred since it better indicates why it is there and doesn't break
should the type size or alignment change.

Note that on m68k the alignment of struct obs_kernel_param is actually
two and that adding a 1- or 2-byte field to the 12-byte struct would
cause a breakage with the current 4-byte alignment.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 include/linux/init.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/init.h b/include/linux/init.h
index 7b53cb3092ee..e668832ef66a 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -255,7 +255,7 @@ struct obs_kernel_param {
 		__aligned(1) = str; 					\
 	static struct obs_kernel_param __setup_##unique_id		\
 		__used __section(".init.setup")				\
-		__attribute__((aligned((sizeof(long)))))		\
+		__aligned(__alignof__(struct obs_kernel_param))		\
 		= { __setup_str_##unique_id, fn, early }
 
 #define __setup(str, fn)						\
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 6/8] params: drop redundant "unused" attributes
  2020-11-23 10:23 [PATCH v2 0/8] linker-section array fix and clean ups Johan Hovold
                   ` (4 preceding siblings ...)
  2020-11-23 10:23 ` [PATCH v2 5/8] init: use type alignment for kernel parameters Johan Hovold
@ 2020-11-23 10:23 ` Johan Hovold
  2020-11-23 10:23 ` [PATCH v2 7/8] params: use type alignment for kernel parameters Johan Hovold
  2020-11-23 10:23 ` [PATCH v2 8/8] params: clean up module-param macros Johan Hovold
  7 siblings, 0 replies; 12+ messages in thread
From: Johan Hovold @ 2020-11-23 10:23 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Jessica Yu, Linus Torvalds
  Cc: Frank Rowand, Arnd Bergmann, Geert Uytterhoeven, Dmitry Torokhov,
	David Miller, Jakub Jelinek, Peter Zijlstra, Thomas Gleixner,
	Steven Rostedt, Daniel Kurtz, linux-arch, linux-m68k,
	linux-kernel, Johan Hovold

Drop the redundant "unused" attributes from module-parameter structures
already marked "used".

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 include/linux/moduleparam.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 6388eb9734a5..742074ad9f6e 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -22,7 +22,7 @@
 
 #define __MODULE_INFO(tag, name, info)					  \
 static const char __UNIQUE_ID(name)[]					  \
-  __used __section(".modinfo") __attribute__((unused, aligned(1)))	  \
+  __used __section(".modinfo") __attribute__((aligned(1)))		  \
   = __MODULE_INFO_PREFIX __stringify(tag) "=" info
 
 #define __MODULE_PARM_TYPE(name, _type)					  \
@@ -289,7 +289,7 @@ struct kparam_array
 	static const char __param_str_##name[] = prefix #name;		\
 	static struct kernel_param __moduleparam_const __param_##name	\
 	__used								\
-    __section("__param") __attribute__ ((unused, aligned(sizeof(void *)))) \
+	__section("__param") __attribute__ ((aligned(sizeof(void *))))  \
 	= { __param_str_##name, THIS_MODULE, ops,			\
 	    VERIFY_OCTAL_PERMISSIONS(perm), level, flags, { arg } }
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 7/8] params: use type alignment for kernel parameters
  2020-11-23 10:23 [PATCH v2 0/8] linker-section array fix and clean ups Johan Hovold
                   ` (5 preceding siblings ...)
  2020-11-23 10:23 ` [PATCH v2 6/8] params: drop redundant "unused" attributes Johan Hovold
@ 2020-11-23 10:23 ` Johan Hovold
  2020-11-23 10:23 ` [PATCH v2 8/8] params: clean up module-param macros Johan Hovold
  7 siblings, 0 replies; 12+ messages in thread
From: Johan Hovold @ 2020-11-23 10:23 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Jessica Yu, Linus Torvalds
  Cc: Frank Rowand, Arnd Bergmann, Geert Uytterhoeven, Dmitry Torokhov,
	David Miller, Jakub Jelinek, Peter Zijlstra, Thomas Gleixner,
	Steven Rostedt, Daniel Kurtz, linux-arch, linux-m68k,
	linux-kernel, Johan Hovold

Specify type alignment for kernel parameters instead of sizeof(void *).

The alignment attribute is used to prevent gcc from increasing the
alignment of objects with static extent as an optimisation, something
which would mess up the __param array stride.

Using __alignof__(struct kernel_param) rather than sizeof(void *) is
preferred since it better indicates why it is there and doesn't break
should the type size or alignment change.

Note that on m68k the alignment of struct kernel_param is actually two
and that adding a 1- or 2-byte field to the 20-byte struct would cause a
breakage with the current 4-byte alignment.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 include/linux/moduleparam.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 742074ad9f6e..15ecc6cc3a3b 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -288,8 +288,8 @@ struct kparam_array
 	/* Default value instead of permissions? */			\
 	static const char __param_str_##name[] = prefix #name;		\
 	static struct kernel_param __moduleparam_const __param_##name	\
-	__used								\
-	__section("__param") __attribute__ ((aligned(sizeof(void *))))  \
+	__used __section("__param")					\
+	__aligned(__alignof__(struct kernel_param))			\
 	= { __param_str_##name, THIS_MODULE, ops,			\
 	    VERIFY_OCTAL_PERMISSIONS(perm), level, flags, { arg } }
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 8/8] params: clean up module-param macros
  2020-11-23 10:23 [PATCH v2 0/8] linker-section array fix and clean ups Johan Hovold
                   ` (6 preceding siblings ...)
  2020-11-23 10:23 ` [PATCH v2 7/8] params: use type alignment for kernel parameters Johan Hovold
@ 2020-11-23 10:23 ` Johan Hovold
  7 siblings, 0 replies; 12+ messages in thread
From: Johan Hovold @ 2020-11-23 10:23 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Jessica Yu, Linus Torvalds
  Cc: Frank Rowand, Arnd Bergmann, Geert Uytterhoeven, Dmitry Torokhov,
	David Miller, Jakub Jelinek, Peter Zijlstra, Thomas Gleixner,
	Steven Rostedt, Daniel Kurtz, linux-arch, linux-m68k,
	linux-kernel, Johan Hovold

Clean up the module-param macros by adding some indentation and using
the __aligned() macro to improve readability.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 include/linux/moduleparam.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 15ecc6cc3a3b..eed280fae433 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -21,12 +21,12 @@
 #define MAX_PARAM_PREFIX_LEN (64 - sizeof(unsigned long))
 
 #define __MODULE_INFO(tag, name, info)					  \
-static const char __UNIQUE_ID(name)[]					  \
-  __used __section(".modinfo") __attribute__((aligned(1)))		  \
-  = __MODULE_INFO_PREFIX __stringify(tag) "=" info
+	static const char __UNIQUE_ID(name)[]				  \
+		__used __section(".modinfo") __aligned(1)		  \
+		= __MODULE_INFO_PREFIX __stringify(tag) "=" info
 
 #define __MODULE_PARM_TYPE(name, _type)					  \
-  __MODULE_INFO(parmtype, name##type, #name ":" _type)
+	__MODULE_INFO(parmtype, name##type, #name ":" _type)
 
 /* One for each parameter, describing how to use it.  Some files do
    multiple of these per line, so can't just use MODULE_INFO. */
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/8] of: fix linker-section match-table corruption
  2020-11-23 10:23 ` [PATCH v2 1/8] of: fix linker-section match-table corruption Johan Hovold
@ 2020-12-04 14:02   ` Johan Hovold
  0 siblings, 0 replies; 12+ messages in thread
From: Johan Hovold @ 2020-12-04 14:02 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Jessica Yu, Linus Torvalds
  Cc: Frank Rowand, Arnd Bergmann, Geert Uytterhoeven, Dmitry Torokhov,
	David Miller, Jakub Jelinek, Peter Zijlstra, Thomas Gleixner,
	Steven Rostedt, Daniel Kurtz, linux-arch, linux-m68k,
	linux-kernel, Johan Hovold, stable

Rob,

On Mon, Nov 23, 2020 at 11:23:12AM +0100, Johan Hovold wrote:
> Specify type alignment when declaring linker-section match-table entries
> to prevent gcc from increasing alignment and corrupting the various
> tables with padding (e.g. timers, irqchips, clocks, reserved memory).
> 
> This is specifically needed on x86 where gcc (typically) aligns larger
> objects like struct of_device_id with static extent on 32-byte
> boundaries which at best prevents matching on anything but the first
> entry. Specifying alignment when declaring variables suppresses this
> optimisation.
> 
> Here's a 64-bit example where all entries are corrupt as 16 bytes of
> padding has been inserted before the first entry:
> 
> 	ffffffff8266b4b0 D __clk_of_table
> 	ffffffff8266b4c0 d __of_table_fixed_factor_clk
> 	ffffffff8266b5a0 d __of_table_fixed_clk
> 	ffffffff8266b680 d __clk_of_table_sentinel
> 
> And here's a 32-bit example where the 8-byte-aligned table happens to be
> placed on a 32-byte boundary so that all but the first entry are corrupt
> due to the 28 bytes of padding inserted between entries:
> 
> 	812b3ec0 D __irqchip_of_table
> 	812b3ec0 d __of_table_irqchip1
> 	812b3fa0 d __of_table_irqchip2
> 	812b4080 d __of_table_irqchip3
> 	812b4160 d irqchip_of_match_end
> 
> Verified on x86 using gcc-9.3 and gcc-4.9 (which uses 64-byte
> alignment), and on arm using gcc-7.2.
> 
> Note that there are no in-tree users of these tables on x86 currently
> (even if they are included in the image).
> 
> Fixes: 54196ccbe0ba ("of: consolidate linker section OF match table declarations")
> Fixes: f6e916b82022 ("irqchip: add basic infrastructure")
> Cc: stable <stable@vger.kernel.org>     # 3.9
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---

Could you pick this one up for 5.11?

Johan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/8] earlycon: simplify earlycon-table implementation
  2020-11-23 10:23 ` [PATCH v2 2/8] earlycon: simplify earlycon-table implementation Johan Hovold
@ 2020-12-04 14:03   ` Johan Hovold
  2020-12-04 14:44     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 12+ messages in thread
From: Johan Hovold @ 2020-12-04 14:03 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Jessica Yu, Linus Torvalds
  Cc: Frank Rowand, Arnd Bergmann, Geert Uytterhoeven, Dmitry Torokhov,
	David Miller, Jakub Jelinek, Peter Zijlstra, Thomas Gleixner,
	Steven Rostedt, Daniel Kurtz, linux-arch, linux-m68k,
	linux-kernel, Johan Hovold

Greg,

On Mon, Nov 23, 2020 at 11:23:13AM +0100, Johan Hovold wrote:
> Instead of using the array-of-pointers trick to avoid having gcc mess up
> the earlycon array stride, specify type alignment when declaring entries
> to prevent gcc from increasing alignment.
> 
> This is essentially an alternative (one-line) fix to the problem
> addressed by commit dd709e72cb93 ("earlycon: Use a pointer table to fix
> __earlycon_table stride").
> 
> gcc can increase the alignment of larger objects with static extent as
> an optimisation, but this can be suppressed by using the aligned
> attribute when declaring variables.
> 
> Note that we have been relying on this behaviour for kernel parameters
> for 16 years and it indeed hasn't changed since the introduction of the
> aligned attribute in gcc-3.1.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>

Could you pick this one up for 5.11?

Johan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/8] earlycon: simplify earlycon-table implementation
  2020-12-04 14:03   ` Johan Hovold
@ 2020-12-04 14:44     ` Greg Kroah-Hartman
  0 siblings, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2020-12-04 14:44 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Rob Herring, Jessica Yu, Linus Torvalds, Frank Rowand,
	Arnd Bergmann, Geert Uytterhoeven, Dmitry Torokhov, David Miller,
	Jakub Jelinek, Peter Zijlstra, Thomas Gleixner, Steven Rostedt,
	Daniel Kurtz, linux-arch, linux-m68k, linux-kernel

On Fri, Dec 04, 2020 at 03:03:45PM +0100, Johan Hovold wrote:
> Greg,
> 
> On Mon, Nov 23, 2020 at 11:23:13AM +0100, Johan Hovold wrote:
> > Instead of using the array-of-pointers trick to avoid having gcc mess up
> > the earlycon array stride, specify type alignment when declaring entries
> > to prevent gcc from increasing alignment.
> > 
> > This is essentially an alternative (one-line) fix to the problem
> > addressed by commit dd709e72cb93 ("earlycon: Use a pointer table to fix
> > __earlycon_table stride").
> > 
> > gcc can increase the alignment of larger objects with static extent as
> > an optimisation, but this can be suppressed by using the aligned
> > attribute when declaring variables.
> > 
> > Note that we have been relying on this behaviour for kernel parameters
> > for 16 years and it indeed hasn't changed since the introduction of the
> > aligned attribute in gcc-3.1.
> > 
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> 
> Could you pick this one up for 5.11?

Sure, will pick this and patch 1/8 up now, thanks for the prod.

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-12-04 14:44 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-23 10:23 [PATCH v2 0/8] linker-section array fix and clean ups Johan Hovold
2020-11-23 10:23 ` [PATCH v2 1/8] of: fix linker-section match-table corruption Johan Hovold
2020-12-04 14:02   ` Johan Hovold
2020-11-23 10:23 ` [PATCH v2 2/8] earlycon: simplify earlycon-table implementation Johan Hovold
2020-12-04 14:03   ` Johan Hovold
2020-12-04 14:44     ` Greg Kroah-Hartman
2020-11-23 10:23 ` [PATCH v2 3/8] module: drop version-attribute alignment Johan Hovold
2020-11-23 10:23 ` [PATCH v2 4/8] module: simplify version-attribute handling Johan Hovold
2020-11-23 10:23 ` [PATCH v2 5/8] init: use type alignment for kernel parameters Johan Hovold
2020-11-23 10:23 ` [PATCH v2 6/8] params: drop redundant "unused" attributes Johan Hovold
2020-11-23 10:23 ` [PATCH v2 7/8] params: use type alignment for kernel parameters Johan Hovold
2020-11-23 10:23 ` [PATCH v2 8/8] params: clean up module-param macros Johan Hovold

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).