From: Matthieu CASTET <[email protected]>
Fix possible unaligned u32 access in b43_generate_plcp_hdr().
Unaligned data is read/write with a u32 pointer instead of using the
packed structure. Some versions of gcc ignore the "packed" attribute, if the
structure element is accessed through a local pointer.
Signed-off-by: Matthieu CASTET <[email protected]>
Signed-off-by: Michael Buesch <[email protected]>
---
Please queue this bugfix.
diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
index a63d888..55f36a7 100644
--- a/drivers/net/wireless/b43/xmit.c
+++ b/drivers/net/wireless/b43/xmit.c
@@ -118,7 +118,6 @@ u8 b43_plcp_get_ratecode_ofdm(const u8 bitrate)
void b43_generate_plcp_hdr(struct b43_plcp_hdr4 *plcp,
const u16 octets, const u8 bitrate)
{
- __le32 *data = &(plcp->data);
__u8 *raw = plcp->raw;
if (b43_is_ofdm_rate(bitrate)) {
@@ -127,7 +126,7 @@ void b43_generate_plcp_hdr(struct b43_plcp_hdr4 *plcp,
d = b43_plcp_get_ratecode_ofdm(bitrate);
B43_WARN_ON(octets & 0xF000);
d |= (octets << 5);
- *data = cpu_to_le32(d);
+ plcp->data = cpu_to_le32(d);
} else {
u32 plen;
@@ -141,7 +140,7 @@ void b43_generate_plcp_hdr(struct b43_plcp_hdr4 *plcp,
raw[1] = 0x04;
} else
raw[1] = 0x04;
- *data |= cpu_to_le32(plen << 16);
+ plcp->data |= cpu_to_le32(plen << 16);
raw[0] = b43_plcp_get_ratecode_cck(bitrate);
}
}
--
Greetings, Michael.
On Fri, Jun 05, 2009 at 05:03:57PM +0200, [email protected] wrote:
> Quoting "John W. Linville" <[email protected]>:
>
> > On Thu, Jun 04, 2009 at 11:18:33PM +0200, Michael Buesch wrote:
> > > From: Matthieu CASTET <[email protected]>
> > >
> > > Fix possible unaligned u32 access in b43_generate_plcp_hdr().
> > > Unaligned data is read/write with a u32 pointer instead of using the
> > > packed structure. Some versions of gcc ignore the "packed" attribute, if
> > the
> > > structure element is accessed through a local pointer.
> > >
> > > Signed-off-by: Matthieu CASTET <[email protected]>
> > > Signed-off-by: Michael Buesch <[email protected]>
> >
> > That seems pretty brain-dead...can you cite a source for this
> > information?
> The test I did with the attached test case in the first post.
Link?
> I don't see why gcc should propagate packed structure info to assignment.
> That will be impossible to handle (think of passing it to function parameter).
Perhaps this is obvious to you, but it isn't to me.
>
> > The patch seems like a no-op...
> At least the code produced on mips is different.
Then why aren't you trying to get the mips gcc guys to fix it?
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Friday 05 June 2009 20:50:48 John W. Linville wrote:
> On Fri, Jun 05, 2009 at 07:29:07PM +0200, Michael Buesch wrote:
> > On Friday 05 June 2009 14:25:03 John W. Linville wrote:
> > > On Thu, Jun 04, 2009 at 11:18:33PM +0200, Michael Buesch wrote:
> > > > From: Matthieu CASTET <[email protected]>
> > > >
> > > > Fix possible unaligned u32 access in b43_generate_plcp_hdr().
> > > > Unaligned data is read/write with a u32 pointer instead of using the
> > > > packed structure. Some versions of gcc ignore the "packed" attribute, if the
> > > > structure element is accessed through a local pointer.
> > > >
> > > > Signed-off-by: Matthieu CASTET <[email protected]>
> > > > Signed-off-by: Michael Buesch <[email protected]>
> > >
> > > That seems pretty brain-dead...can you cite a source for this
> > > information? The patch seems like a no-op...
> > >
> > > John
> >
> > struct foo {
> > int data;
> > } __attribute__((packed));
> >
> > struct foo foo;
> > int *d = &foo->data;
> > foo->data = x; /* Works for unaligned */
> > *d = y; /* Does not work for unaligned */
>
> Why not?
>
Because some compilers don't carry the "packed" attribute of "data" though the "d" pointer.
So foo->data is a "packed" access, while *d might not.
--
Greetings, Michael.
From: "John W. Linville" <[email protected]>
Date: Fri, 5 Jun 2009 14:50:48 -0400
> On Fri, Jun 05, 2009 at 07:29:07PM +0200, Michael Buesch wrote:
>> On Friday 05 June 2009 14:25:03 John W. Linville wrote:
>> > On Thu, Jun 04, 2009 at 11:18:33PM +0200, Michael Buesch wrote:
>> > > From: Matthieu CASTET <[email protected]>
>> > >
>> > > Fix possible unaligned u32 access in b43_generate_plcp_hdr().
>> > > Unaligned data is read/write with a u32 pointer instead of using the
>> > > packed structure. Some versions of gcc ignore the "packed" attribute, if the
>> > > structure element is accessed through a local pointer.
>> > >
>> > > Signed-off-by: Matthieu CASTET <[email protected]>
>> > > Signed-off-by: Michael Buesch <[email protected]>
>> >
>> > That seems pretty brain-dead...can you cite a source for this
>> > information? The patch seems like a no-op...
>> >
>> > John
>>
>> struct foo {
>> int data;
>> } __attribute__((packed));
>>
>> struct foo foo;
>> int *d = &foo->data;
>> foo->data = x; /* Works for unaligned */
>> *d = y; /* Does not work for unaligned */
>
> Why not?
Because "int *" does not have the packed attribute.
From: "John W. Linville" <[email protected]>
Date: Fri, 5 Jun 2009 08:25:03 -0400
> On Thu, Jun 04, 2009 at 11:18:33PM +0200, Michael Buesch wrote:
>> From: Matthieu CASTET <[email protected]>
>>
>> Fix possible unaligned u32 access in b43_generate_plcp_hdr().
>> Unaligned data is read/write with a u32 pointer instead of using the
>> packed structure. Some versions of gcc ignore the "packed" attribute, if the
>> structure element is accessed through a local pointer.
>>
>> Signed-off-by: Matthieu CASTET <[email protected]>
>> Signed-off-by: Michael Buesch <[email protected]>
>
> That seems pretty brain-dead...can you cite a source for this
> information? The patch seems like a no-op...
The "packed" attribute simply doesn't propagate. It isn't
a GCC bug. Look at this:
struct foo {
u32 x;
} attribute((packed));
u32 bar(struct foo *p)
{
u32 *p = &p->x;
return *p;
}
That will (correctly) do a 32-bit aligned load, only doing
the "return p->x;" will proply respect the packed attribute.
Quoting "John W. Linville" <[email protected]>:
> On Thu, Jun 04, 2009 at 11:18:33PM +0200, Michael Buesch wrote:
> > From: Matthieu CASTET <[email protected]>
> >
> > Fix possible unaligned u32 access in b43_generate_plcp_hdr().
> > Unaligned data is read/write with a u32 pointer instead of using the
> > packed structure. Some versions of gcc ignore the "packed" attribute, if
> the
> > structure element is accessed through a local pointer.
> >
> > Signed-off-by: Matthieu CASTET <[email protected]>
> > Signed-off-by: Michael Buesch <[email protected]>
>
> That seems pretty brain-dead...can you cite a source for this
> information?
The test I did with the attached test case in the first post.
I don't see why gcc should propagate packed structure info to assignment.
That will be impossible to handle (think of passing it to function parameter).
> The patch seems like a no-op...
At least the code produced on mips is different.
Matthieu
On Fri, Jun 05, 2009 at 07:29:07PM +0200, Michael Buesch wrote:
> On Friday 05 June 2009 14:25:03 John W. Linville wrote:
> > On Thu, Jun 04, 2009 at 11:18:33PM +0200, Michael Buesch wrote:
> > > From: Matthieu CASTET <[email protected]>
> > >
> > > Fix possible unaligned u32 access in b43_generate_plcp_hdr().
> > > Unaligned data is read/write with a u32 pointer instead of using the
> > > packed structure. Some versions of gcc ignore the "packed" attribute, if the
> > > structure element is accessed through a local pointer.
> > >
> > > Signed-off-by: Matthieu CASTET <[email protected]>
> > > Signed-off-by: Michael Buesch <[email protected]>
> >
> > That seems pretty brain-dead...can you cite a source for this
> > information? The patch seems like a no-op...
> >
> > John
>
> struct foo {
> int data;
> } __attribute__((packed));
>
> struct foo foo;
> int *d = &foo->data;
> foo->data = x; /* Works for unaligned */
> *d = y; /* Does not work for unaligned */
Why not?
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Friday 05 June 2009 14:25:03 John W. Linville wrote:
> On Thu, Jun 04, 2009 at 11:18:33PM +0200, Michael Buesch wrote:
> > From: Matthieu CASTET <[email protected]>
> >
> > Fix possible unaligned u32 access in b43_generate_plcp_hdr().
> > Unaligned data is read/write with a u32 pointer instead of using the
> > packed structure. Some versions of gcc ignore the "packed" attribute, if the
> > structure element is accessed through a local pointer.
> >
> > Signed-off-by: Matthieu CASTET <[email protected]>
> > Signed-off-by: Michael Buesch <[email protected]>
>
> That seems pretty brain-dead...can you cite a source for this
> information? The patch seems like a no-op...
>
> John
struct foo {
int data;
} __attribute__((packed));
struct foo foo;
int *d = &foo->data;
foo->data = x; /* Works for unaligned */
*d = y; /* Does not work for unaligned */
--
Greetings, Michael.
On Thu, Jun 04, 2009 at 11:18:33PM +0200, Michael Buesch wrote:
> From: Matthieu CASTET <[email protected]>
>
> Fix possible unaligned u32 access in b43_generate_plcp_hdr().
> Unaligned data is read/write with a u32 pointer instead of using the
> packed structure. Some versions of gcc ignore the "packed" attribute, if the
> structure element is accessed through a local pointer.
>
> Signed-off-by: Matthieu CASTET <[email protected]>
> Signed-off-by: Michael Buesch <[email protected]>
That seems pretty brain-dead...can you cite a source for this
information? The patch seems like a no-op...
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Fri, Jun 05, 2009 at 02:53:07PM -0400, John W. Linville wrote:
> On Fri, Jun 05, 2009 at 05:03:57PM +0200, [email protected] wrote:
> > Quoting "John W. Linville" <[email protected]>:
> >
> > > On Thu, Jun 04, 2009 at 11:18:33PM +0200, Michael Buesch wrote:
> > > > From: Matthieu CASTET <[email protected]>
> > > >
> > > > Fix possible unaligned u32 access in b43_generate_plcp_hdr().
> > > > Unaligned data is read/write with a u32 pointer instead of using the
> > > > packed structure. Some versions of gcc ignore the "packed" attribute, if
> > > the
> > > > structure element is accessed through a local pointer.
> > > >
> > > > Signed-off-by: Matthieu CASTET <[email protected]>
> > > > Signed-off-by: Michael Buesch <[email protected]>
> > >
> > > That seems pretty brain-dead...can you cite a source for this
> > > information?
> > The test I did with the attached test case in the first post.
>
> Link?
>
> > I don't see why gcc should propagate packed structure info to assignment.
> > That will be impossible to handle (think of passing it to function parameter).
>
> Perhaps this is obvious to you, but it isn't to me.
OK, I got an explanation from Kyle McMartin that I grok...
> >
> > > The patch seems like a no-op...
> > At least the code produced on mips is different.
>
> Then why aren't you trying to get the mips gcc guys to fix it?
Still worth wondering...anyway, the patch is fine -- I just didn't
grok the explanation.
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.