Probable Bug in tcp.h
Hello, I probably found a bug in declaration of "struct tcphdr"! struct tcphdr { u_short th_sport; /* source port */ u_short th_dport; /* destination port */ tcp_seq th_seq; /* sequence number */ tcp_seq th_ack; /* acknowledgement number */ #if BYTE_ORDER == LITTLE_ENDIAN u_int th_x2:4,/* (unused) */ <---here th_off:4; /* data offset */ <--- #endif #if BYTE_ORDER == BIG_ENDIAN u_int th_off:4, /* data offset */ th_x2:4;/* (unused) */ #endif u_char th_flags; First of all I have the problam of misalignment of th_off. Because in this way always 4 bytes are read and the the bits of th_off are replaced. Then the 4 bytes are written back. But should (th_x and th_off) not only be 1 byte in whole -> only read and write 1 byte? I think if this was changed, my misalignment problems would go away! I'll appreciate any thoughts on this! Regards, Marc P.S.: Please cc me because I'm not on the list! ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Probable Bug in tcp.h
On Thu, Jun 05, 2008 at 05:12:47PM +0200, =?ISO-8859-1?Q?Marc_L=F6rner_ wrote: > Hello, > I probably found a bug in declaration of "struct tcphdr"! > > struct tcphdr { > u_short th_sport; /* source port */ > u_short th_dport; /* destination port */ > tcp_seq th_seq; /* sequence number */ > tcp_seq th_ack; /* acknowledgement number */ > #if BYTE_ORDER == LITTLE_ENDIAN > u_int th_x2:4,/* (unused) */ > <---here > th_off:4; /* data offset */ > <--- > #endif > #if BYTE_ORDER == BIG_ENDIAN > u_int th_off:4, /* data offset */ > th_x2:4;/* (unused) */ > #endif > u_char th_flags; > > First of all I have the problam of misalignment of th_off. Because in this > way > always 4 bytes are read and the the bits of th_off are replaced. Then the 4 > bytes are written back. > > But should (th_x and th_off) not only be 1 byte in whole -> only read and > write 1 byte? > > I think if this was changed, my misalignment problems would go away! I'm not sure what you mean. Please supply more information, like: 1) Are you running on little endian? Or big endian? 2) th_x2 + th_off are 1 byte in size. What do you mean? 3) What is exactly the effect? I don't understand the "replaced", "written back" etc. sentence. Regards, -- Rui Paulo ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Probable Bug in tcp.h
Marc Lörner wrote: .. First of all I have the problam of misalignment of th_off. Because in this way always 4 bytes are read and the the bits of th_off are replaced. Then the 4 bytes are written back. But should (th_x and th_off) not only be 1 byte in whole -> only read and write 1 byte? Which machine architecture are you attempting to compile this code on? On FreeBSD Tier 1 platforms, the access is probably going to come out of L2 cache anyway, so the fields in question will be read by a burst cycle. It is worth noting that NetBSD changed the base type of tcphdr's bitfields to uint8_t, however this shuffles the compiler dependency into the treatment of the "char" type. Most modern C compilers support "unsigned char". ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Probable Bug in tcp.h
On Thursday 05 June 2008 17:56, Rui Paulo wrote: > On Thu, Jun 05, 2008 at 05:12:47PM +0200, =?ISO-8859-1?Q?Marc_L=F6rner_ wrote: > > Hello, > > I probably found a bug in declaration of "struct tcphdr"! > > > > struct tcphdr { > > u_short th_sport; /* source port */ > > u_short th_dport; /* destination port */ > > tcp_seq th_seq; /* sequence number */ > > tcp_seq th_ack; /* acknowledgement number */ > > #if BYTE_ORDER == LITTLE_ENDIAN > > u_int th_x2:4,/* (unused) */ > > <---here > > th_off:4; /* data offset */ > > <--- > > #endif > > #if BYTE_ORDER == BIG_ENDIAN > > u_int th_off:4, /* data offset */ > > th_x2:4;/* (unused) */ > > #endif > > u_char th_flags; > > > > First of all I have the problam of misalignment of th_off. Because in > > this way always 4 bytes are read and the the bits of th_off are replaced. > > Then the 4 bytes are written back. > > > > But should (th_x and th_off) not only be 1 byte in whole -> only read and > > write 1 byte? > > > > I think if this was changed, my misalignment problems would go away! > > I'm not sure what you mean. > > Please supply more information, like: > 1) Are you running on little endian? Or big endian? I'm on itanium-architecture, therefore I can run big and little endian. But for now it is little endian. > 2) th_x2 + th_off are 1 byte in size. What do you mean? th_x2 and th_off are created as a bitfield. But C-Standard says that bitfields are accessed as integers => 4-bytes On itanium integers are read with ld4-command but the address of th_x2/th_off may not be aligned to 4-bytes => we get an unaligned reference fault. If we'd change to 1 byte-accesses => I won't get any misaligned faults anymore. Hope this makes my dilemma a bit clearer, Marc ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Probable Bug in tcp.h
On Thursday 05 June 2008 18:09, Bruce M. Simpson wrote: > Marc Lörner wrote: > > .. > > First of all I have the problam of misalignment of th_off. Because in > > this way always 4 bytes are read and the the bits of th_off are replaced. > > Then the 4 bytes are written back. > > > > But should (th_x and th_off) not only be 1 byte in whole -> only read and > > write 1 byte? > > Which machine architecture are you attempting to compile this code on? > ia64/itanium > On FreeBSD Tier 1 platforms, the access is probably going to come out of > L2 cache anyway, so the fields in question will be read by a burst cycle. > I know! But the problem (on itanium) is that bitfields are always accessed as integers => 4-byte access But th_x/th_off may not always be aligned to 4-bytes => I get an unalignment reference fault If access of th_x/th_off could be changed to 1-byte => 1-byte aligned => my unaligned reference fault would go away > It is worth noting that NetBSD changed the base type of tcphdr's > bitfields to uint8_t, however this shuffles the compiler dependency into > the treatment of the "char" type. Most modern C compilers support > "unsigned char". Does this really change the access to 1-byte? As in "Programming in C" by Kernighan and Ritchie is stated that bitfields must and will always be defined as ints. ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Probable Bug in tcp.h
Marc Lörner wrote: th_x2 and th_off are created as a bitfield. But C-Standard says that bitfields are accessed as integers => 4-bytes On itanium integers are read with ld4-command but the address of th_x2/th_off may not be aligned to 4-bytes => we get an unaligned reference fault. If we'd change to 1 byte-accesses => I won't get any misaligned faults anymore. It's worth noting that Linux implements its version of tcphdr using a 32-bit-wide bitfield and the TCP header flags live there as bits instead of as integer quantities. I think it should be OK to change the u_int to a uint8_t as NetBSD has. The problem with bitfields in "signed char" is that they can become unintentionally sign extended on a read, and for many years compilers only supported "char", not "unsigned char". Does anyone see a reason why we should not make this change? ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Probable Bug in tcp.h
On 2008-Jun-06 09:30:28 +0200, Marc Lörner <[EMAIL PROTECTED]> wrote: >th_x2 and th_off are created as a bitfield. But C-Standard says that >bitfields are accessed as integers => 4-bytes > >On itanium integers are read with ld4-command but the address of >th_x2/th_off may not be aligned to 4-bytes => we get an unaligned >reference fault. If the C compiler chooses to implement bitfields as a subset of a 32-bit integers, it is up to it to load an aligned 32-bit integer and shift/mask the result appropriately to extract the fields. In this particular case, th_x2/th_off are immediately preceeded by a tcp_seq (u_int32_t) field and so will have 32-bit alignment. Note that the presence of 32-bit fields in the definition for struct tcphdr means that the struct must be aligned to at least 32 bits. >If we'd change to 1 byte-accesses => I won't get any misaligned faults >anymore. I gather from this comment that you have some code using struct tcphdr that is getting alignment errors. struct tcphdr is extensively used in the TCP stack within the kernel so it's likely that any layout or alignment problem with it would show up there. I suspect you are dereferencing a mis-aligned struct tcphdr. -- Peter Jeremy Please excuse any delays as the result of my ISP's inability to implement an MTA that is either RFC2821-compliant or matches their claimed behaviour. pgp5pk6y5YJfo.pgp Description: PGP signature
Re: Probable Bug in tcp.h
On Friday 06 June 2008 09:52, Peter Jeremy wrote: > On 2008-Jun-06 09:30:28 +0200, Marc Lörner <[EMAIL PROTECTED]> wrote: > >th_x2 and th_off are created as a bitfield. But C-Standard says that > >bitfields are accessed as integers => 4-bytes > > > >On itanium integers are read with ld4-command but the address of > >th_x2/th_off may not be aligned to 4-bytes => we get an unaligned > >reference fault. > > If the C compiler chooses to implement bitfields as a subset of a > 32-bit integers, it is up to it to load an aligned 32-bit integer > and shift/mask the result appropriately to extract the fields. > > In this particular case, th_x2/th_off are immediately preceeded by > a tcp_seq (u_int32_t) field and so will have 32-bit alignment. Note > that the presence of 32-bit fields in the definition for struct tcphdr > means that the struct must be aligned to at least 32 bits. > > >If we'd change to 1 byte-accesses => I won't get any misaligned faults > >anymore. > > I gather from this comment that you have some code using struct tcphdr > that is getting alignment errors. struct tcphdr is extensively used > in the TCP stack within the kernel so it's likely that any layout or > alignment problem with it would show up there. I suspect you are > dereferencing a mis-aligned struct tcphdr. The funny thing is that the dereferencing occurs in "/usr/src/sys/netinet/tcp_input.c" in function tcp_input in line 550: /* * Check that TCP offset makes sense, * pull out TCP options and adjust length. XXX */ off = th->th_off << 2; <- here if (off < sizeof (struct tcphdr) || off > tlen) { tcpstat.tcps_rcvbadoff++; goto drop; } So the misalignment may probably lie in TCP stack? ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Probable Bug in tcp.h
On Thu, 5 Jun 2008, Bruce M. Simpson wrote: Marc L?rner wrote: .. First of all I have the problam of misalignment of th_off. Because in this way always 4 bytes are read and the the bits of th_off are replaced. Then the 4 bytes are written back. But should (th_x and th_off) not only be 1 byte in whole -> only read and write 1 byte? 1 or 2, since struct tcphdr must be 16-bit aligned due to the shorts in it. An unportability in gcc's treatment of bit-fields results in gcc thinking that the struct is 4-byte aligned. Thus alignment traps may occur if an instance of the struct is not in fact aligned. Which machine architecture are you attempting to compile this code on? [ia64]. On FreeBSD Tier 1 platforms, the access is probably going to come out of L2 cache anyway, so the fields in question will be read by a burst cycle. It is worth noting that NetBSD changed the base type of tcphdr's bitfields to uint8_t, however this shuffles the compiler dependency into the treatment of the "char" type. Most modern C compilers support "unsigned char". No C compilers support bit-fields of type uint8_t (unless _Bool is uint8_t), since C requires bit-fields to have type a possibly-qualified version of _Bool, signed int or insigned int. Howver, the behaviour for other types is unspecified, so the compiler can do anything with them including what broken software wants, so it is really no C programs that use bit-fields of type uint8_t. Bit-fields of type larger than u_int are useful for holding more bits than a u_int (since C unfortunately limits the number of bits in a bit-field to the number in the type of the bit-field, so you can't write u_int foo:64 to get a 64-bit integer), but bit-fields of type smaller than u_int are not very useful -- compilers are free to treat them the same as bit fields of larger type. gcc's actual treatment of bit-fields of type smaller than u_int is unportable and apparently undocumented. It affects alignment and the struct size but not padding: e.g., in the following struct: struct { unsigned x:1; char y; } foo; This allocates 1 bit for x and pads to a byte boundary, so that y has offset 1. Any more padding would be bogus. Then, bogusly, due to the bit-field having type unsigned, gcc makes the whole struct have alignment requirements that of u_int and pads the struct at the end to have size a multiple of sizeof(u_int) = 32 bits. Then it is justified as accessing foo.x as part of an an aligned object of size 32 bits, and may trap if foo.x is not actually aligned. Something like this happens for struct tcphdr, with the alignment trap actually ocurring on ia64. Some bug elsewhere is responsible for generating a misaligned struct. OTOH, if `unsigned' in the above is changed to `unsigned char', giving a non-C program: struct { unsigned char x:1; char y; } foo; then all the padding is non-bogus when this is compiled by the non-C compiler gcc: the offset of y is unchanged, but now the struct has size 2 and alignment 1. Due to unportabilities like this, bit-fields should never be used when the layout of an object must be controlled precisely. This includes layouts related to hardware which includes most network layouts. Unportabilities like this are sometimes hacked around using packed structs. ipv6 headers do this a lot. ipv4 headers are not so bad. ip.h now says `__packed __aligned(4)' for struct ip where it used to say just `__packed' __packed forces 1-byte alignment for everything. On ia64, this results in specially pessimized accesses for all the fields in a struct, with 1- byte accesses even for the 32-bit fields and large code to combine the bytes into a word. But struct ip (unlike struct tcphdr?) must always be 32-bit aligned, and declaring it as __aligned(4) is supposed to restore the possibility of wide accesses after declaration it as __packed forces it to have no padding. No padding should occur automatically, and the u_int bit-fields don't affect this since the size of struct ip is 20, but there is a another problem with bogus padding at the end (on arm only?). Bruce___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Probable Bug in tcp.h
On Fri, 6 Jun 2008, Marc [iso-8859-1] L?rner wrote: On Friday 06 June 2008 09:52, Peter Jeremy wrote: I gather from this comment that you have some code using struct tcphdr that is getting alignment errors. struct tcphdr is extensively used in the TCP stack within the kernel so it's likely that any layout or alignment problem with it would show up there. I suspect you are dereferencing a mis-aligned struct tcphdr. The funny thing is that the dereferencing occurs in "/usr/src/sys/netinet/tcp_input.c" in function tcp_input in line 550: /* * Check that TCP offset makes sense, * pull out TCP options and adjust length. XXX */ off = th->th_off << 2; <- here if (off < sizeof (struct tcphdr) || off > tlen) { tcpstat.tcps_rcvbadoff++; goto drop; } So the misalignment may probably lie in TCP stack? Quite likely. th is normally at offset off0 in ip, where ip is required to be 32-bit aligned (see my previous reply). You can see off0 in a stack trace. Bruce___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Probable Bug in tcp.h
On Friday 06 June 2008 14:25, Bruce Evans wrote: > On Fri, 6 Jun 2008, Marc [iso-8859-1] Lörner wrote: > > On Friday 06 June 2008 09:52, Peter Jeremy wrote: > >> I gather from this comment that you have some code using struct tcphdr > >> that is getting alignment errors. struct tcphdr is extensively used > >> in the TCP stack within the kernel so it's likely that any layout or > >> alignment problem with it would show up there. I suspect you are > >> dereferencing a mis-aligned struct tcphdr. > > > > The funny thing is that the dereferencing occurs in > > "/usr/src/sys/netinet/tcp_input.c" in function tcp_input in line 550: > > > > /* > > * Check that TCP offset makes sense, > > * pull out TCP options and adjust length. XXX > > */ > > off = th->th_off << 2; > > <- here > > if (off < sizeof (struct tcphdr) || off > tlen) { > > tcpstat.tcps_rcvbadoff++; > > goto drop; > > } > > > > So the misalignment may probably lie in TCP stack? > > Quite likely. th is normally at offset off0 in ip, where ip is required > to be 32-bit aligned (see my previous reply). You can see off0 in a > stack trace. > off0 is 0x14 => no problem with that but address of ip is 0xe00021c8706e => not correct aligned to 32-bits Can anyone tell me, where ip is allocated, so I can do a little bit more research? Marc ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Probable Bug in tcp.h
Marc Lörner wrote: off0 is 0x14 => no problem with that but address of ip is 0xe00021c8706e => not correct aligned to 32-bits Can anyone tell me, where ip is allocated, so I can do a little bit more research? It really depends on the context! That's a very wide ranging question. It depends upon whether mbuf chains are flowing up or down the stack, whether or not the network driver supports checksum or header/segment offload, and whether or not it is using zero-copy. Zero copy transmit normally only has mmu cost if the mbuf (from userland) can be mapped to a location where headers are easily prepended. Zero copy receive is more expensive and complex as it requires that the DMA engine on the network interface card supports header splitting. The FreeBSD stack is known to have some issues with mbuf alignment and architectures other than those in its Tier 1. ___ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "[EMAIL PROTECTED]"