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
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 <[email protected]>
---
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
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 <[email protected]> # 3.9
Signed-off-by: Johan Hovold <[email protected]>
---
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
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?
> 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/[email protected]/
Johan
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 <[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?
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.
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 <[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?
>
> 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]:
>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
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
On Fri, 6 Nov 2020 17:45:37 +0100
Johan Hovold <[email protected]> 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
On Fri, Nov 06, 2020 at 11:55:23AM -0500, Steven Rostedt wrote:
> On Fri, 6 Nov 2020 17:45:37 +0100
> Johan Hovold <[email protected]> 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
+++ 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/
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/
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
+++ 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
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
+++ 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