[PATCH] b43: Fix possible unaligned u32 access

[PATCH] b43: Fix possible unaligned u32 access

[Michael Buesch]

From: Matthieu CASTET <castet.matthieu@free.fr>

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 <castet.matthieu@free.fr>
Signed-off-by: Michael Buesch <mb@bu3sch.de>

---

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.

Re: [PATCH] b43: Fix possible unaligned u32 access

[John W. Linville]

On Thu, Jun 04, 2009 at 11:18:33PM +0200, Michael Buesch wrote:
> From: Matthieu CASTET <castet.matthieu@free.fr>
> 
> 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 <castet.matthieu@free.fr>
> Signed-off-by: Michael Buesch <mb@bu3sch.de>

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
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] b43: Fix possible unaligned u32 access

[castet.matthieu]

Quoting "John W. Linville" <linville@tuxdriver.com>:

> On Thu, Jun 04, 2009 at 11:18:33PM +0200, Michael Buesch wrote:
> > From: Matthieu CASTET <castet.matthieu@free.fr>
> >
> > 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 <castet.matthieu@free.fr>
> > Signed-off-by: Michael Buesch <mb@bu3sch.de>
>
> 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

Re: [PATCH] b43: Fix possible unaligned u32 access

[John W. Linville]

On Fri, Jun 05, 2009 at 05:03:57PM +0200, castet.matthieu@free.fr wrote:
> Quoting "John W. Linville" <linville@tuxdriver.com>:
> 
> > On Thu, Jun 04, 2009 at 11:18:33PM +0200, Michael Buesch wrote:
> > > From: Matthieu CASTET <castet.matthieu@free.fr>
> > >
> > > 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 <castet.matthieu@free.fr>
> > > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> >
> > 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
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] b43: Fix possible unaligned u32 access

[John W. Linville]

On Fri, Jun 05, 2009 at 02:53:07PM -0400, John W. Linville wrote:
> On Fri, Jun 05, 2009 at 05:03:57PM +0200, castet.matthieu@free.fr wrote:
> > Quoting "John W. Linville" <linville@tuxdriver.com>:
> > 
> > > On Thu, Jun 04, 2009 at 11:18:33PM +0200, Michael Buesch wrote:
> > > > From: Matthieu CASTET <castet.matthieu@free.fr>
> > > >
> > > > 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 <castet.matthieu@free.fr>
> > > > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> > >
> > > 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
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] b43: Fix possible unaligned u32 access

[Michael Buesch]

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 <castet.matthieu@free.fr>
> > 
> > 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 <castet.matthieu@free.fr>
> > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> 
> 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.

Re: [PATCH] b43: Fix possible unaligned u32 access

[John W. Linville]

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 <castet.matthieu@free.fr>
> > > 
> > > 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 <castet.matthieu@free.fr>
> > > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> > 
> > 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
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] b43: Fix possible unaligned u32 access

[Michael Buesch]

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 <castet.matthieu@free.fr>
> > > > 
> > > > 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 <castet.matthieu@free.fr>
> > > > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> > > 
> > > 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.

Re: [PATCH] b43: Fix possible unaligned u32 access

[David Miller]

From: "John W. Linville" <linville@tuxdriver.com>
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 <castet.matthieu@free.fr>
>> > > 
>> > > 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 <castet.matthieu@free.fr>
>> > > Signed-off-by: Michael Buesch <mb@bu3sch.de>
>> > 
>> > 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.

Re: [PATCH] b43: Fix possible unaligned u32 access

[David Miller]

From: "John W. Linville" <linville@tuxdriver.com>
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 <castet.matthieu@free.fr>
>> 
>> 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 <castet.matthieu@free.fr>
>> Signed-off-by: Michael Buesch <mb@bu3sch.de>
> 
> 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.