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(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 9bdf27c..9b7081a 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -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
diff --git a/kernel/params.c b/kernel/params.c
index 0da1411..09a08cb 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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
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.
Signed-off-by: Dmitry Torokhov <[email protected]>
---
include/linux/moduleparam.h | 14 ++++++++------
kernel/params.c | 10 ++++++----
2 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 07b4195..05b92c0 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -147,15 +147,17 @@ struct kparam_array
BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2)) \
+ BUILD_BUG_ON_ZERO(sizeof(""prefix) > MAX_PARAM_PREFIX_LEN); \
static const char __param_str_##name[] = prefix #name; \
- static struct kernel_param __moduleparam_const __param_##name \
- __used \
- __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
- = { __param_str_##name, ops, perm, isbool ? KPARAM_ISBOOL : 0, \
- { arg } }
+ static const struct kernel_param ___param_##name = { \
+ __param_str_##name, ops, perm, isbool ? KPARAM_ISBOOL : 0, \
+ { arg } \
+ }; \
+ static const struct kernel_param \
+ __used __attribute__ ((__section__ ("__param"))) \
+ * __moduleparam_const __param_##name = &___param_##name
/* Obsolete - use module_param_cb() */
#define module_param_call(name, set, get, arg, perm) \
- static struct kernel_param_ops __param_ops_##name = \
+ static const struct kernel_param_ops __param_ops_##name = \
{ (void *)set, (void *)get }; \
__module_param_call(MODULE_PARAM_PREFIX, \
name, &__param_ops_##name, arg, \
diff --git a/kernel/params.c b/kernel/params.c
index 09a08cb..66f7e66 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -498,7 +498,8 @@ EXPORT_SYMBOL(param_ops_string);
#define to_module_attr(n) container_of(n, struct module_attribute, attr)
#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
-extern struct kernel_param __start___param[], __stop___param[];
+extern const struct kernel_param *__start___param[];
+extern const struct kernel_param *__stop___param[];
struct param_attribute
{
@@ -754,7 +755,7 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
}
static void __init kernel_add_sysfs_param(const char *name,
- struct kernel_param *kparam,
+ const struct kernel_param *kparam,
unsigned int name_skip)
{
struct module_kobject *mk;
@@ -789,11 +790,12 @@ static void __init kernel_add_sysfs_param(const char *name,
*/
static void __init param_sysfs_builtin(void)
{
- struct kernel_param *kp;
+ const struct kernel_param **p;
unsigned int name_len;
char modname[MODULE_NAME_LEN];
- for (kp = __start___param; kp < __stop___param; kp++) {
+ for (p = __start___param; p < __stop___param; p++) {
+ const struct kernel_param *kp = *p;
char *dot;
if (kp->perm == 0)
--
1.7.3.2
Doing so prevents the following warning from sparse:
CHECK kernel/params.c
kernel/params.c:817:9: warning: symbol '__modver_version_show' was not
declared. Should it be static?
since kernel/params.c is never compiled with MODULE being set.
Signed-off-by: Dmitry Torokhov <[email protected]>
---
include/linux/module.h | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 9b7081a..cb41837 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -64,6 +64,9 @@ struct module_version_attribute {
const char *version;
};
+extern ssize_t __modver_version_show(struct module_attribute *,
+ struct module *, char *);
+
struct module_kobject
{
struct kobject kobj;
@@ -172,8 +175,6 @@ extern struct module __this_module;
#define MODULE_VERSION(_version) MODULE_INFO(version, _version)
#else
#define MODULE_VERSION(_version) \
- extern ssize_t __modver_version_show(struct module_attribute *, \
- struct module *, char *); \
static struct module_version_attribute ___modver_attr = { \
.mattr = { \
.attr = { \
--
1.7.3.2
On Tue, Feb 8, 2011 at 01:02, Dmitry Torokhov <[email protected]> wrote:
> 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]>
Thanks!
Tested-by: Geert Uytterhoeven <[email protected]>
> ---
> include/linux/module.h | 10 +++++-----
> kernel/params.c | 9 ++++++---
> 2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 9bdf27c..9b7081a 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -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
> diff --git a/kernel/params.c b/kernel/params.c
> index 0da1411..09a08cb 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -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
>
--
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
>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.
Signed-off-by: Dmitry Torokhov <[email protected]>
---
This is v2 of the patch, hopefully fixing all the issues that were
presented in the original submission, namely:
- fix for kparam_[un]block_sysfs_write() breakage;
- convert non-built-in module parameter case to expect array of
pointers to struct kernel_param, not array of the structures
themselves.
Thanks,
Dmitry
include/linux/module.h | 2 +-
include/linux/moduleparam.h | 24 ++++++++++++----------
init/main.c | 2 +-
kernel/module.c | 2 +-
kernel/params.c | 45 +++++++++++++++++++++++-------------------
5 files changed, 41 insertions(+), 34 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index cb41837..c256380 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -292,7 +292,7 @@ struct module
unsigned int num_syms;
/* Kernel parameters. */
- struct kernel_param *kp;
+ const struct kernel_param **kp;
unsigned int num_kp;
/* GPL-only exported symbols. */
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 07b4195..f9de240 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -147,15 +147,17 @@ struct kparam_array
BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2)) \
+ BUILD_BUG_ON_ZERO(sizeof(""prefix) > MAX_PARAM_PREFIX_LEN); \
static const char __param_str_##name[] = prefix #name; \
- static struct kernel_param __moduleparam_const __param_##name \
- __used \
- __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
- = { __param_str_##name, ops, perm, isbool ? KPARAM_ISBOOL : 0, \
- { arg } }
+ static const struct kernel_param __param_##name = { \
+ __param_str_##name, ops, perm, isbool ? KPARAM_ISBOOL : 0, \
+ { arg } \
+ }; \
+ static const struct kernel_param \
+ __used __attribute__ ((__section__ ("__param"))) \
+ * __moduleparam_const ___param_##name = &__param_##name
/* Obsolete - use module_param_cb() */
#define module_param_call(name, set, get, arg, perm) \
- static struct kernel_param_ops __param_ops_##name = \
+ static const struct kernel_param_ops __param_ops_##name = \
{ (void *)set, (void *)get }; \
__module_param_call(MODULE_PARAM_PREFIX, \
name, &__param_ops_##name, arg, \
@@ -265,15 +267,15 @@ static inline void __kernel_param_unlock(void)
/* Called on module insert or kernel boot */
extern int parse_args(const char *name,
char *args,
- const struct kernel_param *params,
+ const struct kernel_param **params,
unsigned num,
int (*unknown)(char *param, char *val));
/* Called by module remove. */
#ifdef CONFIG_SYSFS
-extern void destroy_params(const struct kernel_param *params, unsigned num);
+extern void destroy_params(const struct kernel_param **params, unsigned num);
#else
-static inline void destroy_params(const struct kernel_param *params,
+static inline void destroy_params(const struct kernel_param **params,
unsigned num)
{
}
@@ -391,13 +393,13 @@ struct module;
#if defined(CONFIG_SYSFS) && defined(CONFIG_MODULES)
extern int module_param_sysfs_setup(struct module *mod,
- const struct kernel_param *kparam,
+ const struct kernel_param **kparam,
unsigned int num_params);
extern void module_param_sysfs_remove(struct module *mod);
#else
static inline int module_param_sysfs_setup(struct module *mod,
- const struct kernel_param *kparam,
+ const struct kernel_param **kparam,
unsigned int num_params)
{
return 0;
diff --git a/init/main.c b/init/main.c
index 33c37c3..3601586 100644
--- a/init/main.c
+++ b/init/main.c
@@ -544,7 +544,7 @@ static void __init mm_init(void)
asmlinkage void __init start_kernel(void)
{
char * command_line;
- extern const struct kernel_param __start___param[], __stop___param[];
+ extern const struct kernel_param *__start___param[], *__stop___param[];
smp_setup_processor_id();
diff --git a/kernel/module.c b/kernel/module.c
index efa290e..7c6c3bf 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1470,7 +1470,7 @@ out:
static int mod_sysfs_setup(struct module *mod,
const struct load_info *info,
- struct kernel_param *kparam,
+ const struct kernel_param **kparam,
unsigned int num_params)
{
int err;
diff --git a/kernel/params.c b/kernel/params.c
index 09a08cb..4f2eb43 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -83,9 +83,9 @@ static inline int parameq(const char *input, const char *paramname)
return 0;
}
-static int parse_one(char *param,
+static int parse_one(char *name,
char *val,
- const struct kernel_param *params,
+ const struct kernel_param **params,
unsigned num_params,
int (*handle_unknown)(char *param, char *val))
{
@@ -94,14 +94,16 @@ static int parse_one(char *param,
/* Find parameter */
for (i = 0; i < num_params; i++) {
- if (parameq(param, params[i].name)) {
+ const struct kernel_param *param = params[i];
+
+ if (parameq(name, param->name)) {
/* Noone handled NULL, so do it here. */
- if (!val && params[i].ops->set != param_set_bool)
+ if (!val && param->ops->set != param_set_bool)
return -EINVAL;
DEBUGP("They are equal! Calling %p\n",
- params[i].ops->set);
+ param->ops->set);
mutex_lock(¶m_lock);
- err = params[i].ops->set(val, ¶ms[i]);
+ err = param->ops->set(val, param);
mutex_unlock(¶m_lock);
return err;
}
@@ -109,7 +111,7 @@ static int parse_one(char *param,
if (handle_unknown) {
DEBUGP("Unknown argument: calling %p\n", handle_unknown);
- return handle_unknown(param, val);
+ return handle_unknown(name, val);
}
DEBUGP("Unknown argument `%s'\n", param);
@@ -171,7 +173,7 @@ static char *next_arg(char *args, char **param, char **val)
/* Args looks like "foo=bar,bar2 baz=fuz wiz". */
int parse_args(const char *name,
char *args,
- const struct kernel_param *params,
+ const struct kernel_param **params,
unsigned num,
int (*unknown)(char *param, char *val))
{
@@ -190,8 +192,9 @@ int parse_args(const char *name,
irq_was_disabled = irqs_disabled();
ret = parse_one(param, val, params, num, unknown);
if (irq_was_disabled && !irqs_disabled()) {
- printk(KERN_WARNING "parse_args(): option '%s' enabled "
- "irq's!\n", param);
+ printk(KERN_WARNING
+ "parse_args(): option '%s' enabled irq's!\n",
+ param);
}
switch (ret) {
case -ENOENT:
@@ -498,7 +501,8 @@ EXPORT_SYMBOL(param_ops_string);
#define to_module_attr(n) container_of(n, struct module_attribute, attr)
#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
-extern struct kernel_param __start___param[], __stop___param[];
+extern const struct kernel_param *__start___param[];
+extern const struct kernel_param *__stop___param[];
struct param_attribute
{
@@ -667,16 +671,16 @@ static void free_module_param_attrs(struct module_kobject *mk)
* /sys/module/[mod->name]/parameters/
*/
int module_param_sysfs_setup(struct module *mod,
- const struct kernel_param *kparam,
+ const struct kernel_param **kparam,
unsigned int num_params)
{
int i, err;
bool params = false;
for (i = 0; i < num_params; i++) {
- if (kparam[i].perm == 0)
+ if (kparam[i]->perm == 0)
continue;
- err = add_sysfs_param(&mod->mkobj, &kparam[i], kparam[i].name);
+ err = add_sysfs_param(&mod->mkobj, kparam[i], kparam[i]->name);
if (err)
return err;
params = true;
@@ -710,13 +714,13 @@ void module_param_sysfs_remove(struct module *mod)
}
#endif
-void destroy_params(const struct kernel_param *params, unsigned num)
+void destroy_params(const struct kernel_param **params, unsigned num)
{
unsigned int i;
for (i = 0; i < num; i++)
- if (params[i].ops->free)
- params[i].ops->free(params[i].arg);
+ if (params[i]->ops->free)
+ params[i]->ops->free(params[i]->arg);
}
static struct module_kobject * __init locate_module_kobject(const char *name)
@@ -754,7 +758,7 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
}
static void __init kernel_add_sysfs_param(const char *name,
- struct kernel_param *kparam,
+ const struct kernel_param *kparam,
unsigned int name_skip)
{
struct module_kobject *mk;
@@ -789,11 +793,12 @@ static void __init kernel_add_sysfs_param(const char *name,
*/
static void __init param_sysfs_builtin(void)
{
- struct kernel_param *kp;
+ const struct kernel_param **p;
unsigned int name_len;
char modname[MODULE_NAME_LEN];
- for (kp = __start___param; kp < __stop___param; kp++) {
+ for (p = __start___param; p < __stop___param; p++) {
+ const struct kernel_param *kp = *p;
char *dot;
if (kp->perm == 0)
--
1.7.3.2
On Sat, 12 Feb 2011 08:33:56 am Dmitry Torokhov wrote:
> >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.
I've applied this, but won't put it to linux-next. I'll let it stew for
a bit longer unless someone actually reports a problem with the current
code.
Thanks,
Rusty.
On Tue, Feb 8, 2011 at 22:12, Geert Uytterhoeven <[email protected]> wrote:
> On Tue, Feb 8, 2011 at 01:02, Dmitry Torokhov <[email protected]> wrote:
>> 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]>
>
> Thanks!
>
> Tested-by: Geert Uytterhoeven <[email protected]>
Can we please get this in? 2.6.38-rc5 still crashes on m68k.
Thx!
>> ---
>> include/linux/module.h | 10 +++++-----
>> kernel/params.c | 9 ++++++---
>> 2 files changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 9bdf27c..9b7081a 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -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
>> diff --git a/kernel/params.c b/kernel/params.c
>> index 0da1411..09a08cb 100644
>> --- a/kernel/params.c
>> +++ b/kernel/params.c
>> @@ -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
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Mon, Feb 7, 2011 at 4:02 PM, Dmitry Torokhov <[email protected]> wrote:
>
> 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?
Linus
On Thu, Feb 17, 2011 at 09:24:58AM -0800, Linus Torvalds wrote:
> On Mon, Feb 7, 2011 at 4:02 PM, Dmitry Torokhov <[email protected]> wrote:
> >
> > 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.
Thanks.
--
Dmitry
On Thu, Feb 17, 2011 at 9:31 AM, Dmitry Torokhov <[email protected]> wrote:
>
> 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)?
Linus
On Thu, Feb 17, 2011 at 09:45:37AM -0800, Linus Torvalds wrote:
> On Thu, Feb 17, 2011 at 9:31 AM, Dmitry Torokhov <[email protected]> wrote:
> >
> > 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.
Thanks.
--
Dmitry
On Thu, Feb 17, 2011 at 10:00 AM, Dmitry Torokhov <[email protected]> wrote:
>
> 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.
So mark the struct properly.
> 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.
That sounds totally bogus. Minimum alignment is all it needs, and we
do that in other places too. I really don't see the reason to add some
broken indirection for totally broken reasons. We don't do that with
anything else.
David, why are you saying that regular "just mark the structure
alignment correctly" doesn't work?
Linus
From: Linus Torvalds <[email protected]>
Date: Thu, 17 Feb 2011 10:06:40 -0800
> David, why are you saying that regular "just mark the structure
> alignment correctly" doesn't work?
Because it's been proven to not work:
http://marc.info/?l=linux-kernel&m=129674396021733&w=2
http://marc.info/?l=linux-kernel&m=129674399621795&w=2
http://marc.info/?l=linux-kernel&m=129674396121739&w=2
http://marc.info/?l=linux-kernel&m=129674396021735&w=2
GCC is very clever with "static" objects these days.
It thinks that, because you mark something "static", it can align it
any way it wants because it controls the domain in which the object
exists. It "knows" that the object can't be part of an array, and
therefore no external entity can be concerned about the true "size" of
the object (specifically wrt. side effects of the alignment of the
object).
In these cases it takes the explicit alignment attribute as a minimum,
not as an absolute requirement.
But we lie to the compiler, we mark things static then put them into a
special a special section, then try to iterate over those objects
globally as an array and expect the compiler to lay them all out
with identical alignments and sizes everywhere.
And this doesn't work.
From: Linus Torvalds <[email protected]>
Date: Thu, 17 Feb 2011 13:11:56 -0800
> 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.
On Thu, Feb 17, 2011 at 1:01 PM, David Miller <[email protected]> wrote:
>
> GCC is very clever with "static" objects these days.
I'm sorry, but that simply isn't an acceptable excuse.
The thing is, we DO THIS THING ALL OVER. And the trick used in the
original thing (to just turn it into a pointer array instead) _also_
does it. It just happens to do it with a pointer instead of a struct.
So no, I will not take this piece-of-crap patch based on "gcc is
trying to be clever, but we can work around it by doing the same thing
except now we take the drugs".
That's just stupid.
And I'm scared to see that you have apparently fed those drugs to the
perf tree too.
EVERY SINGLE OF YOUR ARGUMENTS WORK FOR "pointer" TOO! It's pure
happenstance that gcc doesn't decide to do clever things with them. So
replacing that array of structs with array-of-pointers is just voodoo
programming.
What is the crap-for-brains thing that gcc actually does, and how can
we fix it _properly_ without this kind of voodoo?
Linus
On Thu, Feb 17, 2011 at 1:17 PM, David Miller <[email protected]> wrote:
>> 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.
So? Do they actually promise to never do it for pointers? What makes
aggregates so special? Rather than just happenstance?
I wouldn't be surprised if the structure alignment isn't even about
something even subtler and totally random, like size of structure (ie
"don't align small structures")
> 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.
.. and maybe that's the right thing to do here too.
> The GCC manual even documents the alignment attribute behavior.
The gcc manual is worthless. Every time some gcc person wants to
change behavior, they just change the manual. So you can't rely on it
anyway. We've seen that with inline asm before etc.
> 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.
I'm shooting not the messenger, but the people who make these kinds of
really ugly changes with bad changelogs that don't even explain what
the actual problem is. And apparently any gcc person is free to break
it in the future _anyway_.
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.
Linus
From: Linus Torvalds <[email protected]>
Date: Thu, 17 Feb 2011 13:54:57 -0800
> 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.
> 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.
On Thu, Feb 17, 2011 at 02:01:19PM -0800, David Miller wrote:
> From: Linus Torvalds <[email protected]>
> Date: Thu, 17 Feb 2011 13:54:57 -0800
>
> > 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.
>
> > 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?
--
Dmitry
From: Dmitry Torokhov <[email protected]>
Date: Thu, 17 Feb 2011 14:19:57 -0800
> Any pointers as to how to emit these pointers with asm?
.section FOO_SECTION, "a"
.align SIZEOF_POINTER
.{,x}word POINTER
.previous
Where FOO_SECTION is your special section name, SIZEOF_POINTER is
sizeof(void *), and POINTER is the pointer you want to add to the
section.
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.
On Thu, Feb 17, 2011 at 2:23 PM, David Miller <[email protected]> wrote:
>
> 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.
I really detest gcc sometimes. All these "clever" things that just
make things harder to do. If the user explicitly tells it the section
and the alignment, it should damn well not think that it knows better
and change it. Damn.
Maybe we have to take the "just confuse gcc enough and pray" approach
on those pointers after all.
Linus
On Thu, Feb 17, 2011 at 2:48 PM, Linus Torvalds
<[email protected]> wrote:
>
> 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.
Grr.
Where in gcc is the alignment expansion logic? I'd like to see what
the rules are, just for my own perverse satisfaction.
Linus
From: Linus Torvalds <[email protected]>
Date: Thu, 17 Feb 2011 15:08:52 -0800
> Where in gcc is the alignment expansion logic? I'd like to see what
> the rules are, just for my own perverse satisfaction.
The magic that overrides the alignment various cases is in the x86
specific code:
gcc/config/i386/i386.c
ix86_data_alignment() and ix86_local_alignment()
On Thu, 2011-02-17 at 15:08 -0800, Linus Torvalds wrote:
> On Thu, Feb 17, 2011 at 2:48 PM, Linus Torvalds
> <[email protected]> wrote:
> >
> > 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.
Cheers,
Ben.
> 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(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 9bdf27c..9b7081a 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -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
diff --git a/kernel/params.c b/kernel/params.c
index 0da1411..09a08cb 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -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);
On Mon, Feb 21, 2011 at 05:00, Rusty Russell <[email protected]> wrote:
>> 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.
> 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(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 9bdf27c..9b7081a 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -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
> diff --git a/kernel/params.c b/kernel/params.c
> index 0da1411..09a08cb 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -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);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Mon, Feb 21, 2011 at 08:38:46AM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 21, 2011 at 05:00, Rusty Russell <[email protected]> wrote:
> >> 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.
>
How about this one then?
Thanks.
--
Dmitry
>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(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index e7c6385..de5cd21 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -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
On Mon, Feb 21, 2011 at 08:49, Dmitry Torokhov <[email protected]> wrote:
> On Mon, Feb 21, 2011 at 08:38:46AM +0100, Geert Uytterhoeven wrote:
>> On Mon, Feb 21, 2011 at 05:00, Rusty Russell <[email protected]> wrote:
>> >> 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.
>>
>
> How about this one then?
Works.
> 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]>
Tested-by: Geert Uytterhoeven <[email protected]>
> ---
> include/linux/module.h | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index e7c6385..de5cd21 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -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
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Mon, 2011-02-21 at 14:30 +1030, Rusty Russell wrote:
> > 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?
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.
Cheers,
Ben.
> 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(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 9bdf27c..9b7081a 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -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
> diff --git a/kernel/params.c b/kernel/params.c
> index 0da1411..09a08cb 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -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/
On Mon, Feb 21, 2011 at 5:58 PM, Benjamin Herrenschmidt
<[email protected]> wrote:
>
> 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.
Linus
On Mon, Feb 21, 2011 at 06:03:16PM -0800, Linus Torvalds wrote:
> On Mon, Feb 21, 2011 at 5:58 PM, Benjamin Herrenschmidt
> <[email protected]> wrote:
> >
> > 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.
>
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
>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(-)
diff --git a/arch/cris/kernel/vmlinux.lds.S b/arch/cris/kernel/vmlinux.lds.S
index 4422189..fae1b7b 100644
--- a/arch/cris/kernel/vmlinux.lds.S
+++ b/arch/cris/kernel/vmlinux.lds.S
@@ -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 = .;
--
1.7.3.2
On Tue, Feb 22, 2011 at 08:02:40AM +0100, Dmitry Torokhov wrote:
> On Mon, Feb 21, 2011 at 06:03:16PM -0800, Linus Torvalds wrote:
> > On Mon, Feb 21, 2011 at 5:58 PM, Benjamin Herrenschmidt
> > <[email protected]> wrote:
> > >
> > > 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.
> >
>
> 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
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.
> but I they need __param they'll
> need __modev as well.
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]>
> Thanks,
>
> Dmitry
/Jesper
> >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(-)
>
> diff --git a/arch/cris/kernel/vmlinux.lds.S b/arch/cris/kernel/vmlinux.lds.S
> index 4422189..fae1b7b 100644
> --- a/arch/cris/kernel/vmlinux.lds.S
> +++ b/arch/cris/kernel/vmlinux.lds.S
> @@ -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 = .;
> --
> 1.7.3.2
/^JN - Jesper Nilsson
--
Jesper Nilsson -- [email protected]
On Tue, Feb 22, 2011 at 9:08 AM, Jesper Nilsson <[email protected]> wrote:
>
> 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.
Hmm. This makes it sound like the patch should be to remove the
__param thing rather than to add the _modver thing. And I can
definitely confirm that cris seems to use RODATA, which brings in the
RO_DATA_SECTION() thing with proper page alignment.
But:
> 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.
Linus
On Tue, Feb 22, 2011 at 09:47:09PM +0100, Linus Torvalds wrote:
> On Tue, Feb 22, 2011 at 9:08 AM, Jesper Nilsson <[email protected]> wrote:
> > 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.
Testing shows no problems with the below patch.
>From f8c17a94940a012f237986b4f40ca557a88f9019 Mon Sep 17 00:00:00 2001
From: Jesper Nilsson <[email protected]>
Date: Wed, 23 Feb 2011 13:04:25 +0100
Subject: [PATCH] Drop redundant __param section for CRISv32.
The __param section is already brought in by RODATA above.
Signed-off-by: Jesper Nilsson <[email protected]>
---
arch/cris/kernel/vmlinux.lds.S | 5 -----
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/arch/cris/kernel/vmlinux.lds.S b/arch/cris/kernel/vmlinux.lds.S
index 917b727..e8adc35 100644
--- a/arch/cris/kernel/vmlinux.lds.S
+++ b/arch/cris/kernel/vmlinux.lds.S
@@ -75,11 +75,6 @@ SECTIONS
INIT_TEXT_SECTION(PAGE_SIZE)
.init.data : { INIT_DATA }
.init.setup : { INIT_SETUP(16) }
-#ifdef CONFIG_ETRAX_ARCH_V32
- __start___param = .;
- __param : { *(__param) }
- __stop___param = .;
-#endif
.initcall.init : {
INIT_CALLS
}
--
1.7.4
> Linus
/^JN - Jesper Nilsson
--
Jesper Nilsson -- [email protected]