Early crash (was: Re: module: show version information for built-in modules in sysfs)

Early crash (was: Re: module: show version information for built-in modules in sysfs)

[Geert Uytterhoeven]

On Mon, Jan 24, 2011 at 11:59, Linux Kernel Mailing List
<linux-kernel@vger.kernel.org> wrote:
> Gitweb:     http://git.kernel.org/linus/e94965ed5beb23c6fabf7ed31f625e66d7ff28de

>    module: show version information for built-in modules in sysfs
>
>    Currently only drivers that are built as modules have their versions
>    shown in /sys/module/<module_name>/version, but this information might
>    also be useful for built-in drivers as well. This especially important
>    for drivers that do not define any parameters - such drivers, if
>    built-in, are completely invisible from userspace.
>
>    This patch changes MODULE_VERSION() macro so that in case when we are
>    compiling built-in module, version information is stored in a separate
>    section. Kernel then uses this data to create 'version' sysfs attribute
>    in the same fashion it creates attributes for module parameters.

This commit causes the crash below on m68k (ARAnyM).
Reverting this commit and its dependency
3b90a5b292321b2acac3921f77046ae195aef53f
("module: fix linker error for MODULE_VERSION when !MODULE and CONFIG_SYSFS=n")
makes it boot again.

NET: Registered protocol family 16
Unable to handle kernel NULL pointer dereference at virtual address 0000002b
Oops: 00000000
Modules linked in:
PC: [<00141164>] kset_find_obj_hinted+0x5a/0xb6
SR: 2300  SP: 00c09ec8  a2: 00c07900
d0: 00000078    d1: 00c2ee90    d2: 00000000    d3: 00000000
d4: 00000000    d5: 00000000    a0: 00c2f700    a1: 00c2f701
Process swapper (pid: 1, task=00c07900)
Frame format=7 eff addr=0000002b ssw=0525 faddr=0000002b
wb 1 stat/addr/data: 0000 00000000 00000000
wb 2 stat/addr/data: 0000 00000000 00000000
wb 3 stat/addr/data: 0000 0000002b 00000000
push data: 00000000 00000000 00000000 00000000
Stack from 00c09f30:
        0000002b 002d853a 00310dac 0031214a 002dbff8 001411d0 00c2ee90 0000002b
        00000000 00310dc2 00c2ee90 0000002b 00c2ee50 00000000 002d853a 00310eb8
        0000002b 00000000 00000000 003291a8 00310e6c 00c1d8b8 002e6872 00170e00
        00c1d8b0 002e6872 003031f8 00000000 00000000 00303038 0031bec2 0031214a
        0031bf40 00303038 00000000 003291a4 000020f8 00000000 00000000 00000000
        00000000 00000000 00000000 003291a8 00002008 0031214a 0030baee 00310e6c
Call Trace: [<00310dac>] locate_module_kobject+0x0/0xc0
 [<0031214a>] __alloc_bootmem+0x0/0x1a
 [<001411d0>] kset_find_obj+0x10/0x18
 [<00310dc2>] locate_module_kobject+0x16/0xc0
 [<00310eb8>] param_sysfs_init+0x4c/0x1be
 [<00310e6c>] param_sysfs_init+0x0/0x1be
 [<00170e00>] vtconsole_init_device+0x3c/0x90
 [<0031bec2>] vtconsole_class_init+0x0/0xda
 [<0031214a>] __alloc_bootmem+0x0/0x1a
 [<0031bf40>] vtconsole_class_init+0x7e/0xda
 [<000020f8>] do_one_initcall+0xf0/0x186
 [<00002008>] do_one_initcall+0x0/0x186
 [<0031214a>] __alloc_bootmem+0x0/0x1a
 [<0030baee>] kernel_init+0x96/0x136
 [<00310e6c>] param_sysfs_init+0x0/0x1be
 [<0030ba58>] kernel_init+0x0/0x136
 [<0002794e>] printk+0x0/0x1a
 [<00002b52>] kernel_thread+0x3a/0x4e

