lkml.org 
[lkml]   [2008]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: PATCH] firewire: add padding to some struct
    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


    \
     
     \ /
      Last update: 2008-07-20 08:25    [W:3.587 / U:0.660 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site