Re: [PATCH 1/3] module: deal with alignment issues in built-in module versions

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

On Tue, Feb 8, 2011 at 22:12, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> On Tue, Feb 8, 2011 at 01:02, Dmitry Torokhov <dtor@xxxxxxxxxx> 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 <geert@xxxxxxxxxxxxxx>
>> Signed-off-by: Dmitry Torokhov <dtor@xxxxxxxxxx>
>
> Thanks!
>
> Tested-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>

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 -- geert@xxxxxxxxxxxxxx

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
ÿô.nlj·Ÿ®‰­†+%ŠË±é¥Šwÿº{.nlj·¥Š{±ý¶›¡Ü}©ž²ÆzÚj:+v‰¨þø®w¥þŠàÞ¨è&¢)ß«a¶Úÿûz¹ÞúŽŠÝjÿŠwèf