Code: b288 672c 2053 4a88 6716 2248 2c4c 1019 <b01e> 6606 4a00 66f6
6002 9026 4a00 6722 47ed fffc 2a6b 0004 41eb 0004 b288 66d4
Disabling lock debugging due to kernel taint
Kernel panic - not syncing: Attempted to kill init!
Call Trace: [<0002745c>] panic+0x5a/0x1ba
 [<00028bd4>] exit_mm+0x0/0x102
 [<0011dfd6>] exit_sem+0x0/0x162
 [<0002f5f2>] exit_ptrace+0x0/0x106
 [<0002a274>] do_exit+0x6cc/0x6ce
 [<0002794e>] printk+0x0/0x1a
 [<000031de>] die_if_kernel+0x4e/0x52
 [<00006e8c>] send_fault_sig+0xd4/0x11e
 [<00006f80>] do_page_fault+0xaa/0x1c2
 [<000038b6>] buserr_c+0x192/0x6d8
 [<00100100>] mnt_xdr_dec_mountres+0xa2/0xb4
 [<00002542>] buserr+0x1e/0x24
 [<00310dac>] locate_module_kobject+0x0/0xc0
 [<0031214a>] __alloc_bootmem+0x0/0x1a
 [<001411d0>] kset_find_obj+0x10/0x18
 [<00310dc2>] locate_module_kobject+0x16/0xc0
 [<00310eb8>] param_sysfs_init+0x4c/0x1be
 [<00310e6c>] param_sysfs_init+0x0/0x1be
 [<00170e00>] vtconsole_init_device+0x3c/0x90
 [<0031bec2>] vtconsole_class_init+0x0/0xda
 [<0031214a>] __alloc_bootmem+0x0/0x1a
 [<0031bf40>] vtconsole_class_init+0x7e/0xda
 [<000020f8>] do_one_initcall+0xf0/0x186
 [<00002008>] do_one_initcall+0x0/0x186
 [<0031214a>] __alloc_bootmem+0x0/0x1a
 [<0030baee>] kernel_init+0x96/0x136
 [<00310e6c>] param_sysfs_init+0x0/0x1be
 [<0030ba58>] kernel_init+0x0/0x136
 [<0002794e>] printk+0x0/0x1a
 [<00002b52>] kernel_thread+0x3a/0x4e

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: Early crash (was: Re: module: show version information for built-in modules in sysfs)

[Dmitry Torokhov]

On Tue, Feb 01, 2011 at 12:33:29PM -0800, Geert Uytterhoeven wrote:
> On Mon, Jan 24, 2011 at 11:59, Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org> wrote:
> > Gitweb:     http://git.kernel.org/linus/e94965ed5beb23c6fabf7ed31f625e66d7ff28de
> 
> >    module: show version information for built-in modules in sysfs
> >
> >    Currently only drivers that are built as modules have their versions
> >    shown in /sys/module/<module_name>/version, but this information might
> >    also be useful for built-in drivers as well. This especially important
> >    for drivers that do not define any parameters - such drivers, if
> >    built-in, are completely invisible from userspace.
> >
> >    This patch changes MODULE_VERSION() macro so that in case when we are
> >    compiling built-in module, version information is stored in a separate
> >    section. Kernel then uses this data to create 'version' sysfs attribute
> >    in the same fashion it creates attributes for module parameters.
> 
> This commit causes the crash below on m68k (ARAnyM).
> Reverting this commit and its dependency
> 3b90a5b292321b2acac3921f77046ae195aef53f
> ("module: fix linker error for MODULE_VERSION when !MODULE and CONFIG_SYSFS=n")
> makes it boot again.
> 

Hi Geert,

Does the follwing help by any chance?

>From d6fd4a6e0fc2d3f0a74962d4a6f663a46d230ecd Mon Sep 17 00:00:00 2001
diff --git a/arch/m68knommu/kernel/vmlinux.lds.S b/arch/m68knommu/kernel/vmlinux.lds.S
index ef33213..47e15eb 100644
--- a/arch/m68knommu/kernel/vmlinux.lds.S
+++ b/arch/m68knommu/kernel/vmlinux.lds.S
@@ -141,6 +141,12 @@ SECTIONS {
 		*(__param)
 		__stop___param = .;
 
+		/* Built-in module versions */
+		. = ALIGN(4) ;
+		__start___modver = .;
+		*(__modver)
+		__stop___modver = .;
+
 		. = ALIGN(4) ;
 		_etext = . ;
 	} > TEXT

Thanks,

Dmitry

Re: Early crash (was: Re: module: show version information for built-in modules in sysfs)

[Geert Uytterhoeven]

