[PATCH 0/8] linker-section array fix and clean ups

[PATCH 0/8] linker-section array fix and clean ups

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

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/


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

[PATCH 1/8] of: fix linker-section match-table corruption

[Johan Hovold]

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.

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

[PATCH 2/8] earlycon: simplify earlycon-table implementation

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

Note that we have been relying on this gcc 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

[PATCH 3/8] module: drop version-attribute alignment

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

[PATCH 4/8] module: simplify version-attribute handling

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

Note that we have been relying on this gcc 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 293250958512..5958075ea3f4 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 3835fb82c64b..aa7d6f2213f1 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

[PATCH 5/8] init: use type alignment for kernel parameters

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

[PATCH 6/8] params: drop redundant "unused" attributes

[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

[PATCH 7/8] params: use type alignment for kernel parameters

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

[PATCH 8/8] params: clean up module-param macros

[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

get_maintainer.pl bug? (was: Re: [PATCH 0/8] linker-section array fix and clean ups)

[Johan Hovold]

Hi Joe,

Running scrips/get_maintainer.pl on this series [1] gave the wrong
address for Nick Desaulniers:

	Nick Desaulniers <ndesaulniers@gooogle.com> (commit_signer:1/2=50%,commit_signer:1/8=12%)

It seems he recently misspelled his address in a reviewed-by tag to
commit 33def8498fdd ("treewide: Convert macro and uses of __section(foo)
to __section("foo")") and that is now being picked up by the script.

I guess that's to be considered a bug?

> 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

[1] https://lore.kernel.org/lkml/20201103175711.10731-1-johan@kernel.org/

Johan

Re: get_maintainer.pl bug? (was: Re: [PATCH 0/8] linker-section array fix and clean ups)

[Joe Perches]

On Wed, 2020-11-04 at 10:16 +0100, Johan Hovold wrote:
> Running scrips/get_maintainer.pl on this series [1] gave the wrong
> address for Nick Desaulniers:
> 
> 	Nick Desaulniers <ndesaulniers@gooogle.com> (commit_signer:1/2=50%,commit_signer:1/8=12%)
> 
> It seems he recently misspelled his address in a reviewed-by tag to
> commit 33def8498fdd ("treewide: Convert macro and uses of __section(foo)
> to __section("foo")") and that is now being picked up by the script.
> 
> I guess that's to be considered a bug?

No, that's a feature.  If it's _really_ a problem, (and I don't
think it really is), that's what .mailmap is for.

Re: get_maintainer.pl bug? (was: Re: [PATCH 0/8] linker-section array fix and clean ups)

[Johan Hovold]

On Wed, Nov 04, 2020 at 04:04:00AM -0800, Joe Perches wrote:
> On Wed, 2020-11-04 at 10:16 +0100, Johan Hovold wrote:
> > Running scrips/get_maintainer.pl on this series [1] gave the wrong
> > address for Nick Desaulniers:
> > 
> > 	Nick Desaulniers <ndesaulniers@gooogle.com> (commit_signer:1/2=50%,commit_signer:1/8=12%)
> > 
> > It seems he recently misspelled his address in a reviewed-by tag to
> > commit 33def8498fdd ("treewide: Convert macro and uses of __section(foo)
> > to __section("foo")") and that is now being picked up by the script.
> > 
> > I guess that's to be considered a bug?
> 
> No, that's a feature.  If it's _really_ a problem, (and I don't
> think it really is), that's what .mailmap is for.

Ah, Nick doesn't actually have any commits touching these files; I was
confused by the "commit_signer" in the script output and didn't expect
Reviewed-by tags to be considered all (and at least not over SoB).

Hmm. Guess it's working as intended.

Johan

Re: [PATCH 0/8] linker-section array fix and clean ups

[Jessica Yu]

+++ Johan Hovold [03/11/20 18:57 +0100]:
>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.

>[2] https://lore.kernel.org/lkml/20110126222622.GA10794@Krystal/

Hi Johan,

So unfortunately, I am not familiar enough with the semantics of gcc's
aligned attribute. AFAICT from the patch you linked in [2], the
original purpose of the pointer indirection workaround was to avoid
relying on (potentially inconsistent) compiler-specific behavior with
respect to the aligned attribute. The main concern was potential
up-alignment being done by gcc (or the linker) despite the desired
alignment being specified. Indeed, the gcc documentation also states
that the aligned attribute only specifies the *minimum* alignment,
although there's no guarantee that up-alignment wouldn't occur.

So I guess my question is, is there some implicit guarantee that
specifying alignment by type via __alignof__ that's supposed to
prevent gcc from up-aligning? Or are we just assuming that gcc won't
increase the alignment? The gcc docs don't seem to clarify this
unfortunately.

Thanks,

Jessica

Re: [PATCH 0/8] linker-section array fix and clean ups

[Johan Hovold]

On Fri, Nov 06, 2020 at 05:03:45PM +0100, Jessica Yu wrote:
> +++ Johan Hovold [03/11/20 18:57 +0100]:
> >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.
> 
> >[2] https://lore.kernel.org/lkml/20110126222622.GA10794@Krystal/

> So unfortunately, I am not familiar enough with the semantics of gcc's
> aligned attribute. AFAICT from the patch you linked in [2], the
> original purpose of the pointer indirection workaround was to avoid
> relying on (potentially inconsistent) compiler-specific behavior with
> respect to the aligned attribute. The main concern was potential
> up-alignment being done by gcc (or the linker) despite the desired
> alignment being specified. Indeed, the gcc documentation also states
> that the aligned attribute only specifies the *minimum* alignment,
> although there's no guarantee that up-alignment wouldn't occur.
>
> So I guess my question is, is there some implicit guarantee that
> specifying alignment by type via __alignof__ that's supposed to
> prevent gcc from up-aligning? Or are we just assuming that gcc won't
> increase the alignment? The gcc docs don't seem to clarify this
> unfortunately.

It's simply specifying alignment when declaring the variable that
prevents this optimisation. The relevant code is in the function
align_variable() in [1] where DATA_ALIGNMENT() is never called in case
an alignment has been specified (!DECL_USER_ALIGN(decl)).

There's no mention in the documentation of this that I'm aware of, but
this is the way the aligned attribute has worked since its introduction
judging from the commit history.

As mentioned above, we've been relying on this for kernel parameters and
other structures since 2003-2004 so if it ever were to change we'd find
out soon enough.

It's about to be used for scheduler classes as well. [2]

Johan

[1] https://github.com/gcc-mirror/gcc/blob/master/gcc/varasm.c
[2] https://lore.kernel.org/r/160396870486.397.377616182428528939.tip-bot2@tip-bot2

Re: [PATCH 0/8] linker-section array fix and clean ups

[Steven Rostedt]

On Fri, 6 Nov 2020 17:45:37 +0100
Johan Hovold <johan@kernel.org> wrote:

> It's simply specifying alignment when declaring the variable that
> prevents this optimisation. The relevant code is in the function
> align_variable() in [1] where DATA_ALIGNMENT() is never called in case
> an alignment has been specified (!DECL_USER_ALIGN(decl)).
> 
> There's no mention in the documentation of this that I'm aware of, but
> this is the way the aligned attribute has worked since its introduction
> judging from the commit history.
> 
> As mentioned above, we've been relying on this for kernel parameters and
> other structures since 2003-2004 so if it ever were to change we'd find
> out soon enough.
> 
> It's about to be used for scheduler classes as well. [2]

Is this something that gcc folks are aware of? Yes, we appear to be relying
on undocumented implementations, but that hasn't caused gcc to break the
kernel in the past.

-- Steve

Re: [PATCH 0/8] linker-section array fix and clean ups

[Johan Hovold]

On Fri, Nov 06, 2020 at 11:55:23AM -0500, Steven Rostedt wrote:
> On Fri, 6 Nov 2020 17:45:37 +0100
> Johan Hovold <johan@kernel.org> wrote:
> 
> > It's simply specifying alignment when declaring the variable that
> > prevents this optimisation. The relevant code is in the function
> > align_variable() in [1] where DATA_ALIGNMENT() is never called in case
> > an alignment has been specified (!DECL_USER_ALIGN(decl)).
> > 
> > There's no mention in the documentation of this that I'm aware of, but
> > this is the way the aligned attribute has worked since its introduction
> > judging from the commit history.
> > 
> > As mentioned above, we've been relying on this for kernel parameters and
> > other structures since 2003-2004 so if it ever were to change we'd find
> > out soon enough.
> > 
> > It's about to be used for scheduler classes as well. [2]
> 
> Is this something that gcc folks are aware of? Yes, we appear to be relying
> on undocumented implementations, but that hasn't caused gcc to break the
> kernel in the past.

The scheduler change was suggested by Jakub so at least some of them
are.

Johan

Re: [PATCH 0/8] linker-section array fix and clean ups

[Jessica Yu]

+++ Johan Hovold [06/11/20 17:45 +0100]:
>On Fri, Nov 06, 2020 at 05:03:45PM +0100, Jessica Yu wrote:
>> +++ Johan Hovold [03/11/20 18:57 +0100]:
>> >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.
>>
>> >[2] https://lore.kernel.org/lkml/20110126222622.GA10794@Krystal/
>
>> So unfortunately, I am not familiar enough with the semantics of gcc's
>> aligned attribute. AFAICT from the patch you linked in [2], the
>> original purpose of the pointer indirection workaround was to avoid
>> relying on (potentially inconsistent) compiler-specific behavior with
>> respect to the aligned attribute. The main concern was potential
>> up-alignment being done by gcc (or the linker) despite the desired
>> alignment being specified. Indeed, the gcc documentation also states
>> that the aligned attribute only specifies the *minimum* alignment,
>> although there's no guarantee that up-alignment wouldn't occur.
>>
>> So I guess my question is, is there some implicit guarantee that
>> specifying alignment by type via __alignof__ that's supposed to
>> prevent gcc from up-aligning? Or are we just assuming that gcc won't
>> increase the alignment? The gcc docs don't seem to clarify this
>> unfortunately.
>
>It's simply specifying alignment when declaring the variable that
>prevents this optimisation. The relevant code is in the function
>align_variable() in [1] where DATA_ALIGNMENT() is never called in case
>an alignment has been specified (!DECL_USER_ALIGN(decl)).
>
>There's no mention in the documentation of this that I'm aware of, but
>this is the way the aligned attribute has worked since its introduction
>judging from the commit history.
>
>As mentioned above, we've been relying on this for kernel parameters and
>other structures since 2003-2004 so if it ever were to change we'd find
>out soon enough.
>
>It's about to be used for scheduler classes as well. [2]
>
>Johan
>
>[1] https://github.com/gcc-mirror/gcc/blob/master/gcc/varasm.c
>[2] https://lore.kernel.org/r/160396870486.397.377616182428528939.tip-bot2@tip-bot2

Thanks for providing the links and references. Your explanation and
this reply from Jakub [1] clarified things for me. I was not aware of
the distinction gcc made between aligned attributes on types vs. on
variables. So from what I understand now, gcc suppresses the
optimization when the alignment is specified in the variable
declaration, but not necessarily when the aligned attribute is just on
the type.

Even though it's been in use for a long time, I think it would be
really helpful if this gcc quirk was explained just a bit more in the
patch changelogs, especially since this is undocumented behavior.
I found the explanation in [1] (as well as in your cover letter) to be
sufficient. Maybe something like "GCC suppresses any optimizations
increasing alignment when the alignment is specified in the variable
declaration, as opposed to just on the type definition. Therefore,
explicitly specify type alignment when declaring entries to prevent
gcc from increasing alignment."

In any case, I can take the module and moduleparam.h patches through
my tree, but I will wait a few days in case there are any objections.

Thanks,

Jessica

[1] https://lore.kernel.org/lkml/20201021131806.GA2176@tucnak/

Re: [PATCH 0/8] linker-section array fix and clean ups

[Johan Hovold]

On Wed, Nov 11, 2020 at 04:47:16PM +0100, Jessica Yu wrote:

> Thanks for providing the links and references. Your explanation and
> this reply from Jakub [1] clarified things for me. I was not aware of
> the distinction gcc made between aligned attributes on types vs. on
> variables. So from what I understand now, gcc suppresses the
> optimization when the alignment is specified in the variable
> declaration, but not necessarily when the aligned attribute is just on
> the type.
> 
> Even though it's been in use for a long time, I think it would be
> really helpful if this gcc quirk was explained just a bit more in the
> patch changelogs, especially since this is undocumented behavior.
> I found the explanation in [1] (as well as in your cover letter) to be
> sufficient. Maybe something like "GCC suppresses any optimizations
> increasing alignment when the alignment is specified in the variable
> declaration, as opposed to just on the type definition. Therefore,
> explicitly specify type alignment when declaring entries to prevent
> gcc from increasing alignment."

Sure, I can try to expand the commit messages a bit.

> In any case, I can take the module and moduleparam.h patches through
> my tree, but I will wait a few days in case there are any objections.

Sounds good, thanks. I'll send a v2 next week then.

Johan

> [1] https://lore.kernel.org/lkml/20201021131806.GA2176@tucnak/

Re: [PATCH 0/8] linker-section array fix and clean ups

[Johan Hovold]

On Fri, Nov 13, 2020 at 03:18:36PM +0100, Johan Hovold wrote:
> On Wed, Nov 11, 2020 at 04:47:16PM +0100, Jessica Yu wrote:
> 
> > Thanks for providing the links and references. Your explanation and
> > this reply from Jakub [1] clarified things for me. I was not aware of
> > the distinction gcc made between aligned attributes on types vs. on
> > variables. So from what I understand now, gcc suppresses the
> > optimization when the alignment is specified in the variable
> > declaration, but not necessarily when the aligned attribute is just on
> > the type.
> > 
> > Even though it's been in use for a long time, I think it would be
> > really helpful if this gcc quirk was explained just a bit more in the
> > patch changelogs, especially since this is undocumented behavior.
> > I found the explanation in [1] (as well as in your cover letter) to be
> > sufficient. Maybe something like "GCC suppresses any optimizations
> > increasing alignment when the alignment is specified in the variable
> > declaration, as opposed to just on the type definition. Therefore,
> > explicitly specify type alignment when declaring entries to prevent
> > gcc from increasing alignment."
> 
> Sure, I can try to expand the commit messages a bit.

I've amended the commit messages of the relevant patches to make it more
clear that the optimisation can be suppressed by specifying alignment
when declaring variables, but without making additional claims about the
type attribute. I hope the result is acceptable to you.

Perhaps you can include a lore link to the patches when applying so that
this thread can be found easily if needed.

Johan

Re: [PATCH 0/8] linker-section array fix and clean ups

[Jessica Yu]

+++ Johan Hovold [23/11/20 11:39 +0100]:
>On Fri, Nov 13, 2020 at 03:18:36PM +0100, Johan Hovold wrote:
>> On Wed, Nov 11, 2020 at 04:47:16PM +0100, Jessica Yu wrote:
>>
>> > Thanks for providing the links and references. Your explanation and
>> > this reply from Jakub [1] clarified things for me. I was not aware of
>> > the distinction gcc made between aligned attributes on types vs. on
>> > variables. So from what I understand now, gcc suppresses the
>> > optimization when the alignment is specified in the variable
>> > declaration, but not necessarily when the aligned attribute is just on
>> > the type.
>> >
>> > Even though it's been in use for a long time, I think it would be
>> > really helpful if this gcc quirk was explained just a bit more in the
>> > patch changelogs, especially since this is undocumented behavior.
>> > I found the explanation in [1] (as well as in your cover letter) to be
>> > sufficient. Maybe something like "GCC suppresses any optimizations
>> > increasing alignment when the alignment is specified in the variable
>> > declaration, as opposed to just on the type definition. Therefore,
>> > explicitly specify type alignment when declaring entries to prevent
>> > gcc from increasing alignment."
>>
>> Sure, I can try to expand the commit messages a bit.
>
>I've amended the commit messages of the relevant patches to make it more
>clear that the optimisation can be suppressed by specifying alignment
>when declaring variables, but without making additional claims about the
>type attribute. I hope the result is acceptable to you.
>
>Perhaps you can include a lore link to the patches when applying so that
>this thread can be found easily if needed.

Hi Johan,

Good idea, I've included a link to this thread for each patch.
I've queued up patches 3, 4, 6, 7, 8 for testing before pushing them
out to modules-next.

Thanks!

Jessica

Re: [PATCH 0/8] linker-section array fix and clean ups

[Johan Hovold]

On Wed, Nov 25, 2020 at 03:51:20PM +0100, Jessica Yu wrote:

> I've queued up patches 3, 4, 6, 7, 8 for testing before pushing them
> out to modules-next.

Thanks, Jessica.

Perhaps you can consider taking also the one for setup parameters (patch
5/8) through your tree since its related to the module-parameter one.

Johan

Re: [PATCH 0/8] linker-section array fix and clean ups

[Jessica Yu]

+++ Johan Hovold [27/11/20 10:59 +0100]:
>On Wed, Nov 25, 2020 at 03:51:20PM +0100, Jessica Yu wrote:
>
>> I've queued up patches 3, 4, 6, 7, 8 for testing before pushing them
>> out to modules-next.
>
>Thanks, Jessica.
>
>Perhaps you can consider taking also the one for setup parameters (patch
>5/8) through your tree since its related to the module-parameter one.
>
>Johan

Sure, done.

Thanks,

Jessica