module: deal with alignment issues in built-in module versions

On m68k natural alignment is 2-byte boundary but we are trying to align structures in __modver section on sizeof(void *) boundary. This causes trouble when we try to access elements in this section in array-like fashion when create "version" attributes for built-in modules.

Moreover, as DaveM said, we can't reliably put structures into independent objects, put them into a special section, and then expect array access over them (via the section boundaries) after linking the objects together to just "work" due to variable alignment choices in different situations. The only solution that seems to work reliably is to make an array of plain pointers to the objects in question and put those pointers in the special section.

Reported-by Geert Uytterhoeven <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---
 include/linux/module.h |   10 +++++-----
 kernel/params.c        |    9 ++++++---

 2 files changed, 11 insertions(+), 8 deletions(-)

include/linux/module.h +5 -5

index 9bdf27c..9b7081a 100644
@@ -174,10 +174,7 @@ extern struct module __this_module;
  #define MODULE_VERSION(_version)                                       \
        extern ssize_t __modver_version_show(struct module_attribute *, \
                                             struct module *, char *);  \
-       static struct module_version_attribute __modver_version_attr    \
-       __used                                                          \
-    __attribute__ ((__section__ ("__modver"),aligned(sizeof(void *)))) \
-       = {                                                             \
+       static struct module_version_attribute ___modver_attr = {       \
                .mattr  = {                                             \
                        .attr   = {                                     \
                                .name   = "version",                    \

@@ -187,7 +184,10 @@ extern struct module __this_module;
                },                                                      \
                .module_name    = KBUILD_MODNAME,                       \
                .version        = _version,                             \
-       }
+       };                                                              \
+       static const struct module_version_attribute                    \
+       __used __attribute__ ((__section__ ("__modver")))               \
+       * __moduleparam_const __modver_attr = &___modver_attr
  #endif
 
  /* Optional firmware file (or files) needed by the module

kernel/params.c +6 -4

index 0da1411..09a08cb 100644
@@ -821,15 +821,18 @@ ssize_t __modver_version_show(struct module_attribute *mattr,
        return sprintf(buf, "%s\n", vattr->version);
  }
 
-extern struct module_version_attribute __start___modver[], __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 *vattr;
+       const struct module_version_attribute **p;
        struct module_kobject *mk;
        int err;
 
-       for (vattr = __start___modver; vattr < __stop___modver; vattr++) {
+       for (p = __start___modver; p < __stop___modver; p++) {
+               const struct module_version_attribute *vattr = *p;
+
                mk = locate_module_kobject(vattr->module_name);
                if (mk) {
                        err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
--

1.7.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 14 lines · Dmitry Torokhov

On m68k natural alignment is 2-byte boundary but we are trying to align structures in __modver section on sizeof(void *) boundary. This causes trouble when we try to access elements in this section in array-like fashion when create "version" attributes for built-in modules.

Moreover, as DaveM said, we can't reliably put structures into independent objects, put them into a special section, and then expect array access over them (via the section boundaries) after linking the objects together to just "work" due to variable alignment choices in different situations. The only solution that seems to work reliably is to make an array of plain pointers to the objects in question and put those pointers in the special section.

Quoted 2 lines

Reported-by Geert Uytterhoeven <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

Thanks!

Tested-by Geert Uytterhoeven <[email protected]>

Quoted 52 lines

---
 include/linux/module.h |   10 +++++-----
 kernel/params.c        |    9 ++++++---

 2 files changed, 11 insertions(+), 8 deletions(-)

include/linux/module.h +5 -5

index 9bdf27c..9b7081a 100644
@@ -174,10 +174,7 @@ extern struct module __this_module;
  #define MODULE_VERSION(_version)                                       \
        extern ssize_t __modver_version_show(struct module_attribute *, \
                                             struct module *, char *);  \
-       static struct module_version_attribute __modver_version_attr    \
-       __used                                                          \
-    __attribute__ ((__section__ ("__modver"),aligned(sizeof(void *)))) \
-       = {                                                             \
+       static struct module_version_attribute ___modver_attr = {       \
                .mattr  = {                                             \
                        .attr   = {                                     \
                                .name   = "version",                    \

@@ -187,7 +184,10 @@ extern struct module __this_module;
                },                                                      \
                .module_name    = KBUILD_MODNAME,                       \
                .version        = _version,                             \
-       }
+       };                                                              \
+       static const struct module_version_attribute                    \
+       __used __attribute__ ((__section__ ("__modver")))               \
+       * __moduleparam_const __modver_attr = &___modver_attr
  #endif
 
  /* Optional firmware file (or files) needed by the module

kernel/params.c +3 -2

index 0da1411..09a08cb 100644
@@ -821,15 +821,18 @@ ssize_t __modver_version_show(struct module_attribute *mattr,
        return sprintf(buf, "%s\n", vattr->version);
  }
 
-extern struct module_version_attribute __start___modver[], __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 *vattr;
+       const struct module_version_attribute **p;
        struct module_kobject *mk;
        int err;
 

Quoted 9 lines

kernel/params.c +3 -2

-       for (vattr = __start___modver; vattr < __stop___modver; vattr++) {
+       for (p = __start___modver; p < __stop___modver; p++) {
+               const struct module_version_attribute *vattr = *p;
+
                mk = locate_module_kobject(vattr->module_name);
                if (mk) {
                        err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
--

1.7.3.2

From 03f1332fc02af7af0d06ab409e3ec72107637845 Mon Sep 17 00:00:00 2001

Quoted 1 line

From 03f1332fc02af7af0d06ab409e3ec72107637845 Mon Sep 17 00:00:00 2001

From: Dmitry Torokhov <[email protected]>
Date: Mon, 7 Feb 2011 13:47:20 -0800
Subject: [PATCH] module: deal with alignment issues in built-in module parameters

As DaveM said, we can't reliably put structures into independent objects, put them into a special section, and then expect array access over them (via the section boundaries) after linking the objects together to just "work" due to variable alignment choices in different situations. The only solution that seems to work reliably is to make an array of plain pointers to the objects in question and put those pointers in the special section.

Moreover, as DaveM said, we can't reliably put structures into independent objects, put them into a special section, and then expect array access over them (via the section boundaries) after linking the objects together to just "work" due to variable alignment choices in different situations.

Quoted 6 lines · Dmitry Torokhov

Moreover, as DaveM said, we can't reliably put structures into independent objects, put them into a special section, and then expect array access over them (via the section boundaries) after linking the objects together to just "work" due to variable alignment choices in different situations.

Why not?

That's what we normally do. Just align the "__modver", and you should be all good. What's the problem?

From what I understand __attribute__ ((aligned(x))) only guarantees minimum alignment, not exact (gapless) alignment. GCC seems to lay out pointers in the section without gaps on all arches that we have.

Quoted 4 lines · Dmitry Torokhov

From what I understand __attribute__ ((aligned(x))) only guarantees minimum alignment, not exact (gapless) alignment. GCC seems to lay out pointers in the section without gaps on all arches that we have.

I still don't see the problem.

Have you actually _tried_ just adding the proper alignment to before the __modver thing in include/asm-generic/vmlinux.lds.h ?

As far as I can tell, the bug is really simple:
- the section is not aligned
- but we told the compiler that that structure is aligned

End result: there is a gap between the section start and the first structure.

And the fix seems obvious: just make sure that the section is sufficiently aligned.

IOW, why isn't the proper fix the obvious trivial one (attached)?

The problem is that on m68k size of the struct module_version_attribute is not evenly divisible by sizeof(void *), thus when we lay out the __modver section we align on 4 bytes but when we iterate we think that the alignment is 2.

I/Geert tried adding __attribute__ (aligned(sizeof(void *))) to the type definition itself so we have matching alignment everywhere, but DaveM said that this only guarantees minimum alignment and that using structures like we do shown to break from time to time in kprobes and that only pointers worked reliably.

David, why are you saying that regular "just mark the structure alignment correctly" doesn't work?

GCC is very clever with "static" objects these days.

EVERY SINGLE OF YOUR ARGUMENTS WORK FOR "pointer" TOO!

Quoted 1 line

EVERY SINGLE OF YOUR ARGUMENTS WORK FOR "pointer" TOO!

It at least will not happen at the current time, because GCC only plays these games on aggregates.

Also, for exception tables, we've avoided this problem because we emit the exception tables by hand using inline asm and therefore explicitly control all aspects of the alignment and size.

The GCC manual even documents the alignment attribute behavior.

Also, please don't shoot the messenger, I didn't make GCC behave this way but I doubt you'll have any luck undoing this behavior in the tools which therefore means as pragmatists we have to handle it one way or another.

Is there a -fdata-align or something? Or would __attribute__((packed)) help? Something that explicitly tells gcc "don't do this", instead of "let's add indirection and hope gcc doesn't add alignment for _that_". Especially as the extra pointer makes the code even uglier.

And if we do have to use the pointer thing, let's at least then do the pointer with asms, so that gcc _really_ can't screw it up. Rather than just move the potential bug around.

From: Linus Torvalds <[email protected]>
Date: Thu, 17 Feb 2011 13:54:57 -0800

Quoted 4 lines

Is there a -fdata-align or something? Or would __attribute__((packed)) help? Something that explicitly tells gcc "don't do this", instead of "let's add indirection and hope gcc doesn't add alignment for _that_". Especially as the extra pointer makes the code even uglier.

The tracing folks went down the path of trying to use packed in various ways, to no avail, because no matter what they tried it broke other things.

Quoted 3 lines

And if we do have to use the pointer thing, let's at least then do the pointer with asms, so that gcc _really_ can't screw it up. Rather than just move the potential bug around.

That's fine with me.

Any pointers as to how to emit these pointers with asm?

You have to also pick .word vs. .xword, or whatever the appropriate sized pointer mnenomic is for a given architecture.  I know .word works for 32-bit sparc, and .xword works for 64-bit sparc.

Gaah, I didn't realize that we've never had to do anything like this before, and that the exception table is all arch-specific code. So we don't have any way to "output that damned pointer and stop whining about it" model at all.

Where in gcc is the alignment expansion logic? I'd like to see what the rules are, just for my own perverse satisfaction.

Quoted 5 lines · Linus Torvalds

Gaah, I didn't realize that we've never had to do anything like this before, and that the exception table is all arch-specific code. So we don't have any way to "output that damned pointer and stop whining about it" model at all.

Actually, I don't think the problem is about ".word" vs ".xword". We should be able to just use ".long" everywhere.

But the symbol _name_ may have different prefixes, and when we use "asm()" at the top level, we can't use the expressions to fix it up. So a

asm(".long %0":'i" (symbol))

doesn't work (ignore the lack of section naming, that's not important), and neither can we just do something like

  #define output_asm_pointer(section, symbol) \
       asm(".long " #symbol)

portably, because some linker formats want to see prepended underscores etc.

Except that .long is 32-bit on ppc64 :-( You need .llong for 64-bit.

Quoted 1 line

Except that .long is 32-bit on ppc64 :-( You need .llong for 64-bit.

OK, all options suck.  Do we want the workaround or not?

Cheers,
Rusty.

Subject: module: deal with alignment issues in built-in module versions
Date: Mon, 7 Feb 2011 16:02:25 -0800
From: Dmitry Torokhov <[email protected]>

On m68k natural alignment is 2-byte boundary but we are trying to align structures in __modver section on sizeof(void *) boundary. This causes trouble when we try to access elements in this section in array-like fashion when create "version" attributes for built-in modules.

Moreover, as DaveM said, we can't reliably put structures into independent objects, put them into a special section, and then expect array access over them (via the section boundaries) after linking the objects together to just "work" due to variable alignment choices in different situations. The only solution that seems to work reliably is to make an array of plain pointers to the objects in question and put those pointers in the special section.

Reported-by Geert Uytterhoeven <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

Signed-off-by Rusty Russell <[email protected]>

---
 include/linux/module.h |   10 +++++-----
 kernel/params.c        |    9 ++++++---

 2 files changed, 11 insertions(+), 8 deletions(-)

include/linux/module.h +5 -5

index 9bdf27c..9b7081a 100644
@@ -174,10 +174,7 @@ extern struct module __this_module;
  #define MODULE_VERSION(_version)                                       \
        extern ssize_t __modver_version_show(struct module_attribute *, \
                                             struct module *, char *);  \
-       static struct module_version_attribute __modver_version_attr    \
-       __used                                                          \
-    __attribute__ ((__section__ ("__modver"),aligned(sizeof(void *)))) \
-       = {                                                             \
+       static struct module_version_attribute ___modver_attr = {       \
                .mattr  = {                                             \
                        .attr   = {                                     \
                                .name   = "version",                    \

@@ -187,7 +184,10 @@ extern struct module __this_module;
                },                                                      \
                .module_name    = KBUILD_MODNAME,                       \
                .version        = _version,                             \
-       }
+       };                                                              \
+       static const struct module_version_attribute                    \
+       __used __attribute__ ((__section__ ("__modver")))               \
+       * __moduleparam_const __modver_attr = &___modver_attr
  #endif
 
  /* Optional firmware file (or files) needed by the module

kernel/params.c +3 -2

index 0da1411..09a08cb 100644
@@ -821,15 +821,18 @@ ssize_t __modver_version_show(struct module_attribute *mattr,
        return sprintf(buf, "%s\n", vattr->version);
  }
 
-extern struct module_version_attribute __start___modver[], __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 *vattr;
+       const struct module_version_attribute **p;

Quoted 3 lines · Rusty Russell

Quoted 1 line

Except that .long is 32-bit on ppc64 :-( You need .llong for 64-bit.

OK, all options suck.  Do we want the workaround or not?

We can discuss about that until someone gets bitten by that.

But please fix the "aligned(sizeof(void *))"-in-one-place-only issue.

Quoted 3 lines · Geert Uytterhoeven

Quoted 2 lines · Rusty Russell

Quoted 1 line

Except that .long is 32-bit on ppc64 :-( You need .llong for 64-bit.

Quoted 6 lines

Quoted 1 line

OK, all options suck.  Do we want the workaround or not?

We can discuss about that until someone gets bitten by that.

But please fix the "aligned(sizeof(void *))"-in-one-place-only issue.

How about this one then?

From f0e0e10b58b22047e36e21a022abf5e86b5819c2 Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <[email protected]>
Date: Fri, 4 Feb 2011 13:30:10 -0800
Subject: [PATCH] module: explicitly align module_version_attribute structure

We force particular alignment when we generate attribute structures when generation MODULE_VERSION() data and we need to make sure that this alignment is followed when we iterate over these structures, otherwise we may crash on platforms whose natural alignment is not sizeof(void *), such as m68k.

Reported-by Geert Uytterhoeven <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---
 include/linux/module.h |    2 +-

 1 files changed, 1 insertions(+), 1 deletions(-)

include/linux/module.h +1 -1

index e7c6385..de5cd21 100644
@@ -62,7 +62,7 @@ struct module_version_attribute {
        struct module_attribute mattr;
        const char *module_name;
        const char *version;
-};
+} __attribute__ ((__aligned__(sizeof(void *))));
 

 struct module_kobject
 {
--
1.7.3.2

Quoted 1 line

Except that .long is 32-bit on ppc64 :-( You need .llong for 64-bit.

OK, all options suck. Do we want the workaround or not?

Cheers,
Rusty.

Subject: module: deal with alignment issues in built-in module versions
Date: Mon, 7 Feb 2011 16:02:25 -0800
From: Dmitry Torokhov <[email protected]>

On m68k natural alignment is 2-byte boundary but we are trying to align structures in __modver section on sizeof(void *) boundary. This causes trouble when we try to access elements in this section in array-like fashion when create "version" attributes for built-in modules.

Moreover, as DaveM said, we can't reliably put structures into independent objects, put them into a special section, and then expect array access over them (via the section boundaries) after linking the objects together to just "work" due to variable alignment choices in different situations. The only solution that seems to work reliably is to make an array of plain pointers to the objects in question and put those pointers in the special section.

Reported-by Geert Uytterhoeven <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

Signed-off-by Rusty Russell <[email protected]>

---
 include/linux/module.h |   10 +++++-----
 kernel/params.c        |    9 ++++++---
 2 files changed, 11 insertions(+), 8 deletions(-)

include/linux/module.h +5 -5

index 9bdf27c..9b7081a 100644
@@ -174,10 +174,7 @@ extern struct module __this_module;
 #define MODULE_VERSION(_version)					\
 	extern ssize_t __modver_version_show(struct module_attribute *,	\
 					     struct module *, char *);	\
-	static struct module_version_attribute __modver_version_attr	\
-	__used								\
-    __attribute__ ((__section__ ("__modver"),aligned(sizeof(void *)))) \
-	= {								\
+	static struct module_version_attribute ___modver_attr = {	\
 		.mattr	= {						\
 			.attr	= {					\
 				.name	= "version",			\

@@ -187,7 +184,10 @@ extern struct module __this_module;
 		},							\
 		.module_name	= KBUILD_MODNAME,			\
 		.version	= _version,				\
-	}
+	};								\
+	static const struct module_version_attribute			\
+	__used __attribute__ ((__section__ ("__modver")))		\
+	* __moduleparam_const __modver_attr = &___modver_attr
 #endif
 
 /* Optional firmware file (or files) needed by the module

kernel/params.c +6 -4

index 0da1411..09a08cb 100644
@@ -821,15 +821,18 @@ ssize_t __modver_version_show(struct module_attribute *mattr,
 	return sprintf(buf, "%s\n", vattr->version);
 }
 
-extern struct module_version_attribute __start___modver[], __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 *vattr;
+	const struct module_version_attribute **p;
 	struct module_kobject *mk;
 	int err;
 
-	for (vattr = __start___modver; vattr < __stop___modver; vattr++) {
+	for (p = __start___modver; p < __stop___modver; p++) {
+		const struct module_version_attribute *vattr = *p;
+
 		mk = locate_module_kobject(vattr->module_name);
 		if (mk) {
 			err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

The only sane thing I can see is make sure that such structures that we put into sections "arrays" like that are naturally aligned with padding.

Quoted 4 lines · Benjamin Herrenschmidt

The only sane thing I can see is make sure that such structures that we put into sections "arrays" like that are naturally aligned with padding.

The sad part is, that assuming I read the gcc sources correctly (see the earlier emails where David pointed to it), that alignment is:
- architecture-specific
- depends on the size of the structure
- seems to depend on the version of gcc itself.

The _one_ safe case is likely to be "structure size is a power of two". And it does look like using a pointer is going to be safe, not only because the gcc auto-alignment only triggers for things like structs/unions/arrays, but because at least the x86 code only does it if the structure was bigger than the alignment size itself.

So using pointer indirection is likely to be safe. It's still ugly and annoying as heck, though.

Quoted 19 lines · Linus Torvalds

Quoted 4 lines · Benjamin Herrenschmidt

The only sane thing I can see is make sure that such structures that we put into sections "arrays" like that are naturally aligned with padding.

The sad part is, that assuming I read the gcc sources correctly (see the earlier emails where David pointed to it), that alignment is:
- architecture-specific
- depends on the size of the structure
- seems to depend on the version of gcc itself.

The _one_ safe case is likely to be "structure size is a power of two". And it does look like using a pointer is going to be safe, not only because the gcc auto-alignment only triggers for things like structs/unions/arrays, but because at least the x86 code only does it if the structure was bigger than the alignment size itself.

Quoted 3 lines

So using pointer indirection is likely to be safe. It's still ugly and annoying as heck, though.

Regardless the approach we'll take I think the following patch is also needed (for cris architecture). I am not sure why __param section is inly defined for one specific subarch

but I they need __param they'll
need __modev as well.

Thanks,

Dmitry

Quoted 1 line

From a567280f900c15891a55e7ea4e2919b38e1d1a01 Mon Sep 17 00:00:00 2001

From: Dmitry Torokhov <[email protected]>
Date: Thu, 17 Feb 2011 13:12:26 -0800
Subject: [PATCH] cris: add missing __modver section

Commit e94965ed5beb23c6fabf7ed31f625e66d7ff28de added a new __modver section to store module version information for drivers built into the kernel, but missed the fact that cris does some additional steps to set up sections.

Signed-off-by Dmitry Torokhov <[email protected]>

---
 arch/cris/kernel/vmlinux.lds.S |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

arch/cris/kernel/vmlinux.lds.S +4 -0

index 4422189..fae1b7b 100644
@@ -73,6 +73,10 @@ SECTIONS
 	.init.data : { INIT_DATA }
 	.init.setup : { INIT_SETUP(16) }
 #ifdef CONFIG_ETRAX_ARCH_V32
+	__start___modver = .;
+	__modver : { *(__modver) }
+	__stop___modver = .;
+
 	__start___param = .;
 	__param : { *(__param) }
 	__stop___param = .;

Signature · 1 line

1.7.3.2

That's probably just a legacy from when I combined the linkscripts for the two architectures. If I remember correctly, RODATA brings in RO_DATA_SECTION which in turn brings in __param and __modver for both architectures. CRISv32 then duplicates the __param stuff for some historical reason.

True, at least until I've made sure that there isn't any underlying reason for CRISv32 to put __param in a different place...

Acked-by Jesper Nilsson <[email protected]>

Quoted 4 lines · Jesper Nilsson

True, at least until I've made sure that there isn't any underlying reason for CRISv32 to put __param in a different place...

Acked-by Jesper Nilsson <[email protected]>

I really am not going to apply a patch that likely just adds another redundant field to the linker scripts. So the ack I'm just ignoring,

But it would be good to get the "remove the legacy __param thing because it's already there from the asm-generic version" confirmed by testing, and not just looking at the source code.

From f8c17a94940a012f237986b4f40ca557a88f9019 Mon Sep 17 00:00:00 2001

Linus