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]
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
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]
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]
