Re: PATCH] firewire: add padding to some struct

Re: PATCH] firewire: add padding to some struct

[JiSheng Zhang]

Hi,
If p is a pointer to struct fw_cdev_event_response), p->data will point to the
padding data rather than the right place, it will cause problem under some
platforms. For example, in the function handle_device_event of libraw1394(ported
to juju stack):
.....
case FW_CDEV_EVENT_RESPONSE:
    rc = u64_to_ptr(u->response.closure);
    if (rc->data != NULL)
	memcpy(rc->data, u->response.data, rc->length);//here it will lost the last four
bytes
    errcode = juju_to_raw1394_errcode(u->response.rcode);
.....

Although this problem can be solved by add the offset to the pointer, but the
member:__u32 data[0] lost its original meaning.

Thanks in advance,
JiSheng
>From: Stefan Richter <stefanr@s5r6.in-berlin.de>
>Reply-To: 
>To: JiSheng Zhang <jszhang3@mail.ustc.edu.cn>
>Subject: Re: PATCH] firewire: add padding to some struct
>Date:Fri, 18 Jul 2008 12:49:11 +0200
>
>JiSheng Zhang wrote:
> > struct fw_cdev_event_response and struct fw_cdev_event_iso_interrupt need
padding.
> > Otherwise, offset of the zero length array is not equal to the struct size. It
may
> > cause some strange problems under some platforms such as sparc32. This
> > patch(against 2.6.26) should fix it.
> 
> The best solution to this problem would be to use
> 
> 	offsetof(struct fw_cdev_event_XYZ, data)
> 
> instead of sizeof(struct fw_cdev_event_XYZ) in all the places where the
> offset is required.
> 
> Your proposed solution to add padding...
> 
> > --- old/include/linux/firewire-cdev.h	2008-07-18 16:34:01.181794046 +0800
> > +++ new/include/linux/firewire-cdev.h	2008-07-18 16:35:46.649294275 +0800
> > @@ -92,6 +92,7 @@
> >  	__u32 type;
> >  	__u32 rcode;
>

Re: PATCH] firewire: add padding to some struct

[Stefan Richter]

JiSheng Zhang wrote:
> If p is a pointer to struct fw_cdev_event_response), p->data will point to the
> padding data rather than the right place, it will cause problem under some
> platforms. For example, in the function handle_device_event of libraw1394(ported
> to juju stack):
> .....
> case FW_CDEV_EVENT_RESPONSE:
>     rc = u64_to_ptr(u->response.closure);
>     if (rc->data != NULL)
> 	memcpy(rc->data, u->response.data, rc->length);//here it will lost the last four
> bytes
>     errcode = juju_to_raw1394_errcode(u->response.rcode);
> .....
> 
> Although this problem can be solved by add the offset to the pointer, but the
> member:__u32 data[0] lost its original meaning.

I don't understand what the problem is.  As long as both kernel and
library use "response.data" or "&response + offsetof(typeof(response),
data)", they will write and read at the correct location.

There would be a problem if one of the two used "&response +
sizeof(response)" instead.  Does this happen anywhere?  If so, then
these places need to be fixed, not the struct definition.
-- 
Stefan Richter
-=====-==--- -=== =--=-
http://arcgraph.de/sr/

Re: PATCH] firewire: add padding to some struct

[Stefan Richter]

Stefan Richter wrote:
> I don't understand what the problem is.  As long as both kernel and
> library use "response.data" or "&response + offsetof(typeof(response),
> data)", they will write and read at the correct location.

PS, of course the math of the latter should look more like
	(char *)&response + offsetof(...)
-- 
Stefan Richter
-=====-==--- -=== =--=-
http://arcgraph.de/sr/

Re: PATCH] firewire: add padding to some struct

[JiSheng Zhang]

Hi,
>From: Stefan Richter <stefanr@s5r6.in-berlin.de>
>Reply-To: 
>To: JiSheng Zhang <jszhang3@mail.ustc.edu.cn>
>Subject: Re: PATCH] firewire: add padding to some struct
>Date:Fri, 18 Jul 2008 13:38:25 +0200
>
>JiSheng Zhang wrote:
> > If p is a pointer to struct fw_cdev_event_response), p->data will point to
the
> > padding data rather than the right place, it will cause problem under some
> > platforms. For example, in the function handle_device_event of
libraw1394(ported
> > to juju stack):
> > .....
> > case FW_CDEV_EVENT_RESPONSE:
> >     rc = u64_to_ptr(u->response.closure);
> >     if (rc->data != NULL)
> > 	memcpy(rc->data, u->response.data, rc->length);//here it will lost the last
four
> > bytes
> >     errcode = juju_to_raw1394_errcode(u->response.rcode);
> > .....
> > 
> > Although this problem can be solved by add the offset to the pointer, but the
> > member:__u32 data[0] lost its original meaning.
> 
> I don't understand what the problem is.  As long as both kernel and
> library use "response.data" or "&response + offsetof(typeof(response),
> data)", they will write and read at the correct location.
>
This patch can fix the problem while not changing the struct definition.


