From f728041258c56a627eadd020cd286ff66d464e36 Mon Sep 17 00:00:00 2001 From: Zeling Feng Date: Mon, 2 May 2022 18:28:44 -0700 Subject: [PATCH] Do not enable keepalive timer if it was cleaned up Keepalive timers are cleaned up when the socket is closed or reset. But the user can still call setsocketopt(SO_KEEPALIVE) on the closed socket, not checking if the timer is cleaned up will cause panic. This happened when a server tries to set SO_KEEPALIVE for any incoming connection and there is a port scanner in the network which resets the connection immediately after the handshake. PiperOrigin-RevId: 446080773 --- pkg/tcpip/transport/tcp/connect.go | 9 +++-- pkg/tcpip/transport/tcp/endpoint.go | 7 ++-- pkg/tcpip/transport/tcp/endpoint_state.go | 4 ++- test/syscalls/linux/socket_ip_tcp_generic.cc | 35 ++++++++++++++++++++ 4 files changed, 50 insertions(+), 5 deletions(-) diff --git a/pkg/tcpip/transport/tcp/connect.go b/pkg/tcpip/transport/tcp/connect.go index ff20a1478..a86793175 100644 --- a/pkg/tcpip/transport/tcp/connect.go +++ b/pkg/tcpip/transport/tcp/connect.go @@ -1328,6 +1328,13 @@ func (e *endpoint) keepaliveTimerExpired() tcpip.Error { // whether it is enabled for this endpoint. func (e *endpoint) resetKeepaliveTimer(receivedData bool) { e.keepalive.Lock() + defer e.keepalive.Unlock() + if e.keepalive.timer.isZero() { + if state := e.EndpointState(); !state.closed() { + panic(fmt.Sprintf("Unexpected state when the keepalive time is cleaned up, got %s, want %s or %s", state, StateClose, StateError)) + } + return + } if receivedData { e.keepalive.unacked = 0 } @@ -1335,7 +1342,6 @@ func (e *endpoint) resetKeepaliveTimer(receivedData bool) { // data to send. if !e.SocketOptions().GetKeepAlive() || e.snd == nil || e.snd.SndUna != e.snd.SndNxt { e.keepalive.timer.disable() - e.keepalive.Unlock() return } if e.keepalive.unacked > 0 { @@ -1343,7 +1349,6 @@ func (e *endpoint) resetKeepaliveTimer(receivedData bool) { } else { e.keepalive.timer.enable(e.keepalive.idle) } - e.keepalive.Unlock() } // disableKeepaliveTimer stops the keepalive timer. diff --git a/pkg/tcpip/transport/tcp/endpoint.go b/pkg/tcpip/transport/tcp/endpoint.go index c59a49793..42b6c8be6 100644 --- a/pkg/tcpip/transport/tcp/endpoint.go +++ b/pkg/tcpip/transport/tcp/endpoint.go @@ -801,8 +801,9 @@ type keepalive struct { interval time.Duration count int unacked int - timer timer `state:"nosave"` - waker sleep.Waker `state:"nosave"` + // should never be a zero timer if the endpoint is not closed. + timer timer `state:"nosave"` + waker sleep.Waker `state:"nosave"` } func newEndpoint(s *stack.Stack, protocol *protocol, netProto tcpip.NetworkProtocolNumber, waiterQueue *waiter.Queue) *endpoint { @@ -879,6 +880,8 @@ func newEndpoint(s *stack.Stack, protocol *protocol, netProto tcpip.NetworkProto e.segmentQueue.ep = e + // TODO(https://gvisor.dev/issues/7493): Defer creating the timer until TCP connection becomes + // established. e.keepalive.timer.init(e.stack.Clock(), maybeFailTimerHandler(e, e.keepaliveTimerExpired)) return e diff --git a/pkg/tcpip/transport/tcp/endpoint_state.go b/pkg/tcpip/transport/tcp/endpoint_state.go index 38f7b50b0..4e48de8e8 100644 --- a/pkg/tcpip/transport/tcp/endpoint_state.go +++ b/pkg/tcpip/transport/tcp/endpoint_state.go @@ -124,8 +124,10 @@ func (e *endpoint) afterLoad() { // Resume implements tcpip.ResumableEndpoint.Resume. func (e *endpoint) Resume(s *stack.Stack) { - if snd := e.snd; snd != nil { + if !e.EndpointState().closed() { e.keepalive.timer.init(s.Clock(), maybeFailTimerHandler(e, e.keepaliveTimerExpired)) + } + if snd := e.snd; snd != nil { snd.resendTimer.init(s.Clock(), maybeFailTimerHandler(e, e.snd.retransmitTimerExpired)) snd.reorderTimer.init(s.Clock(), timerHandler(e, e.snd.rc.reorderTimerExpired)) snd.probeTimer.init(s.Clock(), timerHandler(e, e.snd.probeTimerExpired)) diff --git a/test/syscalls/linux/socket_ip_tcp_generic.cc b/test/syscalls/linux/socket_ip_tcp_generic.cc index 5b1db3ae9..3fc647bc3 100644 --- a/test/syscalls/linux/socket_ip_tcp_generic.cc +++ b/test/syscalls/linux/socket_ip_tcp_generic.cc @@ -515,6 +515,41 @@ TEST_P(TCPSocketPairTest, SetSoKeepalive) { EXPECT_EQ(get, kSockOptOff); } +TEST_P(TCPSocketPairTest, SetSoKeepaliveClosed) { + auto sockets = ASSERT_NO_ERRNO_AND_VALUE(NewSocketPair()); + + // Force a RST to be sent using SO_LINGER. + auto linger_opt = linger{.l_onoff = 1, .l_linger = 0}; + ASSERT_THAT(setsockopt(sockets->second_fd(), SOL_SOCKET, SO_LINGER, + &linger_opt, sizeof(linger_opt)), + SyscallSucceeds()); + ASSERT_THAT(close(sockets->release_second_fd()), SyscallSucceeds()); + + // Wait for the other end to receive the RST (up to 20 seconds). + constexpr int kPollTimeoutMs = 20000; + auto pfd = pollfd{ + .fd = sockets->first_fd(), + .events = POLLIN | POLLHUP, + .revents = 0, + }; + ASSERT_THAT(RetryEINTR(poll)(&pfd, 1, kPollTimeoutMs), + SyscallSucceedsWithValue(1)); + ASSERT_EQ(pfd.revents & POLLHUP, POLLHUP); + + // Now that the connection is closed, we should still be able to set + // SO_KEEPALIVE. + ASSERT_THAT(setsockopt(sockets->first_fd(), SOL_SOCKET, SO_KEEPALIVE, + &kSockOptOn, sizeof(kSockOptOn)), + SyscallSucceeds()); + int get = -1; + socklen_t get_len = sizeof(get); + ASSERT_THAT( + getsockopt(sockets->first_fd(), SOL_SOCKET, SO_KEEPALIVE, &get, &get_len), + SyscallSucceeds()); + ASSERT_EQ(get, kSockOptOn); + ASSERT_EQ(get_len, sizeof(get)); +} + TEST_P(TCPSocketPairTest, TCPKeepidleDefault) { auto sockets = ASSERT_NO_ERRNO_AND_VALUE(NewSocketPair());