Compute TCP checksum #29
Conversation
Construct the pseudoheader and ones complement that with the serialized packet (and a zero'd out checksum field).
|
Does the switch not compute this checksum for us? IIRC it will for the IP checksum. |
|
If the packet's modified on the controller, the switch won't recompute checksums. @mcanini and I ran into this issue while trying to setup an HTTP request bouncer for the demo. |
|
why is removing the checksum field from the equality test the correct fix? shouldn't it be equal when you round-trip unmarshalling and re-marshalling? |
|
to follow-up: arbitrary_tcp and arbitrary_udp should set the checksum field properly, now that the marshalling code is capable of doing the same. also, while the round-trip testing is clearly useful and has caught many bugs, it shows this approach has a weakness in testing checksum code. ideally, there should be some binary blobs with captured packets, which the unmarshall -> remarshall pipeline should now be able to match (huzzah!), including the checksums which were generated by a non-ocaml-openflow implementation. hope this makes sense. |
|
Odd enough I cannot find mention in the OpenFlow specs that setting the TCP or UDP src/dst port will cause the switch to compute a new checksum. Seems obvious that it should happen though. |
| @@ -883,7 +898,8 @@ module Ip = struct | |||
| let bits = Cstruct.shift bits header_len in | |||
| match pkt.tp with | |||
| | Tcp tcp -> | |||
| Tcp.marshal bits tcp | |||
| Tcp.marshal bits tcp; | |||
| Tcp.checksum bits pkt.src pkt.dst tcp | |||
| | Udp udp -> | |||
| Udp.marshal bits udp | |||
There was a problem hiding this comment.
The UDP checksum should also be computed
There was a problem hiding this comment.
@mcanini UDP checksum requires some more thought with respect to the interface, as the checksum is optional.
Arbitrary instances for TCP and UDP simply set it to zero. Since the marshal code recompute the checksum, the 0 value will not be preserved and the roundtrip property will fail. A simple way to get around this is to marshal and unmarshal a TCP or UDP packet once, which will set the chksum field to the proper value, and then check that the roundtrip property holds on that packet.
|
LGTM |
Construct the pseudoheader and ones complement that with the serialized packet (and a zero'd out checksum field).