Thanks in advance,
JiSheng

--- 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);
 }
 
 static int ioctl_send_request(struct client *client, void *buffer)

Re: PATCH] firewire: add padding to some struct

[Mikael Pettersson]

JiSheng Zhang writes:
 > Hi,
 > >From: Stefan Richter <stefanr@s5r6.in-berlin.de>
 > >Reply-To: 
 > >To: JiSheng Zhang <jszhang3@mail.ustc.edu.cn>
 > >Subject: Re: PATCH] firewire: add padding to some struct
 > >Date:Fri, 18 Jul 2008 13:38:25 +0200
 > >
 > >JiSheng Zhang wrote:
 > > > If p is a pointer to struct fw_cdev_event_response), p->data will point to
 > the
 > > > padding data rather than the right place, it will cause problem under some

Define "the right place". If p->data[] isn't the place for the data,
then something's seriously wrong with either the producer or the
consumer of that data -- or the data type definition if either is HW.

 > > > platforms. For example, in the function handle_device_event of
 > libraw1394(ported
 > > > to juju stack):
 > > > .....
 > > > case FW_CDEV_EVENT_RESPONSE:
 > > >     rc = u64_to_ptr(u->response.closure);
 > > >     if (rc->data != NULL)
 > > > 	memcpy(rc->data, u->response.data, rc->length);//here it will lost the last
 > four
 > > > bytes
 > > >     errcode = juju_to_raw1394_errcode(u->response.rcode);
 > > > .....
 > > > 
 > > > Although this problem can be solved by add the offset to the pointer, but the
 > > > member:__u32 data[0] lost its original meaning.
 > > 
 > > I don't understand what the problem is.  As long as both kernel and
 > > library use "response.data" or "&response + offsetof(typeof(response),
 > > data)", they will write and read at the correct location.
 > >
 > This patch can fix the problem while not changing the struct definition.
 > 
 > 
 > Thanks in advance,
 > JiSheng
 > 
 > --- 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.

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);

Re: PATCH] firewire: add padding to some struct

[JiSheng Zhang]

On Fri, 18 Jul 2008 17:27:44 +0200
Mikael Pettersson <mikpe@it.uu.se> wrote:

> JiSheng Zhang writes:
>  > Hi,
>  > >From: Stefan Richter <stefanr@s5r6.in-berlin.de>
>  > >Reply-To: 
>  > >To: JiSheng Zhang <jszhang3@mail.ustc.edu.cn>
>  > >Subject: Re: PATCH] firewire: add padding to some struct
>  > >Date:Fri, 18 Jul 2008 13:38:25 +0200
>  > >
>  > >JiSheng Zhang wrote:
>  > > > If p is a pointer to struct fw_cdev_event_response), p->data will point to
>  > the
>  > > > padding data rather than the right place, it will cause problem under some
> 
> Define "the right place". If p->data[] isn't the place for the data,
> then something's seriously wrong with either the producer or the
> consumer of that data -- or the data type definition if either is HW.
> 
>  > > > platforms. For example, in the function handle_device_event of
>  > libraw1394(ported
>  > > > to juju stack):
>  > > > .....
>  > > > case FW_CDEV_EVENT_RESPONSE:
>  > > >     rc = u64_to_ptr(u->response.closure);
>  > > >     if (rc->data != NULL)
>  > > > 	memcpy(rc->data, u->response.data, rc->length);//here it will lost the last
>  > four
>  > > > bytes
>  > > >     errcode = juju_to_raw1394_errcode(u->response.rcode);
>  > > > .....
>  > > > 
>  > > > Although this problem can be solved by add the offset to the pointer, but the
>  > > > member:__u32 data[0] lost its original meaning.
>  > > 
>  > > I don't understand what the problem is.  As long as both kernel and
>  > > library use "response.data" or "&response + offsetof(typeof(response),
>  > > data)", they will write and read at the correct location.
>  > >
>  > This patch can fix the problem while not changing the struct definition.
>  > 
>  > 
>  > Thanks in advance,
>  > JiSheng
>  > 
>  > --- 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);

