PMTUD may be implemented incorrectly

[tobyxdd]

Hi,

We talked about the DF bit on Windows a while ago, and I submitted #3155. But apparently it led to this unforeseen problem #3273 so PMTUD ended up being disabled on Windows as a dirty fix. As I revisited this today I realized that perhaps there is something fundamentally wrong with the way quic-go currently implements PMTUD, and it's not really a Windows issue.

Previously we both assumed that the DF bit is enabled by default on Linux. But that's not true, as the man page says this:

The default behavior on Linux (IP_PMTUDISC_WANT) is to only set DF bit when the packet is smaller than the path MTU, and fragment the packet otherwise. This implies that right now even if quic-go sends a packet larger than the path MTU, it will still be fragmented, sent, and received, nullifying PMTUD altogether. And if you explicitly set it to IP_PMTUDISC_DO, you will get a similar behavior as on Windows - errors when trying to send packets larger than the path MTU, instead of discarding them silently.

I wrote a demo and verified this on my Ubuntu 20.04 server, which has an MTU of 1500. When it sends packets smaller than 1500, the DF bit is set. When it sends packets larger than 1500, it fragments them and sends them successfully nonetheless.

[tobyxdd]

I also wrote a custom PacketConn that adds some padding bytes to every packet to make it larger, and passed it to quic-go. The QUIC payload reaches MaxPacketBufferSize regardless of the MTU and the actual packet size going out.

[marten-seemann]

Does setting the DF socket option resolve this?

[tobyxdd]

That would lead to the same problem as in #3273 - the kernel returns error instead of silently dropping the packet, at least on Linux. Haven't tried it on any other platform.

[tobyxdd]

The error looks like this on Linux: write udp [::]:52857->x.x.x.x:6003: sendto: message too long

[marten-seemann]

We can either catch that error, or just ignore it and let loss recovery kick in after an RTT or so. This is not the main purpose of PMTUD anyway. You really shouldn't be sending packets larger than your local link MTU anyway. The more interesting use for PMTUD is finding the MTU of a path over the internet, for which the current code still works fine (or doesn't it)?

[tobyxdd]

I'm not sure how the current code handles this error, I'll have a look tomorrow. But I can confirm that this error does NOT only occur when sending a packet larger than the local MTU. It occurs when sending a packet larger than the path MTU.

[marten-seemann]

How does the kernel know about the path MTU in the case of UDP?

[tobyxdd]

We can either catch that error, or just ignore it and let loss recovery kick in after an RTT or so. This is not the main purpose of PMTUD anyway. You really shouldn't be sending packets larger than your local link MTU anyway. The more interesting use for PMTUD is finding the MTU of a path over the internet, for which the current code still works fine (or doesn't it)?

No it doesn't. quic-go doesn't do anything about DF bit on Linux right now. Since by default it's IP_PMTUDISC_WANT, Linux DOES fragment and send the probe packets.

[tobyxdd]

How does the kernel know about the path MTU in the case of UDP?

https://man7.org/linux/man-pages/man7/udp.7.html

[marten-seemann]

I'm not sure how the kernel would do that for UDP. There's no acknowledgements, so the kernel can't know if a packet of a certain size has been delivered (ICMP can't be relied upon on the internet).

[tobyxdd]

ICMP can't be relied upon on the internet

But ICMP does work in most cases. I assume that's how Linux implements it (RFC 1191)

[marten-seemann]

Then the right solution would be to ignore this error message. PMTUD packets don't carry any application data, so there's no benefit in declaring them lost right away. If we wanted to be fancy, we could tell the MTU discoverer right away, but I'm not sure if that's worth the extra complexity.

Do you want to send us a PR?

[tobyxdd]

Tried this code on a VPS with a local MTU of 9000 but a path MTU of 1500.

package main

import (
	"fmt"
	"golang.org/x/sys/unix"
	"net"
)

func main() {
	conn, err := net.ListenUDP("udp", nil)
	if err != nil {
		panic(err)
	}
	defer conn.Close()
	rc, err := conn.SyscallConn()
	if err != nil {
		panic(err)
	}
	err = rc.Control(func(fd uintptr) {
		err := unix.SetsockoptInt(int(fd), unix.IPPROTO_IP, unix.IP_MTU_DISCOVER, unix.IP_PMTUDISC_DO)
		if err != nil {
			panic(err)
		}
	})
	if err != nil {
		panic(err)
	}
	bs := make([]byte, 7000)
	fmt.Println(conn.WriteToUDP(bs, &net.UDPAddr{
		IP:   net.IPv4(8, 8, 8, 8),
		Port: 53,
	}))
}

First packet went through. Subsequent attempts all returned errors.

[tobyxdd]

Then the right solution would be to ignore this error message. PMTUD packets don't carry any application data, so there's no benefit in declaring them lost right away. If we wanted to be fancy, we could tell the MTU discoverer right away, but I'm not sure if that's worth the extra complexity.

Do you want to send us a PR?

Maybe 🤔 I'll see what I can do

[zllovesuki]

Then the right solution would be to ignore this error message. PMTUD packets don't carry any application data, so there's no benefit in declaring them lost right away. If we wanted to be fancy, we could tell the MTU discoverer right away, but I'm not sure if that's worth the extra complexity.
Do you want to send us a PR?

Maybe 🤔 I'll see what I can do

I'm too busy with school to dive into this. If you are able, would greatly appreciate your effort!

[tobyxdd]

@marten-seemann Are we ignoring all errors from WritePacket or just MTU-related ones? I'm not sure if the former would have any side effects. As for the latter, the only way to tell is... not so elegant:

	bs := make([]byte, 7000)
	_, err = conn.WriteToUDP(bs, &net.UDPAddr{
		IP:   net.IPv4(8, 8, 8, 8),
		Port: 53,
	})
	if opErr, ok := err.(*net.OpError); ok {
		if syscallErr, ok := opErr.Err.(*os.SyscallError); ok {
			if syscallErrno, ok := syscallErr.Err.(syscall.Errno); ok {
				if syscallErrno == unix.EMSGSIZE {
					// This is what we are looking for.
					fmt.Println("EMSGSIZE")
				}
				fmt.Println(syscallErrno)
				fmt.Println(syscallErrno.Error())
			}
		}
	}

This is undocumented and platform-specific.

[marten-seemann]

I think it would be nicer to just ignore the MTU-related ones. At least that would be a requirement if we want to feed the information back into the MTU discoverer immediately.

Your code might become a bit more elegant if you use errors.As, but of course that doesn't change the fact that we'll need a platform-specific isMTUError function.

[marten-seemann]

This was fixed in #3328.