On Tue, Feb 1, 2011 at 22:09, Dmitry Torokhov <dtor@vmware.com> wrote:
> On Tue, Feb 01, 2011 at 12:33:29PM -0800, Geert Uytterhoeven wrote:
>> On Mon, Jan 24, 2011 at 11:59, Linux Kernel Mailing List
>> <linux-kernel@vger.kernel.org> wrote:
>> > Gitweb:     http://git.kernel.org/linus/e94965ed5beb23c6fabf7ed31f625e66d7ff28de
>>
>> >    module: show version information for built-in modules in sysfs
>> >
>> >    Currently only drivers that are built as modules have their versions
>> >    shown in /sys/module/<module_name>/version, but this information might
>> >    also be useful for built-in drivers as well. This especially important
>> >    for drivers that do not define any parameters - such drivers, if
>> >    built-in, are completely invisible from userspace.
>> >
>> >    This patch changes MODULE_VERSION() macro so that in case when we are
>> >    compiling built-in module, version information is stored in a separate
>> >    section. Kernel then uses this data to create 'version' sysfs attribute
>> >    in the same fashion it creates attributes for module parameters.
>>
>> This commit causes the crash below on m68k (ARAnyM).
>> Reverting this commit and its dependency
>> 3b90a5b292321b2acac3921f77046ae195aef53f
>> ("module: fix linker error for MODULE_VERSION when !MODULE and CONFIG_SYSFS=n")
>> makes it boot again.
>>
>
> Hi Geert,
>
> Does the follwing help by any chance?
>
> From d6fd4a6e0fc2d3f0a74962d4a6f663a46d230ecd Mon Sep 17 00:00:00 2001
> diff --git a/arch/m68knommu/kernel/vmlinux.lds.S b/arch/m68knommu/kernel/vmlinux.lds.S
> index ef33213..47e15eb 100644
> --- a/arch/m68knommu/kernel/vmlinux.lds.S
> +++ b/arch/m68knommu/kernel/vmlinux.lds.S

The crash happened on m68k with MMU, not m68knommu.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: Early crash (was: Re: module: show version information for built-in modules in sysfs)

[Dmitry Torokhov]

On Tue, Feb 01, 2011 at 02:03:23PM -0800, Geert Uytterhoeven wrote:
> On Tue, Feb 1, 2011 at 22:09, Dmitry Torokhov <dtor@vmware.com> wrote:
> > On Tue, Feb 01, 2011 at 12:33:29PM -0800, Geert Uytterhoeven wrote:
> >> On Mon, Jan 24, 2011 at 11:59, Linux Kernel Mailing List
> >> <linux-kernel@vger.kernel.org> wrote:
> >> > Gitweb:     http://git.kernel.org/linus/e94965ed5beb23c6fabf7ed31f625e66d7ff28de
> >>
> >> >    module: show version information for built-in modules in sysfs
> >> >
> >> >    Currently only drivers that are built as modules have their versions
> >> >    shown in /sys/module/<module_name>/version, but this information might
> >> >    also be useful for built-in drivers as well. This especially important
> >> >    for drivers that do not define any parameters - such drivers, if
> >> >    built-in, are completely invisible from userspace.
> >> >
> >> >    This patch changes MODULE_VERSION() macro so that in case when we are
> >> >    compiling built-in module, version information is stored in a separate
> >> >    section. Kernel then uses this data to create 'version' sysfs attribute
> >> >    in the same fashion it creates attributes for module parameters.
> >>
> >> This commit causes the crash below on m68k (ARAnyM).
> >> Reverting this commit and its dependency
> >> 3b90a5b292321b2acac3921f77046ae195aef53f
> >> ("module: fix linker error for MODULE_VERSION when !MODULE and CONFIG_SYSFS=n")
> >> makes it boot again.
> >>
> >
> > Hi Geert,
> >
> > Does the follwing help by any chance?
> >
> > From d6fd4a6e0fc2d3f0a74962d4a6f663a46d230ecd Mon Sep 17 00:00:00 2001
> > diff --git a/arch/m68knommu/kernel/vmlinux.lds.S b/arch/m68knommu/kernel/vmlinux.lds.S
> > index ef33213..47e15eb 100644
> > --- a/arch/m68knommu/kernel/vmlinux.lds.S
> > +++ b/arch/m68knommu/kernel/vmlinux.lds.S
> 
> The crash happened on m68k with MMU, not m68knommu.
> 

Hmm, OK then. Could you please see if the crash happens if you return
early in kernel/params.c::version_sysfs_builtin() ? Also, do you see
anything in __modev section of your build?

Thanks,
Dmitry

Re: Early crash (was: Re: module: show version information for built-in modules in sysfs)

[Geert Uytterhoeven]

