Re: PATCH] firewire: add padding to some struct

[Stefan Richter]

JiSheng Zhang wrote:
> On Fri, 18 Jul 2008 17:27:44 +0200
> Mikael Pettersson <mikpe@xxxxxxxx> wrote:
> > JiSheng Zhang writes:
> >  > --- old/drivers/firewire/fw-cdev.c	2008-07-14 05:51:29.000000000 +0800
> >  > +++ new/drivers/firewire/fw-cdev.c	2008-07-18 20:20:45.841328585 +0800
> >  > @@ -382,9 +382,9 @@
> >  >  
> >  >  	response->response.type   = FW_CDEV_EVENT_RESPONSE;
> >  >  	response->response.rcode  = rcode;
> >  > -	queue_event(client, &response->event,
> >  > -		    &response->response, sizeof(response->response),
> >  > -		    response->response.data, response->response.length);
> >  > +	queue_event(client, &response->event, &response->response, 
> >  > +		    sizeof(response->response) + response->response.length,
> >  > +		    NULL, 0);
> >  >  }
> >
> > Neither of these look correct.
> > If sizeof(struct ...) != offsetof(struct ..., data) as you claim is possible,
> > then the old code will copy too much to/from ->response but the correct amount
> > to/from ->response.data, and the new code will copy too much to/from ->response.data.
>
> The old code will copy 4 extra bytes totally on some platforms, the new code
> is correct. The old one queue like this:
> struct ...(excluding the padding bytes)|4 padding bytes|4 padding bytes|data
>
> > That's why C has offsetof():
> >
> > 	queue_event(client, &response->event,
> > 		   &response->response,
> > 		   offsetof(typeof(*response->responce), data), // I don't know the struct name
> > 		   response->response.data, response->response.length);

sizeof(struct ...) != offsetof(struct ..., data) happens for example on 
x86-64.

This is what I get with the current firewire drivers in a block read 
response from firecontrol on i686:

	Command: r . 0 0xfffff0000400 20
	reading from node 0, bus 1023, offset 0XFFFFF0000400 20 bytes
	read succeeded. Data follows (hex):
	04 04 04 8F
	31 33 39 34
	F0 00 A2 22
	00 10 DC 56
	00 FE D2 D4
	Ack code: complete

And the same on x86-64:

	Command: r . 0 0xfffff0000400 20
	reading from node 0, bus 1023, offset 0XFFFFF0000400 20 bytes
	read succeeded. Data follows (hex):
	04 04 04 8F
	04 04 04 8F
	31 33 39 34
	F0 00 A2 22
	00 10 DC 56
	Ack code: complete

	Command: r . 0 0xfffff0000400 24
	reading from node 0, bus 1023, offset 0XFFFFF0000400 20 bytes
	read succeeded. Data follows (hex):
	04 04 04 8F
	04 04 04 8F
	31 33 39 34
	F0 00 A2 22
	00 10 DC 56
	00 FE D2 D4
	Ack code: complete

I used libraw1394 from Dan's git repo.  Gscanbus shows exactly the same 
results.  So, x86-64 and all other architectures where struct 
fw_cdev_event* are aligned on u64 boundaries are currently seriously 
broken... but nobody noticed it before.  The only breakage which I saw 
(and is obviously the result of this bug) is that gscanbus shows a wrong 
bus topology on x86-64 but the correct one on i686.  The damage from 
this bug is limited though because
  - most applications send requests which get responses with 0 or 4
    bytes payload,
  - no application (which can run on both old and new stack without
    change) uses address range mappings, i.e. get incoming requests.

I'll look further into your proposed fix.
--
Stefan Richter
-=====-==--- -=== =--==
http://arcgraph.de/sr/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/