[PATCH] include/linux/unaligned: Pack the whole struct rather than just the field.

[PATCH] include/linux/unaligned: Pack the whole struct rather than just the field.

[Will Newton]

[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]

The current packed struct implementation of unaligned access adds
the packed attribute only to the field within the unaligned struct
rather than to the struct as a whole. This is not sufficient to
enforce proper behaviour on architectures with a default struct
alignment of more than one byte.

For example, the current implementation of __get_unaligned_cpu16
when compiled for arm with gcc -O1 -mstructure-size-boundary=32
assumes the struct is on a 4 byte boundary so performs the load
of the 16bit packed field as if it were on a 4 byte boundary:

__get_unaligned_cpu16:
        ldrh    r0, [r0, #0]
        bx      lr

Moving the packed attribute to the struct rather than the field
causes the proper unaligned access code to be generated:

__get_unaligned_cpu16:
	ldrb	r3, [r0, #0]	@ zero_extendqisi2
	ldrb	r0, [r0, #1]	@ zero_extendqisi2
	orr	r0, r3, r0, asl #8
	bx	lr

Signed-off-by: Will Newton <will.newton@gmail.com>
---
 include/linux/unaligned/packed_struct.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/unaligned/packed_struct.h
b/include/linux/unaligned/packed_struct.h
index 2498bb9..c9a6abd 100644
--- a/include/linux/unaligned/packed_struct.h
+++ b/include/linux/unaligned/packed_struct.h
@@ -3,9 +3,9 @@

 #include <linux/kernel.h>

-struct __una_u16 { u16 x __attribute__((packed)); };
-struct __una_u32 { u32 x __attribute__((packed)); };
-struct __una_u64 { u64 x __attribute__((packed)); };
+struct __una_u16 { u16 x; } __attribute__((packed));
+struct __una_u32 { u32 x; } __attribute__((packed));
+struct __una_u64 { u64 x; } __attribute__((packed));

 static inline u16 __get_unaligned_cpu16(const void *p)
 {
-- 
1.7.0.4

[-- Attachment #2: 0001-include-linux-unaligned-Pack-the-whole-struct-rather.patch --]
[-- Type: text/x-patch, Size: 1943 bytes --]

From bc1a150d8e747a514392279466518431c31c618f Mon Sep 17 00:00:00 2001
From: Will Newton <will.newton@gmail.com>
Date: Wed, 1 Dec 2010 21:45:24 +0000
Subject: [PATCH] include/linux/unaligned: Pack the whole struct rather than just the field.

The current packed struct implementation of unaligned access adds
the packed attribute only to the field within the unaligned struct,
rather than to the struct as a whole. This is not sufficient to
enforce proper behaviour on architectures with a default struct
alignment of more than one byte.

For example, the current implementation of __get_unaligned_cpu16
when compiled for arm with gcc -O1 -mstructure-size-boundary=32
assumes the struct is on a 4 byte boundary so performs the load
of the 16bit packed field as if it were on a 4 byte boundary:

__get_unaligned_cpu16:
        ldrh    r0, [r0, #0]
        bx      lr

Moving the packed attribute to the struct rather than the field
causes the proper unaligned access code to be generated:

__get_unaligned_cpu16:
	ldrb	r3, [r0, #0]	@ zero_extendqisi2
	ldrb	r0, [r0, #1]	@ zero_extendqisi2
	orr	r0, r3, r0, asl #8
	bx	lr

Signed-off-by: Will Newton <will.newton@gmail.com>
---
 include/linux/unaligned/packed_struct.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/unaligned/packed_struct.h b/include/linux/unaligned/packed_struct.h
index 2498bb9..c9a6abd 100644
--- a/include/linux/unaligned/packed_struct.h
+++ b/include/linux/unaligned/packed_struct.h
@@ -3,9 +3,9 @@
 
 #include <linux/kernel.h>
 
-struct __una_u16 { u16 x __attribute__((packed)); };
-struct __una_u32 { u32 x __attribute__((packed)); };
-struct __una_u64 { u64 x __attribute__((packed)); };
+struct __una_u16 { u16 x; } __attribute__((packed));
+struct __una_u32 { u32 x; } __attribute__((packed));
+struct __una_u64 { u64 x; } __attribute__((packed));
 
 static inline u16 __get_unaligned_cpu16(const void *p)
 {
-- 
1.7.0.4

Re: [PATCH] include/linux/unaligned: Pack the whole struct rather than just the field.

[Will Newton]

On Wed, Dec 1, 2010 at 10:11 PM, Will Newton <will.newton@gmail.com> wrote:
> The current packed struct implementation of unaligned access adds
> the packed attribute only to the field within the unaligned struct
> rather than to the struct as a whole. This is not sufficient to
> enforce proper behaviour on architectures with a default struct
> alignment of more than one byte.
>
> For example, the current implementation of __get_unaligned_cpu16
> when compiled for arm with gcc -O1 -mstructure-size-boundary=32
> assumes the struct is on a 4 byte boundary so performs the load
> of the 16bit packed field as if it were on a 4 byte boundary:
>
> __get_unaligned_cpu16:
>        ldrh    r0, [r0, #0]
>        bx      lr
>
> Moving the packed attribute to the struct rather than the field
> causes the proper unaligned access code to be generated:
>
> __get_unaligned_cpu16:
>        ldrb    r3, [r0, #0]    @ zero_extendqisi2
>        ldrb    r0, [r0, #1]    @ zero_extendqisi2
>        orr     r0, r3, r0, asl #8
>        bx      lr
>
> Signed-off-by: Will Newton <will.newton@gmail.com>

I don't know of any designated maintainer for this code, but add a
couple of ccs of people who might be interested.

This change doesn't change any behaviour on current architectures, but
is more correct and may enable other architectures to move to the
packed struct unaligned access implementation and so allow us to move
more architectures to the asm-generic implementation.

> ---
>  include/linux/unaligned/packed_struct.h |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/unaligned/packed_struct.h
> b/include/linux/unaligned/packed_struct.h
> index 2498bb9..c9a6abd 100644
> --- a/include/linux/unaligned/packed_struct.h
> +++ b/include/linux/unaligned/packed_struct.h
> @@ -3,9 +3,9 @@
>
>  #include <linux/kernel.h>
>
> -struct __una_u16 { u16 x __attribute__((packed)); };
> -struct __una_u32 { u32 x __attribute__((packed)); };
> -struct __una_u64 { u64 x __attribute__((packed)); };
> +struct __una_u16 { u16 x; } __attribute__((packed));
> +struct __una_u32 { u32 x; } __attribute__((packed));
> +struct __una_u64 { u64 x; } __attribute__((packed));
>
>  static inline u16 __get_unaligned_cpu16(const void *p)
>  {
> --
> 1.7.0.4
>

Re: [PATCH] include/linux/unaligned: Pack the whole struct rather than just the field.

[Will Newton]

On Tue, Dec 7, 2010 at 11:12 AM, Will Newton <will.newton@gmail.com> wrote:
> On Wed, Dec 1, 2010 at 10:11 PM, Will Newton <will.newton@gmail.com> wrote:
>> The current packed struct implementation of unaligned access adds
>> the packed attribute only to the field within the unaligned struct
>> rather than to the struct as a whole. This is not sufficient to
>> enforce proper behaviour on architectures with a default struct
>> alignment of more than one byte.
>>
>> For example, the current implementation of __get_unaligned_cpu16
>> when compiled for arm with gcc -O1 -mstructure-size-boundary=32
>> assumes the struct is on a 4 byte boundary so performs the load
>> of the 16bit packed field as if it were on a 4 byte boundary:
>>
>> __get_unaligned_cpu16:
>>        ldrh    r0, [r0, #0]
>>        bx      lr
>>
>> Moving the packed attribute to the struct rather than the field
>> causes the proper unaligned access code to be generated:
>>
>> __get_unaligned_cpu16:
>>        ldrb    r3, [r0, #0]    @ zero_extendqisi2
>>        ldrb    r0, [r0, #1]    @ zero_extendqisi2
>>        orr     r0, r3, r0, asl #8
>>        bx      lr
>>
>> Signed-off-by: Will Newton <will.newton@gmail.com>
>
> I don't know of any designated maintainer for this code, but add a
> couple of ccs of people who might be interested.
>
> This change doesn't change any behaviour on current architectures, but
> is more correct and may enable other architectures to move to the
> packed struct unaligned access implementation and so allow us to move
> more architectures to the asm-generic implementation.

Ping!

Does anyone have any opinions on this?

>> ---
>>  include/linux/unaligned/packed_struct.h |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/unaligned/packed_struct.h
>> b/include/linux/unaligned/packed_struct.h
>> index 2498bb9..c9a6abd 100644
>> --- a/include/linux/unaligned/packed_struct.h
>> +++ b/include/linux/unaligned/packed_struct.h
>> @@ -3,9 +3,9 @@
>>
>>  #include <linux/kernel.h>
>>
>> -struct __una_u16 { u16 x __attribute__((packed)); };
>> -struct __una_u32 { u32 x __attribute__((packed)); };
>> -struct __una_u64 { u64 x __attribute__((packed)); };
>> +struct __una_u16 { u16 x; } __attribute__((packed));
>> +struct __una_u32 { u32 x; } __attribute__((packed));
>> +struct __una_u64 { u64 x; } __attribute__((packed));
>>
>>  static inline u16 __get_unaligned_cpu16(const void *p)
>>  {
>> --
>> 1.7.0.4
>>
>

Re: [PATCH] include/linux/unaligned: Pack the whole struct rather than just the field.

[Andrew Morton]

On Wed, 1 Dec 2010 22:11:53 +0000 Will Newton <will.newton@gmail.com> wrote:

> The current packed struct implementation of unaligned access adds
> the packed attribute only to the field within the unaligned struct
> rather than to the struct as a whole. This is not sufficient to
> enforce proper behaviour on architectures with a default struct
> alignment of more than one byte.
> 
> For example, the current implementation of __get_unaligned_cpu16
> when compiled for arm with gcc -O1 -mstructure-size-boundary=32
> assumes the struct is on a 4 byte boundary so performs the load
> of the 16bit packed field as if it were on a 4 byte boundary:
> 
> __get_unaligned_cpu16:
>         ldrh    r0, [r0, #0]
>         bx      lr
> 
> Moving the packed attribute to the struct rather than the field
> causes the proper unaligned access code to be generated:
> 
> __get_unaligned_cpu16:
> 	ldrb	r3, [r0, #0]	@ zero_extendqisi2
> 	ldrb	r0, [r0, #1]	@ zero_extendqisi2
> 	orr	r0, r3, r0, asl #8
> 	bx	lr
> 
> Signed-off-by: Will Newton <will.newton@gmail.com>
> ---
>  include/linux/unaligned/packed_struct.h |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/unaligned/packed_struct.h
> b/include/linux/unaligned/packed_struct.h
> index 2498bb9..c9a6abd 100644
> --- a/include/linux/unaligned/packed_struct.h
> +++ b/include/linux/unaligned/packed_struct.h
> @@ -3,9 +3,9 @@
> 
>  #include <linux/kernel.h>
> 
> -struct __una_u16 { u16 x __attribute__((packed)); };
> -struct __una_u32 { u32 x __attribute__((packed)); };
> -struct __una_u64 { u64 x __attribute__((packed)); };
> +struct __una_u16 { u16 x; } __attribute__((packed));
> +struct __una_u32 { u32 x; } __attribute__((packed));
> +struct __una_u64 { u64 x; } __attribute__((packed));
> 

Yes, that was wrong.

Do you think this bug affects 2.6.36 or earlier?

Even if it doesn't, it looks like a bit of a hand-grenade to leave it
unfixed in earlier kernels.

Re: [PATCH] include/linux/unaligned: Pack the whole struct rather than just the field.

[Will Newton]

On Tue, Dec 21, 2010 at 5:44 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 1 Dec 2010 22:11:53 +0000 Will Newton <will.newton@gmail.com> wrote:
>
>> The current packed struct implementation of unaligned access adds
>> the packed attribute only to the field within the unaligned struct
>> rather than to the struct as a whole. This is not sufficient to
>> enforce proper behaviour on architectures with a default struct
>> alignment of more than one byte.
>>
>> For example, the current implementation of __get_unaligned_cpu16
>> when compiled for arm with gcc -O1 -mstructure-size-boundary=32
>> assumes the struct is on a 4 byte boundary so performs the load
>> of the 16bit packed field as if it were on a 4 byte boundary:
>>
>> __get_unaligned_cpu16:
>>         ldrh    r0, [r0, #0]
>>         bx      lr
>>
>> Moving the packed attribute to the struct rather than the field
>> causes the proper unaligned access code to be generated:
>>
>> __get_unaligned_cpu16:
>>       ldrb    r3, [r0, #0]    @ zero_extendqisi2
>>       ldrb    r0, [r0, #1]    @ zero_extendqisi2
>>       orr     r0, r3, r0, asl #8
>>       bx      lr
>>
>> Signed-off-by: Will Newton <will.newton@gmail.com>
>> ---
>>  include/linux/unaligned/packed_struct.h |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/unaligned/packed_struct.h
>> b/include/linux/unaligned/packed_struct.h
>> index 2498bb9..c9a6abd 100644
>> --- a/include/linux/unaligned/packed_struct.h
>> +++ b/include/linux/unaligned/packed_struct.h
>> @@ -3,9 +3,9 @@
>>
>>  #include <linux/kernel.h>
>>
>> -struct __una_u16 { u16 x __attribute__((packed)); };
>> -struct __una_u32 { u32 x __attribute__((packed)); };
>> -struct __una_u64 { u64 x __attribute__((packed)); };
>> +struct __una_u16 { u16 x; } __attribute__((packed));
>> +struct __una_u32 { u32 x; } __attribute__((packed));
>> +struct __una_u64 { u64 x; } __attribute__((packed));
>>
>
> Yes, that was wrong.
>
> Do you think this bug affects 2.6.36 or earlier?

The code is present in 2.6.36 and earlier (the code itself has been
around since 2.6.26). However from looking at the gcc source code, the
only architectures that have a non-8 bit STRUCTURE_SIZE_BOUNDARY are:

arm - although it may need a compiler flag, and it uses bitshift
unaligned access code by default.
crx - doesn't run Linux.
m68k/openbsd
sh - with a deprecated compiler flag

So I think the likelihood of someone tripping over it is quite small,
and is most likely to affect freshly merged architectures that aren't
in the gcc tree yet.

> Even if it doesn't, it looks like a bit of a hand-grenade to leave it
> unfixed in earlier kernels.

I'm not sure what the right thing to do is - there is a very small
chance that the change could cause compiler bugs to surface that have
gone unnoticed before now, but the new code is strictly more correct.

Re: [PATCH] include/linux/unaligned: Pack the whole struct rather than just the field.

[Harvey Harrison]

On Tue, Dec 21, 2010 at 2:43 AM, Will Newton <will.newton@gmail.com> wrote:
> On Tue, Dec 21, 2010 at 5:44 AM, Andrew Morton
>>>  #include <linux/kernel.h>

>>>
>>> -struct __una_u16 { u16 x __attribute__((packed)); };
>>> -struct __una_u32 { u32 x __attribute__((packed)); };
>>> -struct __una_u64 { u64 x __attribute__((packed)); };
>>> +struct __una_u16 { u16 x; } __attribute__((packed));
>>> +struct __una_u32 { u32 x; } __attribute__((packed));
>>> +struct __una_u64 { u64 x; } __attribute__((packed));
>>>
>>
>> Yes, that was wrong.
>>
>> Do you think this bug affects 2.6.36 or earlier?

Interesting, I thought about this when modifying this awhile ago, and
was relying on this from the gcc manual:

packed
<snip>

    Specifying this attribute for struct and union types is equivalent
to specifying the packed attribute on each of the


^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
structure or union members. Specifying the -fshort-enums flag on the
line is equivalent to specifying the packed
attribute on all enum definitions.

What version of gcc was this?

So, no objection to specifying packed on the struct definition rather
than the member, it was only done that way
as that was what was there when I made some of the code common.

Harvey

Re: [PATCH] include/linux/unaligned: Pack the whole struct rather than just the field.

[Will Newton]

On Tue, Dec 21, 2010 at 11:36 PM, Harvey Harrison
<harvey.harrison@gmail.com> wrote:
> On Tue, Dec 21, 2010 at 2:43 AM, Will Newton <will.newton@gmail.com> wrote:
>> On Tue, Dec 21, 2010 at 5:44 AM, Andrew Morton
>>>>  #include <linux/kernel.h>
>
>>>>
>>>> -struct __una_u16 { u16 x __attribute__((packed)); };
>>>> -struct __una_u32 { u32 x __attribute__((packed)); };
>>>> -struct __una_u64 { u64 x __attribute__((packed)); };
>>>> +struct __una_u16 { u16 x; } __attribute__((packed));
>>>> +struct __una_u32 { u32 x; } __attribute__((packed));
>>>> +struct __una_u64 { u64 x; } __attribute__((packed));
>>>>
>>>
>>> Yes, that was wrong.
>>>
>>> Do you think this bug affects 2.6.36 or earlier?
>
> Interesting, I thought about this when modifying this awhile ago, and
> was relying on this from the gcc manual:
>
> packed
> <snip>
>
>    Specifying this attribute for struct and union types is equivalent
> to specifying the packed attribute on each of the
>
>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> structure or union members. Specifying the -fshort-enums flag on the
> line is equivalent to specifying the packed
> attribute on all enum definitions.

I think that's possibly a gcc documentation deficiency, it is
equivalent to adding the packed attribute to all the struct members
but *also* affects the assumed alignment of the struct.

Looking at the gcc svn it seems the documentation has been improved a
bit in this area.

> What version of gcc was this?

4.2.4