On Tue, Feb 1, 2011 at 23:26, Dmitry Torokhov <dtor@vmware.com> wrote:
> On Tue, Feb 01, 2011 at 02:03:23PM -0800, Geert Uytterhoeven wrote:
>> On Tue, Feb 1, 2011 at 22:09, Dmitry Torokhov <dtor@vmware.com> wrote:
>> > On Tue, Feb 01, 2011 at 12:33:29PM -0800, Geert Uytterhoeven wrote:
>> >> On Mon, Jan 24, 2011 at 11:59, Linux Kernel Mailing List
>> >> <linux-kernel@vger.kernel.org> wrote:
>> >> > Gitweb:     http://git.kernel.org/linus/e94965ed5beb23c6fabf7ed31f625e66d7ff28de
>> >>
>> >> >    module: show version information for built-in modules in sysfs
>> >> >
>> >> >    Currently only drivers that are built as modules have their versions
>> >> >    shown in /sys/module/<module_name>/version, but this information might
>> >> >    also be useful for built-in drivers as well. This especially important
>> >> >    for drivers that do not define any parameters - such drivers, if
>> >> >    built-in, are completely invisible from userspace.
>> >> >
>> >> >    This patch changes MODULE_VERSION() macro so that in case when we are
>> >> >    compiling built-in module, version information is stored in a separate
>> >> >    section. Kernel then uses this data to create 'version' sysfs attribute
>> >> >    in the same fashion it creates attributes for module parameters.
>> >>
>> >> This commit causes the crash below on m68k (ARAnyM).
>> >> Reverting this commit and its dependency
>> >> 3b90a5b292321b2acac3921f77046ae195aef53f
>> >> ("module: fix linker error for MODULE_VERSION when !MODULE and CONFIG_SYSFS=n")
>> >> makes it boot again.
>> >>
>> >
>> > Hi Geert,
>> >
>> > Does the follwing help by any chance?
>> >
>> > From d6fd4a6e0fc2d3f0a74962d4a6f663a46d230ecd Mon Sep 17 00:00:00 2001
>> > diff --git a/arch/m68knommu/kernel/vmlinux.lds.S b/arch/m68knommu/kernel/vmlinux.lds.S
>> > index ef33213..47e15eb 100644
>> > --- a/arch/m68knommu/kernel/vmlinux.lds.S
>> > +++ b/arch/m68knommu/kernel/vmlinux.lds.S
>>
>> The crash happened on m68k with MMU, not m68knommu.
>>
>
> Hmm, OK then. Could you please see if the crash happens if you return
> early in kernel/params.c::version_sysfs_builtin() ? Also, do you see

It does not crash if version_sysfs_builtin() returns early.

> anything in __modev section of your build?

"objdump -h" says:

| Sections:
| Idx Name          Size      VMA       LMA       File off  Algn
|   7 __modver      0000007c  002e0f84  002e0f84  002e0f84  2**2
|                   CONTENTS, ALLOC, LOAD, DATA

"nm vmlinux | grep __modver" says:

| 002e0f84 d __modver_version_attr
| 002e0fa8 d __modver_version_attr
| 00039026 T __modver_version_show
| 002e0f84 D __start___modver
| 002e0fca D __stop___modver

The section size (0x7c) is larger than __stop___modver -
__start___modver (0x46)?

Adding some debugging code (which increases the section size even more?) shows:

vattr = 002e1004
vattr->module_name = xz_dec
    mk = 00c2ee50
    err = 0
    kobject_uevent done
    kobject_put done
vattr = 002e1026
vattr->module_name = (null)
Unable to handle kernel NULL pointer dereference at virtual address 0000002c

So the second module in the list has no name. Why?
Aha, it's not NULL, but just < PAGE_SIZE (0x2c).

sizeof(struct module_version_attribute) = 34, which you can see from
the 2 consecutive vattr
pointers above. But the "aligned(sizeof(void *))" in the definition of
MODULE_VERSION() puts
the next module_version_attribute struct in the array at offset 36,
not offset 34!
On m68k, the alignment of 32-bit integrals is 2 bytes, not 4.

Removing the alignment, cfr. this (gmail-pasted hence
whitespace-corrupted) patch
fixes it:

--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -176,7 +176,7 @@ extern struct module __this_module;
                                             struct module *, char *);  \
        static struct module_version_attribute __modver_version_attr    \
        __used                                                          \