Bye,
JiSheng

Re: PATCH] firewire: add padding to some struct

[JiSheng Zhang]

On Fri, 18 Jul 2008 17:27:44 +0200
Mikael Pettersson <mikpe@it.uu.se> wrote:

> JiSheng Zhang writes:
>  > Hi,
>  > >From: Stefan Richter <stefanr@s5r6.in-berlin.de>
>  > >Reply-To: 
>  > >To: JiSheng Zhang <jszhang3@mail.ustc.edu.cn>
>  > >Subject: Re: PATCH] firewire: add padding to some struct
>  > >Date:Fri, 18 Jul 2008 13:38:25 +0200
>  > >
>  > >JiSheng Zhang wrote:
>  > > > If p is a pointer to struct fw_cdev_event_response), p->data will point to
>  > the
>  > > > padding data rather than the right place, it will cause problem under some
> 
> Define "the right place". If p->data[] isn't the place for the data,
> then something's seriously wrong with either the producer or the
> consumer of that data -- or the data type definition if either is HW.
> 
>  > > > platforms. For example, in the function handle_device_event of
>  > libraw1394(ported
>  > > > to juju stack):
>  > > > .....
>  > > > case FW_CDEV_EVENT_RESPONSE:
>  > > >     rc = u64_to_ptr(u->response.closure);
>  > > >     if (rc->data != NULL)
>  > > > 	memcpy(rc->data, u->response.data, rc->length);//here it will lost the last
>  > four
>  > > > bytes
>  > > >     errcode = juju_to_raw1394_errcode(u->response.rcode);
>  > > > .....
>  > > > 
>  > > > Although this problem can be solved by add the offset to the pointer, but the
>  > > > member:__u32 data[0] lost its original meaning.
>  > > 
>  > > I don't understand what the problem is.  As long as both kernel and
>  > > library use "response.data" or "&response + offsetof(typeof(response),
>  > > data)", they will write and read at the correct location.
>  > >
>  > This patch can fix the problem while not changing the struct definition.
>  > 
>  > 
>  > Thanks in advance,
>  > JiSheng
>  > 
>  > --- 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);

Bye,
JiSheng

Re: PATCH] firewire: add padding to some struct

[Mikael Pettersson]

JiSheng Zhang writes:
 > On Fri, 18 Jul 2008 17:27:44 +0200
 > Mikael Pettersson <mikpe@it.uu.se> wrote:
 > 
 > > JiSheng Zhang writes:
 > >  > Hi,
 > >  > >From: Stefan Richter <stefanr@s5r6.in-berlin.de>
 > >  > >Reply-To: 
 > >  > >To: JiSheng Zhang <jszhang3@mail.ustc.edu.cn>
 > >  > >Subject: Re: PATCH] firewire: add padding to some struct
 > >  > >Date:Fri, 18 Jul 2008 13:38:25 +0200
 > >  > >
 > >  > >JiSheng Zhang wrote:
 > >  > > > If p is a pointer to struct fw_cdev_event_response), p->data will point to
 > >  > the
 > >  > > > padding data rather than the right place, it will cause problem under some
 > > 
 > > Define "the right place". If p->data[] isn't the place for the data,
 > > then something's seriously wrong with either the producer or the
 > > consumer of that data -- or the data type definition if either is HW.
 > > 
 > >  > > > platforms. For example, in the function handle_device_event of
 > >  > libraw1394(ported
 > >  > > > to juju stack):
 > >  > > > .....
 > >  > > > case FW_CDEV_EVENT_RESPONSE:
 > >  > > >     rc = u64_to_ptr(u->response.closure);
 > >  > > >     if (rc->data != NULL)
 > >  > > > 	memcpy(rc->data, u->response.data, rc->length);//here it will lost the last
 > >  > four
 > >  > > > bytes
 > >  > > >     errcode = juju_to_raw1394_errcode(u->response.rcode);
 > >  > > > .....
 > >  > > > 
 > >  > > > Although this problem can be solved by add the offset to the pointer, but the
 > >  > > > member:__u32 data[0] lost its original meaning.
 > >  > > 
 > >  > > I don't understand what the problem is.  As long as both kernel and
 > >  > > library use "response.data" or "&response + offsetof(typeof(response),
 > >  > > data)", they will write and read at the correct location.
 > >  > >
 > >  > This patch can fix the problem while not changing the struct definition.
 > >  > 
 > >  > 
 > >  > Thanks in advance,
 > >  > JiSheng
 > >  > 
 > >  > --- 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 new code is only correct if the variable length payload starts
