Seems fine to me for x/net.

Change https://golang.org/cl/112817 mentions this issue: ipv4: add Header.HeaderChecksum method

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.

how can the checksum be OS-specific?

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

As mentioned in #18633, the byte sequences may differ from the wire format on a few BSD variants.

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.

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.

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.

@mdlayher can you show us (or describe) examples of code where you would use this function? A few of us are trying to understand this wire-neutral checksum vs what @mikioh's been saying here and in #18633 and are a bit confused about exactly when each would be needed. Thanks.

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 func (h *Header) UpdateChecksum() that updates h.Checksum?

@mdlayher, what is https://godoc.org/golang.org/x/net/ipv4#Header.Checksum today?

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

But if that field is already the wire checksum (for received packets only), what about a method that like > > func (h *Header) UpdateChecksum() that updates 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.

two checksums (a system checksum vs a wire checksum?) as @mikioh says.

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.

It sounds like some operating systems separately require user-space code to send the kernel a different checksum over IPv4 headers in certain contexts

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.

@mikioh, we want to proceed with @mdlayher's CL. Please give a concrete example of why this is a bad idea. Which OS, and which "X" for "X over IPv4"?

Which OS

and which kernel version:
https://github.com/golang/net/blob/master/ipv4/header.go#L118
https://github.com/golang/net/blob/master/ipv4/header.go#L66
https://github.com/golang/net/blob/master/icmp/ipv4.go#L42

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.

which "X" for "X over IPv4"?

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."

we want to proceed with @mdlayher's CL.

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.

X means a protocol instance that is capable of tunneling/stacking with IPv4. A

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".

Which actual X value?

On Darwin, DragonFly BSD, NetBSD, FreeBSD 10 or below, every usable layer 2 and 3 protocol.

@mdlayher's CL is a reasonable addition.

I have to disagree. Hm, again,

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.

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 buf[:n] using the CL function results in the same value as we received, 0x4531.

On at least some versions of Darwin, FreeBSD, NetBSD, and DragonFlyBSD this assumption isn't true, the values may be significantly different. FreeBSD's ip(4) for example:

Before FreeBSD 10.0 packets received on raw IP sockets had the ip_hl sub-
tracted from the ip_len field.

Before FreeBSD 11.0 packets received on raw IP sockets had the ip_len and
ip_off fields converted to host byte order.  Packets written to raw IP
sockets were expected to have ip_len and ip_off in host byte order.

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 Marshal on those systems would result in differing values.

I'd suggest updating the documentation of ipv4.Header.Checksum to something along the lines of "wire header checksum". As far as I know, when sending raw packets, if set to 0 it will be filled in by all kernels, even if the IP_HDRINCL option is provided