sendQueue: ignore "datagram too large" error #3328

zllovesuki · 2022-02-09T12:13:31Z

This commit introduces additional platform-dependent checking when the
kernel returns an error. Previously, the session is terminated when
PingFrame sends a discovery packet larger than the limit. With this
commit, the error is checked, and if it is "datagram too large", the
error is ignored.

Additionally,

This commit re-enables MTU discovery on Windows unless it
is disabled explicitly by user (Undo disable Path MTU Discovery on Windows #3276),
Set IP_DONTFRAGMENT and IPV6_DONTFRAG with error checking on Windows,
and
Set IP_MTU_DISCOVERY to PMTUDISC_DO for both IPv4 and IPv6 on Linux
so that the kernel will return "message too long".

Fixes #3273 #3327

zllovesuki · 2022-02-09T12:40:59Z

Hooking into onLost and onAcked shows that MTU was able to increase with this commit.

		OnLost: func(wire.Frame) {
			f.probeInFlight = false
			f.max = size
			fmt.Printf("MTU DISCOVERY GONE: %+v\n", f)
		},
		OnAcked: func(wire.Frame) {
			f.probeInFlight = false
			f.current = size
			f.mtuIncreased(size)
			fmt.Printf("MTU DISCOVERY GOT: %+v\n", f)
		},

On a connection via WireGuard that causes MTU to be reduced (Windows):

MTU DISCOVERY GOT: &{lastProbeTime:{wall:13869129029832926620 ext:622940201 loc:0x171a600} probeInFlight:false mtuIncreased:0x102ede0 rttStats:0xc0000d43c0 current:1352 max:1452}
MTU DISCOVERY GONE: &{lastProbeTime:{wall:13869129030004107620 ext:794121201 loc:0x171a600} probeInFlight:false mtuIncreased:0x102ede0 rttStats:0xc0000d43c0 current:1352 max:1402}
MTU DISCOVERY GOT: &{lastProbeTime:{wall:13869129038891791436 ext:9018128601 loc:0x171a600} probeInFlight:false mtuIncreased:0x102ede0 rttStats:0xc0000d43c0 current:1377 max:1402}
MTU DISCOVERY GOT: &{lastProbeTime:{wall:13869129039070470136 ext:9196807301 loc:0x171a600} probeInFlight:false mtuIncreased:0x102ede0 rttStats:0xc0000d43c0 current:1389 max:1402}

On vanilla connection (Windows):

MTU DISCOVERY GOT: &{lastProbeTime:{wall:13869129101720445128 ext:470531601 loc:0x11ba600} probeInFlight:false mtuIncreased:0xacede0 rttStats:0xc00002c540 current:1352 max:1452}
MTU DISCOVERY GOT: &{lastProbeTime:{wall:13869129101906285828 ext:656372301 loc:0x11ba600} probeInFlight:false mtuIncreased:0xacede0 rttStats:0xc00002c540 current:1402 max:1452}
MTU DISCOVERY GOT: &{lastProbeTime:{wall:13869129102166036452 ext:842381101 loc:0x11ba600} probeInFlight:false mtuIncreased:0xacede0 rttStats:0xc00002c540 current:1427 max:1452}
MTU DISCOVERY GOT: &{lastProbeTime:{wall:13869129102354195652 ext:1030540301 loc:0x11ba600} probeInFlight:false mtuIncreased:0xacede0 rttStats:0xc00002c540 current:1439 max:1452}

marten-seemann

This looks very clean. Thank you @zllovesuki!
Just a few nits.

err_size.go

send_queue.go

conn_windows.go

codecov · 2022-02-09T13:40:50Z

Codecov Report

Merging #3328 (b255d7d) into master (f3b0987) will decrease coverage by 0.02%.
The diff coverage is 81.82%.

@@            Coverage Diff             @@
##           master    #3328      +/-   ##
==========================================
- Coverage   85.63%   85.61%   -0.02%     
==========================================
  Files         133      136       +3     
  Lines        9792     9808      +16     
==========================================
+ Hits         8385     8397      +12     
- Misses       1032     1036       +4     
  Partials      375      375

Impacted Files	Coverage Δ
conn_windows.go	`58.82% <66.67%> (+2.30%)`	⬆️
conn_oob.go	`72.09% <100.00%> (+0.22%)`	⬆️
conn_oob_opts_linux.go	`100.00% <100.00%> (ø)`
err_size_linux.go	`100.00% <100.00%> (ø)`
err_size_windows.go	`100.00% <100.00%> (ø)`
send_queue.go	`96.77% <100.00%> (+0.11%)`	⬆️
session.go	`77.35% <100.00%> (-0.02%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3b0987...b255d7d. Read the comment docs.

This commit introduces additional platform-dependent checking when the kernel returns an error. Previously, the session is terminated when PingFrame sends a discovery packet larger than the limit. With this commit, the error is checked, and if it is "datagram too large", the error is ignored. Additionally, 1. This commit re-enables MTU discovery on Windows unless it is disabled explicitly by user (Undo quic-go#3276), 2. Set IP_DONTFRAGMENT and IPV6_DONTFRAG with error checking on Windows, and 3. Set IP_MTU_DISCOVERY to PMTUDISC_DO for both IPv4 and IPv6 on Linux so that the kernel will return "message too long". Fixes quic-go#3273 quic-go#3327

zllovesuki · 2022-02-09T14:48:16Z

Unfortunately I couldn't properly test the commit on Linux or FreeBSD as they are running in Hyper-V. If someone can help with testing those (e.g. vanilla vs via VPN so reduced MTU) that would be greatly appreciated!

On illumos/amd64 it produces no regression (good).

tobyxdd · 2022-02-12T03:27:39Z

Perhaps we should disable PMTUD for all platforms, except for those that have a working isMsgSizeErr? This PR only covers Windows and Linux. Does anyone know the default behavior and how to turn on DF on platforms like macOS, FreeBSD, etc.?

zllovesuki · 2022-02-12T19:23:31Z

That is certainly an idea, however I wold really prefer if we can add more support for those OSes. MTU of 1300 vs 1400, that's extra 10 packets to send for 1000 bytes of data (!). Literally making the internet a better place if we can make it more efficient.

tobyxdd · 2022-02-20T02:05:56Z

@marten-seemann care to comment on this?

I can confirm that this works fine on Linux & Windows.

tobyxdd · 2022-02-20T03:53:38Z

https://gist.github.com/tobyxdd/1964cc8d559fb367010c80c1dafee1b1

This patch (based on the code of current PR) moves the set DF implementation to a separate file for each platform & the passed-in PacketConn no longer needs to be an OOBCapablePacketConn - it only needs to have SyscallConn.

This is useful when the PacketConn is not a net.UDPConn for whatever reason (e.g. in my project where there's an added layer of obfuscation on top of UDPConn)

marten-seemann

LGTM. Thank you @zllovesuki!

zllovesuki · 2022-02-20T08:05:29Z

gist.github.com/tobyxdd/1964cc8d559fb367010c80c1dafee1b1

This patch (based on the code of current PR) moves the set DF implementation to a separate file for each platform & the passed-in PacketConn no longer needs to be an OOBCapablePacketConn - it only needs to have SyscallConn.

This is useful when the PacketConn is not a net.UDPConn for whatever reason (e.g. in my project where there's an added layer of obfuscation on top of UDPConn)

Do you think we should abstract this part out? @marten-seemann

marten-seemann · 2022-02-20T08:13:07Z

Yes, but let's do that in a follow-up PR. Makes reviewing easier.

zllovesuki · 2022-02-20T08:15:30Z

/unrelated

I got my hands on an ppc64le machine to test (courtesy of folks at https://openpower.ic.unicamp.br/minicloud/), and it seems like quic-go works quite well on ppc64le as well (on Debian/ppc64el at least).

zllovesuki · 2022-02-20T08:17:29Z

@tobyxdd would you mind submitting a new PR with your patch after this PR has been merged?

database64128 · 2022-04-14T13:27:51Z

If we are going to ignore the ICMP PTB message anyway, why not just set IP(V6)_MTU_DISCOVER to IP_PMTUDISC_PROBE? This disables IP fragmentation and allows you to send UDP packets exceeding the system cached PMTU without returning -EMSGSIZE.

IP_PMTUDISC_PROBE is supported by Linux and all versions of Windows that are still supported by Microsoft. Unless supporting outdated Windows systems like Windows 7 is a goal, IP(V6)_MTU_DISCOVER should be preferred over IP(V6)_DONTFRAGMENT.

tobyxdd · 2022-04-14T17:10:04Z

If we are going to ignore the ICMP PTB message anyway, why not just set IP(V6)_MTU_DISCOVER to IP_PMTUDISC_PROBE? This disables IP fragmentation and allows you to send UDP packets exceeding the system cached PMTU without returning -EMSGSIZE.

IP_PMTUDISC_PROBE is supported by Linux and all versions of Windows that are still supported by Microsoft. Unless supporting outdated Windows systems like Windows 7 is a goal, IP(V6)_MTU_DISCOVER should be preferred over IP(V6)_DONTFRAGMENT.

https://docs.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options#windows-support-for-ip_proto-options

According to MS docs, neither IP_PMTUDISC_PROBE nor IP_MTU_DISCOVER is available on Windows

database64128 · 2022-04-14T17:51:58Z

@tobyxdd The link you referenced directly contradicts what you said. Just press Ctrl + F and search IP_MTU_DISCOVER.

If you have any Windows device around, open "C:\Program Files (x86)\Windows Kits\10\Include\10.0.22000.0\shared\ws2ipdef.h" and go to line 121. Everything I mentioned is right there.

I suggested using IP_MTU_DISCOVER on Windows, because recently I have been working on this project of mine and it requires disabling IP fragmentation. The code dealing with disabling IP fragmentation on Linux, Windows, macOS and FreeBSD is here. I can confirm it works as intended on my Windows and Linux PC. A friend of mine confirmed that it also works on macOS.

tobyxdd · 2022-04-14T18:19:52Z

@tobyxdd The link you referenced directly contradicts what you said. Just press Ctrl + F and search IP_MTU_DISCOVER.

If you have any Windows device around, open "C:\Program Files (x86)\Windows Kits\10\Include\10.0.22000.0\shared\ws2ipdef.h" and go to line 121. Everything I mentioned is right there.

I suggested using IP_MTU_DISCOVER on Windows, because recently I have been working on this project of mine and it requires disabling IP fragmentation. The code dealing with disabling IP fragmentation on Linux, Windows, macOS and FreeBSD is here. I can confirm it works as intended on my Windows and Linux PC. A friend of mine confirmed that it also works on macOS.

My bad. I did see IP_MTU_DISCOVER, but since it's not listed in the "Windows support for IP_PROTO options" chart, I thought it was unsupported. I'll give it a try later today, and possibly submit a new PR.

This commit introduces additional platform-dependent checking when the kernel returns an error. Previously, the session is terminated when PingFrame sends a discovery packet larger than the limit. With this commit, the error is checked, and if it is "datagram too large", the error is ignored. Additionally, 1. This commit re-enables MTU discovery on Windows unless it is disabled explicitly by user (Undo quic-go#3276), 2. Set IP_DONTFRAGMENT and IPV6_DONTFRAG with error checking on Windows, and 3. Set IP_MTU_DISCOVERY to PMTUDISC_DO for both IPv4 and IPv6 on Linux so that the kernel will return "message too long". Fixes quic-go#3273 quic-go#3327

zllovesuki force-pushed the emsgsize_fix branch from ba9563a to a1fb28a Compare February 9, 2022 13:16

marten-seemann approved these changes Feb 9, 2022

View reviewed changes

err_size.go Show resolved Hide resolved

send_queue.go Show resolved Hide resolved

conn_windows.go Outdated Show resolved Hide resolved

zllovesuki force-pushed the emsgsize_fix branch from a1fb28a to f5b6364 Compare February 9, 2022 14:28

zllovesuki force-pushed the emsgsize_fix branch from f5b6364 to b255d7d Compare February 9, 2022 14:37

zllovesuki requested a review from marten-seemann February 9, 2022 16:21

marten-seemann approved these changes Feb 20, 2022

View reviewed changes

marten-seemann merged commit fd2c345 into quic-go:master Feb 20, 2022

marten-seemann mentioned this pull request Mar 17, 2022

PMTUD may be implemented incorrectly #3327

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sendQueue: ignore "datagram too large" error #3328

sendQueue: ignore "datagram too large" error #3328

zllovesuki commented Feb 9, 2022 •

edited

zllovesuki commented Feb 9, 2022 •

edited

marten-seemann left a comment

codecov bot commented Feb 9, 2022 •

edited

zllovesuki commented Feb 9, 2022 •

edited

tobyxdd commented Feb 12, 2022 •

edited

zllovesuki commented Feb 12, 2022

tobyxdd commented Feb 20, 2022

tobyxdd commented Feb 20, 2022

marten-seemann left a comment

zllovesuki commented Feb 20, 2022

marten-seemann commented Feb 20, 2022

zllovesuki commented Feb 20, 2022

zllovesuki commented Feb 20, 2022

database64128 commented Apr 14, 2022

tobyxdd commented Apr 14, 2022

database64128 commented Apr 14, 2022

tobyxdd commented Apr 14, 2022

sendQueue: ignore "datagram too large" error #3328

sendQueue: ignore "datagram too large" error #3328

Conversation

zllovesuki commented Feb 9, 2022 • edited

zllovesuki commented Feb 9, 2022 • edited

marten-seemann left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 9, 2022 • edited

Codecov Report

zllovesuki commented Feb 9, 2022 • edited

tobyxdd commented Feb 12, 2022 • edited

zllovesuki commented Feb 12, 2022

tobyxdd commented Feb 20, 2022

tobyxdd commented Feb 20, 2022

marten-seemann left a comment

Choose a reason for hiding this comment

zllovesuki commented Feb 20, 2022

marten-seemann commented Feb 20, 2022

zllovesuki commented Feb 20, 2022

zllovesuki commented Feb 20, 2022

database64128 commented Apr 14, 2022

tobyxdd commented Apr 14, 2022

database64128 commented Apr 14, 2022

tobyxdd commented Apr 14, 2022

zllovesuki commented Feb 9, 2022 •

edited

zllovesuki commented Feb 9, 2022 •

edited

codecov bot commented Feb 9, 2022 •

edited

zllovesuki commented Feb 9, 2022 •

edited

tobyxdd commented Feb 12, 2022 •

edited