-    __attribute__ ((__section__ ("__modver"),aligned(sizeof(void *)))) \
+    __attribute__ ((__section__ ("__modver"))) \
        = {                                                             \
                .mattr  = {                                             \
                        .attr   = {                                     \

Why is the explicit alignment there? Can it be removed?
If not, you can move the alignment into the definition of struct
module_version_attribute,
so it will be honoured everywhere.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: Early crash (was: Re: module: show version information for built-in modules in sysfs)

[Dmitry Torokhov]

On Wed, Feb 02, 2011 at 06:48:50AM -0800, Geert Uytterhoeven wrote:
> On Tue, Feb 1, 2011 at 23:26, Dmitry Torokhov <dtor@vmware.com> wrote:
> > On Tue, Feb 01, 2011 at 02:03:23PM -0800, Geert Uytterhoeven wrote:
> >> On Tue, Feb 1, 2011 at 22:09, Dmitry Torokhov <dtor@vmware.com> wrote:
> >> > On Tue, Feb 01, 2011 at 12:33:29PM -0800, Geert Uytterhoeven wrote:
> >> >> On Mon, Jan 24, 2011 at 11:59, Linux Kernel Mailing List
> >> >> <linux-kernel@vger.kernel.org> wrote:
> >> >> > Gitweb:     http://git.kernel.org/linus/e94965ed5beb23c6fabf7ed31f625e66d7ff28de
> >> >>
> >> >> >    module: show version information for built-in modules in sysfs
> >> >> >
> >> >> >    Currently only drivers that are built as modules have their versions
> >> >> >    shown in /sys/module/<module_name>/version, but this information might
> >> >> >    also be useful for built-in drivers as well. This especially important
> >> >> >    for drivers that do not define any parameters - such drivers, if
> >> >> >    built-in, are completely invisible from userspace.
> >> >> >
> >> >> >    This patch changes MODULE_VERSION() macro so that in case when we are
> >> >> >    compiling built-in module, version information is stored in a separate
> >> >> >    section. Kernel then uses this data to create 'version' sysfs attribute
> >> >> >    in the same fashion it creates attributes for module parameters.
> >> >>
> >> >> This commit causes the crash below on m68k (ARAnyM).
> >> >> Reverting this commit and its dependency
> >> >> 3b90a5b292321b2acac3921f77046ae195aef53f
> >> >> ("module: fix linker error for MODULE_VERSION when !MODULE and CONFIG_SYSFS=n")
> >> >> makes it boot again.
> >> >>
> >> >
> >> > Hi Geert,
> >> >
> >> > Does the follwing help by any chance?
> >> >
> >> > From d6fd4a6e0fc2d3f0a74962d4a6f663a46d230ecd Mon Sep 17 00:00:00 2001
> >> > diff --git a/arch/m68knommu/kernel/vmlinux.lds.S b/arch/m68knommu/kernel/vmlinux.lds.S
> >> > index ef33213..47e15eb 100644
> >> > --- a/arch/m68knommu/kernel/vmlinux.lds.S
> >> > +++ b/arch/m68knommu/kernel/vmlinux.lds.S
> >>
> >> The crash happened on m68k with MMU, not m68knommu.
> >>
> >
> > Hmm, OK then. Could you please see if the crash happens if you return
> > early in kernel/params.c::version_sysfs_builtin() ? Also, do you see
> 
> It does not crash if version_sysfs_builtin() returns early.
> 
> > anything in __modev section of your build?
> 
> "objdump -h" says:
> 
> | Sections:
> | Idx Name          Size      VMA       LMA       File off  Algn
> |   7 __modver      0000007c  002e0f84  002e0f84  002e0f84  2**2
> |                   CONTENTS, ALLOC, LOAD, DATA
> 
> "nm vmlinux | grep __modver" says:
> 
> | 002e0f84 d __modver_version_attr
> | 002e0fa8 d __modver_version_attr
> | 00039026 T __modver_version_show
> | 002e0f84 D __start___modver
> | 002e0fca D __stop___modver
> 
> The section size (0x7c) is larger than __stop___modver -
> __start___modver (0x46)?
> 
> Adding some debugging code (which increases the section size even more?) shows:
> 
> vattr = 002e1004
> vattr->module_name = xz_dec
>     mk = 00c2ee50
>     err = 0
>     kobject_uevent done
>     kobject_put done
> vattr = 002e1026
> vattr->module_name = (null)
> Unable to handle kernel NULL pointer dereference at virtual address 0000002c
> 
> So the second module in the list has no name. Why?
> Aha, it's not NULL, but just < PAGE_SIZE (0x2c).
> 
> sizeof(struct module_version_attribute) = 34, which you can see from
> the 2 consecutive vattr
> pointers above. But the "aligned(sizeof(void *))" in the definition of
> MODULE_VERSION() puts
> the next module_version_attribute struct in the array at offset 36,
> not offset 34!
> On m68k, the alignment of 32-bit integrals is 2 bytes, not 4.

But why is it aligned on 2-byte boundary and why m64k is not happy with
module_version_attribute but is happy with kernel_param which is also
aligned similarly?

If we unroll module_version_attribute it woud look like this:

struct module_version_attribute {

	struct module_attribute {

		struct attribute {
			const char *name;
			mode_t mode;
		} attr;
		...

	} mattr;

	const char *module_name;
	const char *version;
};

So I would expect it be aligned on (char *) boundary which should be the
same as (void *).

Will it help if we rearrange module_version_attribute definition to
explicitly have first field being a pointer so it is more like
kernel_param, like this:

struct module_version_attribute {
	const char *module_name;
	const char *version;
	struct module_attribute mattr;
};

Thanks,
Dmitry

Re: Early crash (was: Re: module: show version information for built-in modules in sysfs)

[Andreas Schwab]

Dmitry Torokhov <dtor@vmware.com> writes:

> But why is it aligned on 2-byte boundary and why m64k is not happy with
> module_version_attribute but is happy with kernel_param which is also
> aligned similarly?

struct kernel_parm doesn't contain internal padding on 32 bit
architectures (it does on 64bit architectures though).

> If we unroll module_version_attribute it woud look like this:
>
> struct module_version_attribute {
>
> 	struct module_attribute {
>
> 		struct attribute {
> 			const char *name;
> 			mode_t mode;
> 		} attr;
> 		...
>
> 	} mattr;
>
> 	const char *module_name;
> 	const char *version;
> };
>
> So I would expect it be aligned on (char *) boundary which should be the
> same as (void *).

mode_t is a 16 bit type, thus any following member becomes aligned on an
odd 2 byte boundary.  On 32bit architectures with 4 byte alignment and
16 bit mode_t struct attribute contains 2 bytes of internal padding.
(64bit architectures typically have a 32bit mode_t, and there are 4
bytes of padding.)

> Will it help if we rearrange module_version_attribute definition to
> explicitly have first field being a pointer so it is more like
> kernel_param, like this:
>
> struct module_version_attribute {
> 	const char *module_name;
> 	const char *version;
> 	struct module_attribute mattr;
> };

That won't change the total size of the structure.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Early crash (was: Re: module: show version information for built-in modules in sysfs)

[Dmitry Torokhov]

On Wed, Feb 02, 2011 at 02:52:04PM -0800, Andreas Schwab wrote:
> Dmitry Torokhov <dtor@vmware.com> writes:
> 
> > But why is it aligned on 2-byte boundary and why m64k is not happy with
> > module_version_attribute but is happy with kernel_param which is also
> > aligned similarly?
> 
> struct kernel_parm doesn't contain internal padding on 32 bit
> architectures (it does on 64bit architectures though).
> 
> > If we unroll module_version_attribute it woud look like this:
> >
> > struct module_version_attribute {
> >
> > 	struct module_attribute {
> >
> > 		struct attribute {
> > 			const char *name;
> > 			mode_t mode;
> > 		} attr;
> > 		...
> >
> > 	} mattr;
> >
> > 	const char *module_name;
> > 	const char *version;
> > };
> >
> > So I would expect it be aligned on (char *) boundary which should be the
> > same as (void *).
> 
> mode_t is a 16 bit type, thus any following member becomes aligned on an
> odd 2 byte boundary.

Even pointers? I'd expect pointers to be aligned on 4-bytes boundaries?

Thanks,

Dmitry

Re: Early crash (was: Re: module: show version information for built-in modules in sysfs)

[Andreas Schwab]

Dmitry Torokhov <dtor@vmware.com> writes:

> Even pointers? I'd expect pointers to be aligned on 4-bytes boundaries?

Pointers are not special in any way.  Why should they?  On the machine
level pointers are just numbers.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Early crash (was: Re: module: show version information for built-in modules in sysfs)

[Dmitry Torokhov]

On Wed, Feb 02, 2011 at 04:10:04PM -0800, Andreas Schwab wrote:
> Dmitry Torokhov <dtor@vmware.com> writes:
> 
> > Even pointers? I'd expect pointers to be aligned on 4-bytes boundaries?
> 
> Pointers are not special in any way.  Why should they?  On the machine
> level pointers are just numbers.

Are pointers (along with ints/longs) on m68k naturally aligned on word
boundary even though they are 32 bit?

Anyway, here is the description that introduced alignment statement:

commit 02dba5c6439cff34936460b95cd1ba42b370f345
Author: ak <ak>
Date:   Sat Jun 21 16:18:16 2003 +0000

    [PATCH] Fix over-alignment problem on x86-64

    Thanks to Jan Hubicka who suggested this fix.

    The problem seems to be that gcc generates a 32byte alignment for static
    objects > 32bytes.  This causes gas to set a high alignment on the
    section, which causes the uneven (not multiple of sizeof(struct
    kernel_param)) section size.  The pointer division with a base not being
    a multiple of sizeof(*ptr) then causes the invalid result.

    This just forces a small alignment, which makes the section end come out
    with the correct alignment.

    The only mystery left is why ld chose a 16 byte padding instead of
    32byte.

    BKrev: 3ef485487jZN-h3PtASDeL2Vs55NIg


I guess this does not directly apply to modversions since they are
currently under 32 bytes, but I wonder what happen if we decide to
extend one of the structures involved...

I guess explicitly setting alignment requirement for struct
module_version_attribute is the best option.

Thanks,
Dmitry

Re: Early crash (was: Re: module: show version information for built-in modules in sysfs)

[Andreas Schwab]

Dmitry Torokhov <dtor@vmware.com> writes:

> Are pointers (along with ints/longs) on m68k naturally aligned on word
> boundary even though they are 32 bit?

On m68k, every object by default is at most aligned to a 2 byte
boundary.  That's a heritage of the original Sun m68000 compiler, which
served as the template for the gcc configuration for Linux/m68k when the
porting started.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Early crash (was: Re: module: show version information for built-in modules in sysfs)

[Dmitry Torokhov]

On Wed, Feb 02, 2011 at 04:24:59PM -0800, Dmitry Torokhov wrote:
> On Wed, Feb 02, 2011 at 04:10:04PM -0800, Andreas Schwab wrote:
> > Dmitry Torokhov <dtor@vmware.com> writes:
> > 
> > > Even pointers? I'd expect pointers to be aligned on 4-bytes boundaries?
> > 
> > Pointers are not special in any way.  Why should they?  On the machine
> > level pointers are just numbers.
> 
> Are pointers (along with ints/longs) on m68k naturally aligned on word
> boundary even though they are 32 bit?
> 
> Anyway, here is the description that introduced alignment statement:
> 
> commit 02dba5c6439cff34936460b95cd1ba42b370f345
> Author: ak <ak>
> Date:   Sat Jun 21 16:18:16 2003 +0000
> 
>     [PATCH] Fix over-alignment problem on x86-64
> 
>     Thanks to Jan Hubicka who suggested this fix.
> 
>     The problem seems to be that gcc generates a 32byte alignment for static
>     objects > 32bytes.  This causes gas to set a high alignment on the
>     section, which causes the uneven (not multiple of sizeof(struct
>     kernel_param)) section size.  The pointer division with a base not being
>     a multiple of sizeof(*ptr) then causes the invalid result.
> 
>     This just forces a small alignment, which makes the section end come out
>     with the correct alignment.
> 
>     The only mystery left is why ld chose a 16 byte padding instead of
>     32byte.
> 
>     BKrev: 3ef485487jZN-h3PtASDeL2Vs55NIg
> 
> 
> I guess this does not directly apply to modversions since they are
> currently under 32 bytes, but I wonder what happen if we decide to
> extend one of the structures involved...
> 
> I guess explicitly setting alignment requirement for struct
> module_version_attribute is the best option.
> 

So here is the patch that explicitly specifies alignment for struct
module_version_attribute. I tested it on i386 and x86_64 and I believe
it will fix the issue with m68k but I do not have access to such a box.

Thanks,
Dmitry

>From f0e0e10b58b22047e36e21a022abf5e86b5819c2 Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <dtor@vmware.com>
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 <geert@linux-m68k.org>
Signed-off-by: Dmitry Torokhov <dtor@vmware.com>
---
 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

Re: Early crash

[David Miller]

From: Dmitry Torokhov <dtor@vmware.com>
Date: Mon, 7 Feb 2011 00:19:33 -0800

> So here is the patch that explicitly specifies alignment for struct
> module_version_attribute. I tested it on i386 and x86_64 and I believe
> it will fix the issue with m68k but I do not have access to such a box.

While this may or may not fix the m68k issue, this isn't really
sufficient to make this thing work in all cases.  And the older
tracepoint commits referenced in this thread are known to cause
problems with platforms such as sparc64.

You 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".

Your attribute specification is only a lower-bound.

GCC can and does use variable alignment choices in different situations,
so the align directive in the various objects can all be different.  So
the array iteration will assume the iterations should use one object
size, but within the linked together section the alignments are all
different so the inter-struct gap is different.

The only portable mechanism that will work in all cases, as we've
found recently for tracepoints and similar, is to make an array of
plain pointers to the objects in the special section.

See:

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

Re: Early crash

[Dmitry Torokhov]

On Mon, Feb 07, 2011 at 12:50:10AM -0800, David Miller wrote:
> From: Dmitry Torokhov <dtor@vmware.com>
> Date: Mon, 7 Feb 2011 00:19:33 -0800
> 
> > So here is the patch that explicitly specifies alignment for struct
> > module_version_attribute. I tested it on i386 and x86_64 and I believe
> > it will fix the issue with m68k but I do not have access to such a box.
> 
> While this may or may not fix the m68k issue, this isn't really
> sufficient to make this thing work in all cases.  And the older
> tracepoint commits referenced in this thread are known to cause
> problems with platforms such as sparc64.
> 
> You 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".
> 
> Your attribute specification is only a lower-bound.

Right.

> 
> GCC can and does use variable alignment choices in different situations,
> so the align directive in the various objects can all be different.  So
> the array iteration will assume the iterations should use one object
> size, but within the linked together section the alignments are all
> different so the inter-struct gap is different.
> 
> The only portable mechanism that will work in all cases, as we've
> found recently for tracepoints and similar, is to make an array of
> plain pointers to the objects in the special section.

But, theoretically speaking, nothing stops GCC to align pointers with
"gaps" as well? Let's say having everything (or some) aligned on
quadword boundary even though arch is 32 bit?


Thanks,

Dmitry

Re: Early crash

[David Miller]

From: Dmitry Torokhov <dtor@vmware.com>
Date: Mon, 7 Feb 2011 08:58:29 -0800

> But, theoretically speaking, nothing stops GCC to align pointers with
> "gaps" as well? Let's say having everything (or some) aligned on
> quadword boundary even though arch is 32 bit?

The alignment business only applies to aggregates (ie. structs and
unions).

This has been confirmed via several weeks of expermentation with
different GCC versions on different platforms as well.

Re: Early crash

[Dmitry Torokhov]

On Mon, Feb 07, 2011 at 11:27:08AM -0800, David Miller wrote:
> From: Dmitry Torokhov <dtor@vmware.com>
> Date: Mon, 7 Feb 2011 08:58:29 -0800
> 
> > But, theoretically speaking, nothing stops GCC to align pointers with
> > "gaps" as well? Let's say having everything (or some) aligned on
> > quadword boundary even though arch is 32 bit?
> 
> The alignment business only applies to aggregates (ie. structs and
> unions).
> 
> This has been confirmed via several weeks of expermentation with
> different GCC versions on different platforms as well.

I see. OK, then I'll prepare a patch to switch over to struct + pointer
scheme.


Thanks,

Dmitry

Re: Early crash

[Rusty Russell]

On Tue, 8 Feb 2011 05:57:08 am David Miller wrote:
> From: Dmitry Torokhov <dtor@vmware.com>
> Date: Mon, 7 Feb 2011 08:58:29 -0800
> 
> > But, theoretically speaking, nothing stops GCC to align pointers with
> > "gaps" as well? Let's say having everything (or some) aligned on
> > quadword boundary even though arch is 32 bit?
> 
> The alignment business only applies to aggregates (ie. structs and
> unions).
> 
> This has been confirmed via several weeks of expermentation with
> different GCC versions on different platforms as well.

But OTOH, this is an old problem which was faced by module params since
pre-git.  And we use the-align-to-void*-size method there; I vaguely recall
inserting it.

You've now got me wondering whether these platforms have broken builtin
module parameters, but I think it would crash iterating if you had any
boot parameters at all if that were the case.

So do we fix that now too, or wait for it to break?
Rusty.

Re: Early crash

[David Miller]

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Tue, 8 Feb 2011 13:42:48 +1030

> But OTOH, this is an old problem which was faced by module params since
> pre-git.  And we use the-align-to-void*-size method there; I vaguely recall
> inserting it.
> 
> You've now got me wondering whether these platforms have broken builtin
> module parameters, but I think it would crash iterating if you had any
> boot parameters at all if that were the case.
> 
> So do we fix that now too, or wait for it to break?

Good question.

I think the magic align magic on x86_64 in gcc only kicks in if a
structure is larger than a certain size.  But of course they could
change that at some point if they like.

So probably better safe than sorry.