after the struct, i.e. (void*)(&response->response + 1), and not at
the data field, i.e. (void*)response->response.data.
Which is it? That's why I asked:

 > > Define "the right place". If p->data[] isn't the place for the data,
 > > then something's seriously wrong with either the producer or the
 > > consumer of that data -- or the data type definition if either is HW.

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@it.uu.se> 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/

Re: PATCH] firewire: add padding to some struct

[JiSheng Zhang]

On Sat, 19 Jul 2008 12:32:35 +0200
Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> JiSheng Zhang wrote:
> > On Fri, 18 Jul 2008 17:27:44 +0200
> > Mikael Pettersson <mikpe@it.uu.se> 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
this is the 4 extra padding bytes
> 	04 04 04 8F
> 	31 33 39 34
> 	F0 00 A2 22
> 	00 10 DC 56
here lost the last 4 bytes of data
> 	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 
the read_topology_map in the testlibraw of libraw1394(with support to juju)
will show similar breakage. 
> (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,
I think so.
>    - 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.
Thanks

Regards,
JiSheng

Re: PATCH] firewire: add padding to some struct

[JiSheng Zhang]

On Sat, 19 Jul 2008 12:32:35 +0200
Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> JiSheng Zhang wrote:
> > On Fri, 18 Jul 2008 17:27:44 +0200
> > Mikael Pettersson <mikpe@it.uu.se> 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
this is the 4 extra padding bytes
> 	04 04 04 8F
> 	31 33 39 34
> 	F0 00 A2 22
> 	00 10 DC 56
here lost the last 4 bytes of data
> 	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 
the read_topology_map in the testlibraw of libraw1394(with support to juju)
will show similar breakage. 
> (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,
I think so.
>    - 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.
Thanks

Regards,
JiSheng

Re: PATCH] firewire: add padding to some struct

[JiSheng Zhang]

On Sat, 19 Jul 2008 12:09:18 +0200
Mikael Pettersson <mikpe@it.uu.se> wrote:

> JiSheng Zhang writes:
>  > On Fri, 18 Jul 2008 17:27:44 +0200
>  > Mikael Pettersson <mikpe@it.uu.se> wrote:
>  > 
>  > > JiSheng Zhang writes:
>  > >  > Hi,
>  > >  > >From: Stefan Richter <stefanr@s5r6.in-berlin.de>
>  > >  > >Reply-To: 
>  > >  > >To: JiSheng Zhang <jszhang3@mail.ustc.edu.cn>
>  > >  > >Subject: Re: PATCH] firewire: add padding to some struct
>  > >  > >Date:Fri, 18 Jul 2008 13:38:25 +0200
>  > >  > >
>  > >  > >JiSheng Zhang wrote:
>  > >  > > > If p is a pointer to struct fw_cdev_event_response), p->data will point to
>  > >  > the
>  > >  > > > padding data rather than the right place, it will cause problem under some
>  > > 
>  > > Define "the right place". If p->data[] isn't the place for the data,
>  > > then something's seriously wrong with either the producer or the
>  > > consumer of that data -- or the data type definition if either is HW.
>  > > 
>  > >  > > > platforms. For example, in the function handle_device_event of
>  > >  > libraw1394(ported
>  > >  > > > to juju stack):
>  > >  > > > .....
>  > >  > > > case FW_CDEV_EVENT_RESPONSE:
>  > >  > > >     rc = u64_to_ptr(u->response.closure);
>  > >  > > >     if (rc->data != NULL)
>  > >  > > > 	memcpy(rc->data, u->response.data, rc->length);//here it will lost the last
>  > >  > four
>  > >  > > > bytes
>  > >  > > >     errcode = juju_to_raw1394_errcode(u->response.rcode);
>  > >  > > > .....
>  > >  > > > 
>  > >  > > > Although this problem can be solved by add the offset to the pointer, but the
>  > >  > > > member:__u32 data[0] lost its original meaning.
>  > >  > > 
>  > >  > > I don't understand what the problem is.  As long as both kernel and
>  > >  > > library use "response.data" or "&response + offsetof(typeof(response),
>  > >  > > data)", they will write and read at the correct location.
>  > >  > >
>  > >  > This patch can fix the problem while not changing the struct definition.
>  > >  > 
>  > >  > 
>  > >  > Thanks in advance,
>  > >  > JiSheng
>  > >  > 
>  > >  > --- 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 new code is only correct if the variable length payload starts
> after the struct, i.e. (void*)(&response->response + 1), and not at
> the data field, i.e. (void*)response->response.data.
> Which is it? That's why I asked:
Yes, the payload starts at the data field(including the padding bytes). But
there is no problem, "As long as both kernel and library use response.data"
as Stefan said.
> 
>  > > Define "the right place". If p->data[] isn't the place for the data,
>  > > then something's seriously wrong with either the producer or the
>  > > consumer of that data -- or the data type definition if either is HW.

