-
Notifications
You must be signed in to change notification settings - Fork 17.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
x/net/ipv4: add IPv4 header checksum computation for ipv4.Header type #25342
Comments
Seems fine to me for x/net. |
Change https://golang.org/cl/112817 mentions this issue: |
The CL 112817 provides wrong checksum values on a few BSD variants. I think the functionality should be provided as part of wire format friendly API in the other package, the same as #18633. |
@mikioh how can the checksum be OS-specific? What do you mean "on a few BSD variants"? |
Sorry, clicked "close and comment" instead of comment. |
The packages in x/net repository generate and parse byte sequences from/to host protocol stack implementations. As mentioned in #18633, the byte sequences may differ from the wire format on a few BSD variants. |
IPv4 defines a wire checksum. It sounds like some operating systems separately require user-space code to send the kernel a different checksum over IPv4 headers in certain contexts, and that different checksum varies from system to system. I can see the possible benefit in abstracting that way, but we should certainly not call this an IPv4 checksum. Any confusion with the portable computation should be avoided. It sounds like what's really being suggested is SystemHeaderChecksum. If it's a method on ipv4.Header maybe it's just SystemChecksum. Either way the doc comment must make clear this is an OS-specific calculation and not the standard IPv4 checksum. Do I have this right? |
Ping @mdlayher |
TBH, I wasn't even aware this was possible. I had assumed that all operating systems used the same algorithm: https://en.wikipedia.org/wiki/IPv4_header_checksum.
I was proposing adding the "standard" IPv4 checksum, per the above Wikipedia article. I don't quite understand how this could vary between OSes, but my experience is pretty much only Linux-based. |
My use case is as follows: https://gist.github.com/mdlayher/fe65443a0d82319fea1e2f2a01cade53 I'm using packet sockets on Linux (AF_PACKET) and crafting Ethernet frames, IPv4 headers, and UDP headers, manually for use in a work project. I built out the IPv4 checksum function based on the Wikipedia article linked above. I'm asking for what appears to be the "wire checksum" you are mentioning. |
@mdlayher, what is https://godoc.org/golang.org/x/net/ipv4#Header.Checksum today? It's woefully underdocumented if there actually exist two checksums (a system checksum vs a wire checksum?) as @mikioh says. If there's two, let's update the comments there first about what that one is. But if that field is already the wire checksum (for received packets only), what about a method that like |
That's the "wire checksum" I'd expect. Since my current code already has the header bytes, I'm just putting the computed checksum in that spot already. x/net/ipv4 is doing the same. // Compute the correct IPv4 checksum and place it in the correct
// offset in the marshaled header.
ip4Chk, err := ipv4Checksum(ip4Buf)
if err != nil {
return nil, err
}
binary.BigEndian.PutUint16(ip4Buf[10:12], ip4Chk) // x/net/ipv4.Header.Marshal
binary.BigEndian.PutUint16(b[10:12], uint16(h.Checksum))
Yep, that's exactly what I'd like to see. |
Okay, update https://go-review.googlesource.com/c/net/+/112817 as such (and include the doc change on that field) and I'll approve. |
I'm good with that, but I still don't understand what the "system checksum" is and would appreciate some clarification how it is different than what was present in my CL. |
None of us understand the conditions under which the "system checksum" is different. As long as your CL is the wire checksum, I think this is OK. |
Sorry for being late.
I think I didn't use the word "system checksum." Please let me know the place where I used the word for correction; for avoiding unnecessary confusion.
Well, s/checksum/a few fields/. On IPv4 datagram reception via raw IP socket, some kernels tweak a few fields of the received IPv4 header, and on the transmission, some kernels require a few wire-format incompatible fields of IPv4 header, ignore the existing checksum field and recalculate and set the checksum field. In addition, the tweaks/requirements change in context, for example, X over IPv4, IPv4 over X and IPv4 header in ICMP error notification may need different tweaks/requirements. I still don't think it's worth the effort of abstraction (or adding API just work for Linux and IPv4 datagrams captured in link layers.) I still don't want to mix the existing host-stack friendly IP-level socket option API, x/net/{ipv4,ipv6,icmp} packages, with wire-format friendly API for avoiding unnecessary confusion. So I'd prefer making a new wire-format friendly API instead. |
and which kernel version: In CL 112817, it looks to me that calculating a wire-format IPv4 header checksum field using the output of ipv4.Header.Marshal "without calibration" may generate a wrong value on the kernels mentioned above.
X means a protocol instance that is capable of tunneling/stacking with IPv4. As long as the kernels tweak IPv4 datagrams when they process incoming datagrams, applications must be sure whether the datagrams pass through the IPv4 protocol stack or not on the kernels, unfortunately. Otherwise, applications may face funny troubles like "ipv4.PaserHeader parses IPv4 datagrams captured in link-layers on BSD variants incorrectly."
Since the OP states that the use case is just for "dealing with the wire format" and the package description of x/net/ipv4 says "IP-level socket options", I think the objectives are different (even though the implementation in CL 112817 may work for any layer tapping on Linux). The former is useful for universal applications and the latter is for maintaining signaling and routing applications tightly coupled with the host stack. I think it's better to have separate packages for each objective. |
I know what the expression means. Which actual X value? We don't want more net packages for every esoteric corner of networking behavior. @mdlayher's CL is a reasonable addition. Documentation can address any concerns about it not working on "X over Y with Z". |
On Darwin, DragonFly BSD, NetBSD, FreeBSD 10 or below, every usable layer 2 and 3 protocol.
I have to disagree. Hm, again,
I'm still not sure the reason why the CL uses the inappropriate byte sequence for wire checksum calculation, the output of ipv4.Header.Marshal. In other words, why not use a byte sequence in wire format instead? In that way, no need to worry about each platform implementation. |
I'd be okay with closing this out as I no longer have a need for this, unless there are objections to doing so. |
I still want to see some better docs at least. @mikioh confused me and if I ever look at this field again, I'd like to know what it means. |
I'm not @mdlayher or @mikioh, but as someone recently interested in this code, I can try to explain what I believe to be going on. Let's say you created a raw socket and print the first packet you receive. package main
import (
"encoding/hex"
"fmt"
"net"
"os"
"golang.org/x/net/ipv4"
)
func main() {
conn, err := net.ListenIP("ip:40", &net.IPAddr{
IP: net.ParseIP("127.0.0.1"),
})
if err != nil {
fmt.Fprintf(os.Stderr, "error listening: %v\n", err)
os.Exit(-1)
}
buf := make([]byte, 1024)
n, _ := conn.Read(buf)
fmt.Printf("buf: (len=%d)\n%s\n\n", len(buf[:n]), hex.Dump(buf[:n]))
hdr, err := ipv4.ParseHeader(buf[:n])
if err != nil {
fmt.Fprintf(os.Stderr, "error parsing header: %v\n", err)
os.Exit(-1)
}
fmt.Printf("IP hdr: %+v\n", hdr)
} buf: (len=43)
00000000 45 00 00 2b f7 77 40 00 40 28 45 31 7f 00 00 01 |E..+.w@.@(E1....|
00000010 7f 00 00 01 00 00 00 18 00 00 00 07 00 07 00 00 |................|
00000020 00 01 00 00 00 00 68 65 6c 6c 6f |......hello|
IP hdr: ver=4 hdrlen=20 tos=0x0 totallen=43 id=0xf777 flags=0x2 fragoff=0x0 ttl=64 proto=40 cksum=0x4531 src=127.0.0.1 dst=127.0.0.1 On Linux, the bytes you receive are the same bytes received over the wire. Computing the IPv4 checksum over On at least some versions of Darwin, FreeBSD, NetBSD, and DragonFlyBSD this assumption isn't true, the values may be significantly different. FreeBSD's
The checksum value received on the raw connection would be what was seen "on-the-wire" (that is, with these fields in network byte order). Computing a checksum separately by using the output of I'd suggest updating the documentation of |
I'm working on a project that uses packet sockets directly, and I have to calculate the IPv4 checksum on my own since I'm building from Ethernet frames up.
I notice that
x/net/ipv4
doesn't provide any way to easily calculate a checksum, but I think such a function/method could be useful in conjunction with theipv4.Header
type.I wrote a basic implementation in ~30 lines with documentation comments, and would be happy to submit it upstream. At this point, my questions are:
x/net/ipv4
?My current implementation accepts a byte slice from the output of
ipv4.Header.Marshal
, but I could see a method making sense as well./cc @mikioh
The text was updated successfully, but these errors were encountered: