linker-section array fix and clean ups

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/[email protected]/

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

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

---
 include/linux/of.h | 1 +
 1 file changed, 1 insertion(+)

index 5d51891cbf1a..af655d264f10 100644
@@ -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

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.

---
 drivers/of/fdt.c              |  7 ++-----
 drivers/tty/serial/earlycon.c |  6 ++----
 include/linux/serial_core.h   | 20 +++++++-------------
 3 files changed, 11 insertions(+), 22 deletions(-)

index 4602e467ca8b..feb0f2d67fc5 100644
@@ -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;
 

index b70877932d47..57c70851f22a 100644
@@ -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))

index ff63c2963359..3e32b788c28d 100644
@@ -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

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.

---
 include/linux/module.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 7ccdf87f376f..293250958512 100644
@@ -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

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.

---
 include/linux/module.h | 26 +++++++++++++-------------
 kernel/params.c        | 10 ++++------
 2 files changed, 17 insertions(+), 19 deletions(-)

index 293250958512..5958075ea3f4 100644
@@ -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

index 3835fb82c64b..aa7d6f2213f1 100644
@@ -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

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.

---
 include/linux/init.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 7b53cb3092ee..e668832ef66a 100644
@@ -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

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

---
 include/linux/moduleparam.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 6388eb9734a5..742074ad9f6e 100644
@@ -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

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.

---
 include/linux/moduleparam.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 742074ad9f6e..15ecc6cc3a3b 100644
@@ -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

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

---
 include/linux/moduleparam.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 15ecc6cc3a3b..eed280fae433 100644
@@ -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

Hi Joe,

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

	Nick Desaulniers <[email protected]> (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?

[1] https://lore.kernel.org/lkml/[email protected]/

Johan

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

+++ Johan Hovold [03/11/20 18:57 +0100]:

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

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

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

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

Johan

+++ Johan Hovold [06/11/20 17:45 +0100]:

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/

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

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

Johan

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

+++ Johan Hovold [23/11/20 11:39 +0100]:

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

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

+++ Johan Hovold [27/11/20 10:59 +0100]:

Sure, done.

Thanks,

Jessica