Re: PATCH] firewire: add padding to some struct

[JiSheng Zhang]

On Sat, 19 Jul 2008 12:09:18 +0200
Mikael Pettersson <mikpe@it.uu.se> wrote:

> JiSheng Zhang writes:
>  > On Fri, 18 Jul 2008 17:27:44 +0200
>  > Mikael Pettersson <mikpe@it.uu.se> wrote:
>  > 
>  > > JiSheng Zhang writes:
>  > >  > Hi,
>  > >  > >From: Stefan Richter <stefanr@s5r6.in-berlin.de>
>  > >  > >Reply-To: 
>  > >  > >To: JiSheng Zhang <jszhang3@mail.ustc.edu.cn>
>  > >  > >Subject: Re: PATCH] firewire: add padding to some struct
>  > >  > >Date:Fri, 18 Jul 2008 13:38:25 +0200
>  > >  > >
>  > >  > >JiSheng Zhang wrote:
>  > >  > > > If p is a pointer to struct fw_cdev_event_response), p->data will point to
>  > >  > the
>  > >  > > > padding data rather than the right place, it will cause problem under some
>  > > 
>  > > Define "the right place". If p->data[] isn't the place for the data,
>  > > then something's seriously wrong with either the producer or the
>  > > consumer of that data -- or the data type definition if either is HW.
>  > > 
>  > >  > > > platforms. For example, in the function handle_device_event of
>  > >  > libraw1394(ported
>  > >  > > > to juju stack):
>  > >  > > > .....
>  > >  > > > case FW_CDEV_EVENT_RESPONSE:
>  > >  > > >     rc = u64_to_ptr(u->response.closure);
>  > >  > > >     if (rc->data != NULL)
>  > >  > > > 	memcpy(rc->data, u->response.data, rc->length);//here it will lost the last
>  > >  > four
>  > >  > > > bytes
>  > >  > > >     errcode = juju_to_raw1394_errcode(u->response.rcode);
>  > >  > > > .....
>  > >  > > > 
>  > >  > > > Although this problem can be solved by add the offset to the pointer, but the
>  > >  > > > member:__u32 data[0] lost its original meaning.
>  > >  > > 
>  > >  > > I don't understand what the problem is.  As long as both kernel and
>  > >  > > library use "response.data" or "&response + offsetof(typeof(response),
>  > >  > > data)", they will write and read at the correct location.
>  > >  > >
>  > >  > This patch can fix the problem while not changing the struct definition.
>  > >  > 
>  > >  > 
>  > >  > Thanks in advance,
>  > >  > JiSheng
>  > >  > 
>  > >  > --- 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 new code is only correct if the variable length payload starts
> after the struct, i.e. (void*)(&response->response + 1), and not at
> the data field, i.e. (void*)response->response.data.
> Which is it? That's why I asked:
Yes, the payload starts at the data field(including the padding bytes). But
there is no problem, "As long as both kernel and library use response.data"
as Stefan said.
> 
>  > > Define "the right place". If p->data[] isn't the place for the data,
>  > > then something's seriously wrong with either the producer or the
>  > > consumer of that data -- or the data type definition if either is HW.

Re: PATCH] firewire: add padding to some struct

[JiSheng Zhang]

Hi,

