From 4128148b1683d9b7d9483cdc23dd68ce0a51d75a Mon Sep 17 00:00:00 2001 From: Rachel Chen Date: Wed, 9 Feb 2022 04:04:24 -0800 Subject: [PATCH] sendQueue: ignore "datagram too large" error 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 #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 #3273 #3327 --- conn_generic.go | 2 -- conn_helper_darwin.go | 5 +--- conn_helper_freebsd.go | 3 +-- conn_helper_linux.go | 5 +--- conn_oob.go | 2 ++ conn_oob_opts.go | 8 ++++++ conn_oob_opts_linux.go | 15 +++++++++++ conn_windows.go | 25 +++++++++++++----- err_size.go | 9 +++++++ err_size_linux.go | 15 +++++++++++ err_size_windows.go | 15 +++++++++++ interface.go | 2 +- send_queue.go | 8 +++++- session.go | 10 +++----- session_test.go | 57 ++++++++++++++++++++---------------------- 15 files changed, 124 insertions(+), 57 deletions(-) create mode 100644 conn_oob_opts.go create mode 100644 conn_oob_opts_linux.go create mode 100644 err_size.go create mode 100644 err_size_linux.go create mode 100644 err_size_windows.go diff --git a/conn_generic.go b/conn_generic.go index 526778c1ccc..a2a125c08e6 100644 --- a/conn_generic.go +++ b/conn_generic.go @@ -5,8 +5,6 @@ package quic import "net" -const disablePathMTUDiscovery = false - func newConn(c net.PacketConn) (connection, error) { return &basicConn{PacketConn: c}, nil } diff --git a/conn_helper_darwin.go b/conn_helper_darwin.go index fdab73b6159..eabf489f109 100644 --- a/conn_helper_darwin.go +++ b/conn_helper_darwin.go @@ -5,10 +5,7 @@ package quic import "golang.org/x/sys/unix" -const ( - msgTypeIPTOS = unix.IP_RECVTOS - disablePathMTUDiscovery = false -) +const msgTypeIPTOS = unix.IP_RECVTOS const ( ipv4RECVPKTINFO = unix.IP_RECVPKTINFO diff --git a/conn_helper_freebsd.go b/conn_helper_freebsd.go index e22f986127c..0b3e8434b8a 100644 --- a/conn_helper_freebsd.go +++ b/conn_helper_freebsd.go @@ -6,8 +6,7 @@ package quic import "golang.org/x/sys/unix" const ( - msgTypeIPTOS = unix.IP_RECVTOS - disablePathMTUDiscovery = false + msgTypeIPTOS = unix.IP_RECVTOS ) const ( diff --git a/conn_helper_linux.go b/conn_helper_linux.go index 4aa04dc9c2e..51bec900242 100644 --- a/conn_helper_linux.go +++ b/conn_helper_linux.go @@ -5,10 +5,7 @@ package quic import "golang.org/x/sys/unix" -const ( - msgTypeIPTOS = unix.IP_TOS - disablePathMTUDiscovery = false -) +const msgTypeIPTOS = unix.IP_TOS const ( ipv4RECVPKTINFO = unix.IP_PKTINFO diff --git a/conn_oob.go b/conn_oob.go index b46781377d2..f1aebfaa354 100644 --- a/conn_oob.go +++ b/conn_oob.go @@ -87,6 +87,8 @@ func newConn(c OOBCapablePacketConn) (*oobConn, error) { errPIIPv4 = unix.SetsockoptInt(int(fd), unix.IPPROTO_IP, ipv4RECVPKTINFO, 1) errPIIPv6 = unix.SetsockoptInt(int(fd), unix.IPPROTO_IPV6, ipv6RECVPKTINFO, 1) } + + setOOBSockOpts(fd) }); err != nil { return nil, err } diff --git a/conn_oob_opts.go b/conn_oob_opts.go new file mode 100644 index 00000000000..4d29c7c4c1f --- /dev/null +++ b/conn_oob_opts.go @@ -0,0 +1,8 @@ +//go:build !linux && !windows +// +build !linux,!windows + +package quic + +func setOOBSockOpts(fd uintptr) { + // no-op on unsupported platforms +} diff --git a/conn_oob_opts_linux.go b/conn_oob_opts_linux.go new file mode 100644 index 00000000000..81e7ca58e69 --- /dev/null +++ b/conn_oob_opts_linux.go @@ -0,0 +1,15 @@ +//go:build linux +// +build linux + +package quic + +import ( + "golang.org/x/sys/unix" +) + +func setOOBSockOpts(fd uintptr) { + // Enabling IP_MTU_DISCOVER will force the kernel to return "sendto: message too long" + // and the datagram will not be fragmented + unix.SetsockoptInt(int(fd), unix.IPPROTO_IP, unix.IP_MTU_DISCOVER, unix.IP_PMTUDISC_DO) + unix.SetsockoptInt(int(fd), unix.IPPROTO_IPV6, unix.IPV6_MTU_DISCOVER, unix.IPV6_PMTUDISC_DO) +} diff --git a/conn_windows.go b/conn_windows.go index a6e591b62aa..72906b8950b 100644 --- a/conn_windows.go +++ b/conn_windows.go @@ -9,12 +9,16 @@ import ( "net" "syscall" + "github.com/lucas-clemente/quic-go/internal/utils" "golang.org/x/sys/windows" ) const ( - disablePathMTUDiscovery = true - IP_DONTFRAGMENT = 14 + // same for both IPv4 and IPv6 on Windows + // https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Networking/WinSock/constant.IP_DONTFRAG.html + // https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Networking/WinSock/constant.IPV6_DONTFRAG.html + IP_DONTFRAGMENT = 14 + IPV6_DONTFRAG = 14 ) func newConn(c OOBCapablePacketConn) (connection, error) { @@ -22,14 +26,23 @@ func newConn(c OOBCapablePacketConn) (connection, error) { if err != nil { return nil, fmt.Errorf("couldn't get syscall.RawConn: %w", err) } + var errDFIPv4, errDFIPv6 error if err := rawConn.Control(func(fd uintptr) { - // This should succeed if the connection is a IPv4 or a dual-stack connection. - // It will fail for IPv6 connections. - // TODO: properly handle error. - _ = windows.SetsockoptInt(windows.Handle(fd), windows.IPPROTO_IP, IP_DONTFRAGMENT, 1) + errDFIPv4 = windows.SetsockoptInt(windows.Handle(fd), windows.IPPROTO_IP, IP_DONTFRAGMENT, 1) + errDFIPv6 = windows.SetsockoptInt(windows.Handle(fd), windows.IPPROTO_IPV6, IPV6_DONTFRAG, 1) }); err != nil { return nil, err } + switch { + case errDFIPv4 == nil && errDFIPv6 == nil: + utils.DefaultLogger.Debugf("Setting DF for IPv4 and IPv6.") + case errDFIPv4 == nil && errDFIPv6 != nil: + utils.DefaultLogger.Debugf("Setting DF for IPv4.") + case errDFIPv4 != nil && errDFIPv6 == nil: + utils.DefaultLogger.Debugf("Setting DF for IPv6.") + case errDFIPv4 != nil && errDFIPv6 != nil: + return nil, errors.New("setting Df failed for both IPv4 and IPv6") + } return &basicConn{PacketConn: c}, nil } diff --git a/err_size.go b/err_size.go new file mode 100644 index 00000000000..a0638574050 --- /dev/null +++ b/err_size.go @@ -0,0 +1,9 @@ +//go:build !linux && !windows +// +build !linux,!windows + +package quic + +func isMsgSizeErr(err error) bool { + // to be implemented for more specific platforms + return false +} diff --git a/err_size_linux.go b/err_size_linux.go new file mode 100644 index 00000000000..aa020809110 --- /dev/null +++ b/err_size_linux.go @@ -0,0 +1,15 @@ +//go:build linux +// +build linux + +package quic + +import ( + "errors" + + "golang.org/x/sys/unix" +) + +func isMsgSizeErr(err error) bool { + // https://man7.org/linux/man-pages/man7/udp.7.html + return errors.Is(err, unix.EMSGSIZE) +} diff --git a/err_size_windows.go b/err_size_windows.go new file mode 100644 index 00000000000..46e72f2466a --- /dev/null +++ b/err_size_windows.go @@ -0,0 +1,15 @@ +//go:build windows +// +build windows + +package quic + +import ( + "errors" + + "golang.org/x/sys/windows" +) + +func isMsgSizeErr(err error) bool { + // https://docs.microsoft.com/en-us/windows/win32/winsock/windows-sockets-error-codes-2 + return errors.Is(err, windows.WSAEMSGSIZE) +} diff --git a/interface.go b/interface.go index 6b6b195f9d4..e054e38a5ba 100644 --- a/interface.go +++ b/interface.go @@ -290,7 +290,7 @@ type Config struct { KeepAlive bool // DisablePathMTUDiscovery disables Path MTU Discovery (RFC 8899). // Packets will then be at most 1252 (IPv4) / 1232 (IPv6) bytes in size. - // Note that Path MTU discovery is always disabled on Windows, see https://github.com/lucas-clemente/quic-go/issues/3273. + // Note that if Path MTU discovery is causing issues on your system, please open a new issue DisablePathMTUDiscovery bool // DisableVersionNegotiationPackets disables the sending of Version Negotiation packets. // This can be useful if version information is exchanged out-of-band. diff --git a/send_queue.go b/send_queue.go index bf25dded60e..1fc8c1bf893 100644 --- a/send_queue.go +++ b/send_queue.go @@ -64,7 +64,13 @@ func (h *sendQueue) Run() error { shouldClose = true case p := <-h.queue: if err := h.conn.Write(p.Data); err != nil { - return err + // This additional check enables: + // 1. Checking for "datagram too large" message from the kernel, as such, + // 2. Path MTU discovery,and + // 3. Eventual detection of loss PingFrame. + if !isMsgSizeErr(err) { + return err + } } p.Release() select { diff --git a/session.go b/session.go index ceeaa71a95f..4228543c92b 100644 --- a/session.go +++ b/session.go @@ -130,10 +130,6 @@ func (e *errCloseForRecreating) Error() string { var sessionTracingID uint64 // to be accessed atomically func nextSessionTracingID() uint64 { return atomic.AddUint64(&sessionTracingID, 1) } -func pathMTUDiscoveryEnabled(config *Config) bool { - return !disablePathMTUDiscovery && !config.DisablePathMTUDiscovery -} - // A Session is a QUIC session type session struct { // Destination connection ID used during the handshake. @@ -758,7 +754,7 @@ func (s *session) maybeResetTimer() { deadline = s.idleTimeoutStartTime().Add(s.idleTimeout) } } - if s.handshakeConfirmed && pathMTUDiscoveryEnabled(s.config) { + if s.handshakeConfirmed && !s.config.DisablePathMTUDiscovery { if probeTime := s.mtuDiscoverer.NextProbeTime(); !probeTime.IsZero() { deadline = utils.MinTime(deadline, probeTime) } @@ -822,7 +818,7 @@ func (s *session) handleHandshakeConfirmed() { s.sentPacketHandler.SetHandshakeConfirmed() s.cryptoStreamHandler.SetHandshakeConfirmed() - if pathMTUDiscoveryEnabled(s.config) { + if !s.config.DisablePathMTUDiscovery { maxPacketSize := s.peerParams.MaxUDPPayloadSize if maxPacketSize == 0 { maxPacketSize = protocol.MaxByteCount @@ -1783,7 +1779,7 @@ func (s *session) sendPacket() (bool, error) { s.sendQueue.Send(packet.buffer) return true, nil } - if pathMTUDiscoveryEnabled(s.config) && s.mtuDiscoverer.ShouldSendProbe(now) { + if !s.config.DisablePathMTUDiscovery && s.mtuDiscoverer.ShouldSendProbe(now) { packet, err := s.packer.PackMTUProbePacket(s.mtuDiscoverer.GetPing()) if err != nil { return false, err diff --git a/session_test.go b/session_test.go index 7ca60947f55..531828a0920 100644 --- a/session_test.go +++ b/session_test.go @@ -9,7 +9,6 @@ import ( "fmt" "io" "net" - "runtime" "runtime/pprof" "strings" "time" @@ -1682,35 +1681,33 @@ var _ = Describe("Session", func() { time.Sleep(50 * time.Millisecond) }) - if runtime.GOOS != "windows" { // Path MTU Discovery is disabled on Windows - It("sends a Path MTU probe packet", func() { - mtuDiscoverer := NewMockMtuDiscoverer(mockCtrl) - sess.mtuDiscoverer = mtuDiscoverer - sess.config.DisablePathMTUDiscovery = false - sph.EXPECT().SentPacket(gomock.Any()) - sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() - sph.EXPECT().SendMode().Return(ackhandler.SendAny) - sph.EXPECT().SendMode().Return(ackhandler.SendNone) - written := make(chan struct{}, 1) - sender.EXPECT().WouldBlock().AnyTimes() - sender.EXPECT().Send(gomock.Any()).DoAndReturn(func(p *packetBuffer) { written <- struct{}{} }) - gomock.InOrder( - mtuDiscoverer.EXPECT().NextProbeTime(), - mtuDiscoverer.EXPECT().ShouldSendProbe(gomock.Any()).Return(true), - mtuDiscoverer.EXPECT().NextProbeTime(), - ) - ping := ackhandler.Frame{Frame: &wire.PingFrame{}} - mtuDiscoverer.EXPECT().GetPing().Return(ping, protocol.ByteCount(1234)) - packer.EXPECT().PackMTUProbePacket(ping, protocol.ByteCount(1234)).Return(getPacket(1), nil) - go func() { - defer GinkgoRecover() - cryptoSetup.EXPECT().RunHandshake().MaxTimes(1) - sess.run() - }() - sess.scheduleSending() - Eventually(written).Should(Receive()) - }) - } + It("sends a Path MTU probe packet", func() { + mtuDiscoverer := NewMockMtuDiscoverer(mockCtrl) + sess.mtuDiscoverer = mtuDiscoverer + sess.config.DisablePathMTUDiscovery = false + sph.EXPECT().SentPacket(gomock.Any()) + sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() + sph.EXPECT().SendMode().Return(ackhandler.SendAny) + sph.EXPECT().SendMode().Return(ackhandler.SendNone) + written := make(chan struct{}, 1) + sender.EXPECT().WouldBlock().AnyTimes() + sender.EXPECT().Send(gomock.Any()).DoAndReturn(func(p *packetBuffer) { written <- struct{}{} }) + gomock.InOrder( + mtuDiscoverer.EXPECT().NextProbeTime(), + mtuDiscoverer.EXPECT().ShouldSendProbe(gomock.Any()).Return(true), + mtuDiscoverer.EXPECT().NextProbeTime(), + ) + ping := ackhandler.Frame{Frame: &wire.PingFrame{}} + mtuDiscoverer.EXPECT().GetPing().Return(ping, protocol.ByteCount(1234)) + packer.EXPECT().PackMTUProbePacket(ping, protocol.ByteCount(1234)).Return(getPacket(1), nil) + go func() { + defer GinkgoRecover() + cryptoSetup.EXPECT().RunHandshake().MaxTimes(1) + sess.run() + }() + sess.scheduleSending() + Eventually(written).Should(Receive()) + }) }) Context("scheduling sending", func() {