Re: R: [lwip-users] ip_reass

[Ed Sutter]

address@hidden wrote:
> Kieran Mansley wrote:
> > On Fri, 2008-10-10 at 07:21 -0400, Ed Sutter wrote:
> >  
> > > Ok, I am running with the BF537, I'll take a look at that setting.
> > > However, it still seems to me that this is a problem in the code
> > > because the combination of...
> > >
> > >    1. MEM_ALIGNMENT = 4
> > >    2. pbuf_alloc() setting payload buffers up to be aligned based on 
> > > MEM_ALIGNMENT
> > >    3. the increment of the payload pointer by 14 in ethernet_input()
> > > and...
> > >    4. the overlay of the "helper" structure in 
> > > ip_reass_chain_frag_into_datagram_and_validate()
> > >
> > > will cause the misaligned access.  If the option you mention fixes it 
> > > for my
> > > situation on the BF537, that doesn't resolve it for other systems.
> > > Right?
> > >     
> First of all, Ed, I think you're right on this, it seems like a bug to me.
> > Including a 2-byte offset at the start of an ethernet frame in order to
> > align the IP and TCP headers is pretty common, so it should work for
> > other systems too.
> It doesn't, in case the ethernet DMA engine needs aligned source (TX) or 
> destination (RX) memory!
> >   Documenting this need in the above circumstance
> > would be handy.  As would making a check that the alignment requirements
> > of the overlaid helper structure are met.  I'm surprised that we got
> > that far though
> That would be because when writing the code, I tested it only on an x86, 
> which isn't very helpful in detecting these kind of errors :-(
> >  - you would think that accesses to the IP header would
> > have had the same problem in your case.
> >   
> The protocol headers are packed, which hides/solves this problem.
> I'll add a bug entry. The possible fix will be packing the helper 
> struct, I guess, since it has to fit inside the IP header - 
> MEM_ALIGNMENT can make it too big.
>
> Oh, Ed: thanks for finding this.
Glad to help...
The way I fixed this was to change the helper structure to this...

struct ip_reass_helper {
  PACK_STRUCT_FIELD(u16_t start);
  PACK_STRUCT_FIELD(struct pbuf *next_pbuf);
  PACK_STRUCT_FIELD(u16_t end);
};

Please let me know if you take a different approach.
Ed