>From: Stefan Richter <stefanr@s5r6.in-berlin.de>
>Reply-To: 
>To: JiSheng Zhang <jszhang3@mail.ustc.edu.cn>
>Subject: Re: PATCH] firewire: add padding to some struct
>Date:Fri, 18 Jul 2008 13:38:25 +0200
>
>JiSheng Zhang wrote:
> > If p is a pointer to struct fw_cdev_event_response), p->data will point to
the
> > padding data rather than the right place, it will cause problem under some
> > platforms. For example, in the function handle_device_event of
libraw1394(ported
> > to juju stack):
> > .....
> > case FW_CDEV_EVENT_RESPONSE:
> >     rc = u64_to_ptr(u->response.closure);
> >     if (rc->data != NULL)
> > 	memcpy(rc->data, u->response.data, rc->length);//here it will lost the last
four
> > bytes
> >     errcode = juju_to_raw1394_errcode(u->response.rcode);
> > .....
> > 
> > Although this problem can be solved by add the offset to the pointer, but the
> > member:__u32 data[0] lost its original meaning.
> 
> I don't understand what the problem is.  As long as both kernel and
> library use "response.data" or "&response + offsetof(typeof(response),
> data)", they will write and read at the correct location.
>
> There would be a problem if one of the two used "&response +
> sizeof(response)" instead.  Does this happen anywhere?  If so, then
> these places need to be fixed, not the struct definition.
yes, complete_transaction in fw-cdev.c, it queues the response and data.

how about adding __attribute__((packed)) to the two struct definition? It will not
break abi compatibility.

Thanks in advance,
JiSheng


--- old/include/linux/firewire-cdev.h	2008-07-18 16:34:01.181794046 +0800
+++ new/include/linux/firewire-cdev.h	2008-07-18 19:39:16.389293987 +0800
@@ -93,7 +93,7 @@ struct fw_cdev_event_response {
 	__u32 rcode;
 	__u32 length;
 	__u32 data[0];
-};
+} __attribute__((packed));
 
 /**
  * struct fw_cdev_event_request - Sent on incoming request to an address region
@@ -144,7 +144,7 @@ struct fw_cdev_event_iso_interrupt {
 	__u32 cycle;
 	__u32 header_length;
 	__u32 header[0];
-};
+} __attribute__((packed));
 
 /**
  * union fw_cdev_event - Convenience union of fw_cdev_event_ types

PATCH] firewire: add padding to some struct

[JiSheng Zhang]

>From JiSheng Zhang <jszhang3@mail.ustc.edu.cn>

struct fw_cdev_event_response and struct fw_cdev_event_iso_interrupt need padding.
Otherwise, offset of the zero length array is not equal to the struct size. It may
cause some strange problems under some platforms such as sparc32. This
patch(against 2.6.26) should fix it.


--- old/include/linux/firewire-cdev.h	2008-07-18 16:34:01.181794046 +0800
+++ new/include/linux/firewire-cdev.h	2008-07-18 16:35:46.649294275 +0800
@@ -92,6 +92,7 @@
 	__u32 type;
 	__u32 rcode;
 	__u32 length;
+	__u32 pad;
 	__u32 data[0];
 };
 
@@ -143,6 +144,7 @@
 	__u32 type;
 	__u32 cycle;
 	__u32 header_length;
+	__u32 pad;
 	__u32 header[0];
 };
 

Re: PATCH] firewire: add padding to some struct

[Stefan Richter]

JiSheng Zhang wrote:
> struct fw_cdev_event_response and struct fw_cdev_event_iso_interrupt need padding.
> Otherwise, offset of the zero length array is not equal to the struct size. It may
> cause some strange problems under some platforms such as sparc32. This
> patch(against 2.6.26) should fix it.

The best solution to this problem would be to use

	offsetof(struct fw_cdev_event_XYZ, data)

instead of sizeof(struct fw_cdev_event_XYZ) in all the places where the
offset is required.

Your proposed solution to add padding...

> --- old/include/linux/firewire-cdev.h	2008-07-18 16:34:01.181794046 +0800
> +++ new/include/linux/firewire-cdev.h	2008-07-18 16:35:46.649294275 +0800
> @@ -92,6 +92,7 @@
>  	__u32 type;
>  	__u32 rcode;
>  	__u32 length;
> +	__u32 pad;
>  	__u32 data[0];
>  };
>  
> @@ -143,6 +144,7 @@
>  	__u32 type;
>  	__u32 cycle;
>  	__u32 header_length;
> +	__u32 pad;
>  	__u32 header[0];
>  };
>  

...can alas not be applied because it would break ABI compatibility.

Thanks for looking into it,
-- 
Stefan Richter
-=====-==--- -=== =--=-
http://arcgraph.de/sr/