Dialing fails with an INTERNAL_ERROR when ECN is enabled

[arashpayan]

I found an issue with ECN on at least one configuration of Linux:

Configuration
Distribution: CentOS Linux
Version: 7
Kernel: 3.10.0-1160.el7.x86_64
CPU: Intel Xeon CPU E5-2683 v4 @ 2.10GHz

Reproduction

conn, err := quic.DialAddr(context.Background(), "some.address:port", &tls.Config{}, &quic.Config{})

Expected
conn is valid and err should be nil.

Actual
err is INTERNAL_ERROR (local): write udp [::]:<SRCPORT>->XXX.XXX.XXX.XXX:DSTPORT: sendmsg: invalid argument

If the program is run with QUIC_GO_DISABLE_ECN=true the dial succeeds.

Notes
Here's a qlog in case it helps: https://ara.sh/private/4250200fe9d3c5d44d3f60cf346c9c82bd0f53_client.qlog

I'm available to gather other info and try out other fixes as you may need.

[marten-seemann]

We've had some reports of ECN not working on certain systems: #4178

I'm available to gather other info and try out other fixes as you may need.

This would be greatly appreciated!!! I was unable to reproduce these failures, so if you could help us figure out a way to make ECN work on every system, that would be amazing!

First suspect would be the size of the cmsg, see AdguardTeam/AdGuardHome#6422 (comment). Could you change it and see if that fixes the problem?

[arashpayan]

That was it! Changing ecnIPv4DataLen to 4 lets it connect. It's very odd that the man page says it's a single byte.

Would you like me to open a PR with the change?

[marten-seemann]

We're in a bit of a conundrum here. We changed it to the 1 byte value in #4127, based on the discussion in AdguardTeam/AdGuardHome#6335. Apparently there's a (different?) set of Linux machines that fails if we don't use the 1 byte value.

What we could do (and what I would really, really hate!) is build a fallback: first try one length, and then switch to the other length. I'm not ready to build such an ugly mechanism yet.

I'm wondering if there's any way we can find out which size to use. Any ideas?

Also looping in @ainar-g, do you have any opinion here?

[ainar-g]

@marten-seemann, I haven't been able to determine a way to discover it the last time. I've treid looking through the Linux Kernel changelogs, but each revision is a separate file, so I gave up and started digging through manuals instead.

This time I've decided a more radical approach: wget'ing the changelogs for v4 branch off of Kernel's CDN and grep -r'ing through them.

I have found this message about commit torvalds/linux@c4cbc3ebb9fc4 in version 4.4.223:

[…]

In commit f02db315b8d8 ("ipv4: IP_TOS and IP_TTL can be specified as
ancillary data") Francesco added IP_TOS values specified as integer.

However, kernel sends to userspace (at recvmsg() time) an IP_TOS value
in a single byte, when IP_RECVTOS is set on the socket.

It can be very useful to reflect all ancillary options as given by the
kernel in a subsequent sendmsg(), instead of aborting the sendmsg() with
EINVAL after Francesco patch.

So this patch extends IP_TOS ancillary to accept an u8, so that an UDP
server can simply reuse same ancillary block without having to mangle
it.

[…]

If I'm reading this correctly, on older kernels software must receive IP_TOS value as a byte, but send the value as four bytes. That is, there are two separate ECS sizes at play.

[arashpayan]

I agree with @ainar-g's assessment about the change reflected in the commit, except for the kernel version involved. I don't know why the changelog for 4.4 shows that commit (with that commit id). I even downloaded a tarball of the 4.4 kernel and didn't see the new code in there.

So I pulled down Linus's tree locally, and found that the commit in question was added to his tree on September 7, 2016 and had a different commit id too. That would mean the first possible release it could be in would be 4.8.

If I'm actually right, and there's a good chance I'm not because I don't know the ins and outs of cutting kernel releases, the change arriving in 4.8 would line up with what was reported in AdguardTeam/AdGuardHome#6422. The reported problematic kernels there are all <= 4.5.

Anyway...

I think 4 also might not be the best value for older kernels, because the old code actually checked if the length was equal to sizeof(int):

if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)))
	return -EINVAL;

I'm not an expert in C, but I don't think sizeof(int) is guaranteed to be 4, though it often is 4. If it was wouldn't they have compared against 4 instead? I think it could be 2, 4 or 8 bytes.

I see two options from here:

• check what kernel you're running on. If it's < 4.8, try 2 then 4 then 8 bytes for the IP_TOS length until sendmsg works. (the kludgy hack you wanted to avoid)
• Disable ECN if the kernel version is < 4.8.

If it were completely up to me, I'd just disable it for kernels older than 4.8. I don't think ECN is a must have for many people, and if it is, they should just make sure they have a recent kernel installed.

Thoughts?

[marten-seemann]

Disabling ECN is totally fine. The performance benefits (at least until we've implemented L4S / Prague) are not as big as you'd get with GSO, for example.

The question is, how do we detect the kernel version? We could also just disable ECN if we receive this error. The problem is, the error is as unspecific as it can be, if I understand correctly. So while this works for now, what if we add another cmsg at some point in the future?

[arashpayan]

We could detect the kernel version with this.

var releaseRE = regexp.MustCompile(`(?P<major>\d+)\.(?P<minor>\d+)\.(?P<point>\d+)`)

func KernelVersion() (major, minor, point uint, err error) {
	buf := syscall.Utsname{}
	if err := syscall.Uname(&buf); err != nil {
		return 0, 0, 0, err
	}

	var r []byte
	for _, b := range buf.Release {
		if b == 0 {
			break
		}
		r = append(r, byte(b))
	}

	match := releaseRE.FindSubmatch(r)
	if match == nil {
		return 0, 0, 0, errors.New("version not found")
	}

	mjr, _ := strconv.ParseUint(string(match[1]), 10, 8)
	major = uint(mjr)

	mnr, _ := strconv.ParseUint(string(match[2]), 10, 8)
	minor = uint(mnr)

	pnt, _ := strconv.ParseUint(string(match[3]), 10, 8)
	point = uint(pnt)

	return
}

[ainar-g]

There seem to be backports to several v4.x branches, so if one wanted to detect this as rigorously as possible (excluding kernel modifications by distributions' maintainers), they'd need a list of these backports and detecting these versions in a manner similar the the above comment. I.e. if major == 4 && ((minor == 4 && patch <= 223) || ….

A less complex way would be to find the v4.x branch that is guaranteed to include the change from the start.

[arashpayan]

tl;dr I suggest modifying isECNDisabled() to check if the kernel is at least at version 4.9. @marten-seemann, if that's acceptable to you I can create a PR.

Full details

I checked the tarballs of tagged commits from Linus's branch because that's where Ubuntu pulls from for their kernels:

The kernel source for the Ubuntu kernel is based very closely on the upstream mainline kernel tree maintained by Linus. The Ubuntu-ness of this kernel is maintained as a git branch against the Linus tree.

The first tagged release that contains the IP_TOS change is 4.9.

I wouldn't want to introduce the complexity required to check for backports into even older kernels. There are so many distributions out there, and that doesn't even account for people building their own kernel. 4.9 is already 7.5 years old and the number of users of even older kernels is quickly diminishing.