Adds TCP FastOpen blackhole recovery code to avoid getting persistently blackholed.

net/socket/tcp_socket_libevent.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/socket/tcp_socket.h"
 
 #include <errno.h>
 #include <netinet/tcp.h>
 #include <sys/socket.h>
 
@@ skipping 21 matching lines @@
 
 namespace net {
 
 namespace {
 
 // True if OS supports TCP FastOpen.
 bool g_tcp_fastopen_supported = false;
 // True if TCP FastOpen is user-enabled for all connections.
 // TODO(jri): Change global variable to param in HttpNetworkSession::Params.
 bool g_tcp_fastopen_user_enabled = false;
+// True if TCP FastOpen connect-with-write has failed at least once.
+bool g_tcp_fastopen_has_failed = false;
 
 // SetTCPNoDelay turns on/off buffering in the kernel. By default, TCP sockets
 // will wait up to 200ms for more data to complete a packet before transmitting.
 // After calling this function, the kernel will not wait. See TCP_NODELAY in
 // `man 7 tcp`.
 bool SetTCPNoDelay(int fd, bool no_delay) {
   int on = no_delay ? 1 : 0;
   int error = setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on));
   return error == 0;
 }
@@ skipping 70 matching lines @@
       base::WorkerPool::GetTaskRunner(/*task_is_slow=*/false).get(),
       FROM_HERE,
       base::Bind(SystemSupportsTCPFastOpen),
       base::Bind(RegisterTCPFastOpenIntentAndSupport, user_enabled));
 #endif
 }
 
 TCPSocketLibevent::TCPSocketLibevent(NetLog* net_log,
                                      const NetLog::Source& source)
     : use_tcp_fastopen_(false),
+      tcp_fastopen_write_attempted_(false),
       tcp_fastopen_connected_(false),
-      fast_open_status_(FAST_OPEN_STATUS_UNKNOWN),
+      tcp_fastopen_status_(TCP_FASTOPEN_STATUS_UNKNOWN),
       logging_multiple_connect_attempts_(false),
       net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_SOCKET)) {
   net_log_.BeginEvent(NetLog::TYPE_SOCKET_ALIVE,
                       source.ToEventParametersCallback());
 }
 
 TCPSocketLibevent::~TCPSocketLibevent() {
   net_log_.EndEvent(NetLog::TYPE_SOCKET_ALIVE);
-  if (tcp_fastopen_connected_) {
-    UMA_HISTOGRAM_ENUMERATION("Net.TcpFastOpenSocketConnection",
-                              fast_open_status_, FAST_OPEN_MAX_VALUE);
-  }
+  Close();
 }
 
 int TCPSocketLibevent::Open(AddressFamily family) {
   DCHECK(!socket_);
   socket_.reset(new SocketLibevent);
   int rv = socket_->Open(ConvertAddressFamily(family));
   if (rv != OK)
     socket_.reset();
   return rv;
 }
@@ skipping 59 matching lines @@
 
   net_log_.BeginEvent(NetLog::TYPE_TCP_CONNECT_ATTEMPT,
                       CreateNetLogIPEndPointCallback(&address));
 
   SockaddrStorage storage;
   if (!address.ToSockAddr(storage.addr, &storage.addr_len))
     return ERR_ADDRESS_INVALID;
 
   if (use_tcp_fastopen_) {
     // With TCP FastOpen, we pretend that the socket is connected.
-    DCHECK(!tcp_fastopen_connected_);
+    DCHECK(!tcp_fastopen_write_attempted_);
     socket_->SetPeerAddress(storage);
     return OK;
   }
 
   int rv = socket_->Connect(storage,
                             base::Bind(&TCPSocketLibevent::ConnectCompleted,
                                        base::Unretained(this), callback));
   if (rv != ERR_IO_PENDING)
     rv = HandleConnectCompleted(rv);
   return rv;
 }
 
 bool TCPSocketLibevent::IsConnected() const {
   if (!socket_)
     return false;
 
-  if (use_tcp_fastopen_ && !tcp_fastopen_connected_ &&
+  if (use_tcp_fastopen_ && !tcp_fastopen_write_attempted_ &&

[mmenke, 2014/09/30 14:30:34]

One more question:  tcp_fastopen_write_attempted_ is set to true on the first
write attempt, not when the first write (Which is also really the connection
attempt) completes.  Will socket_->IsConnected return true that soon?

[Jana, 2014/10/01 01:54:00]

Good question -- it took me some digging to figure this out. So the answer turns
out is yes. socket_->IsConnected() uses recv() and looks for EWOULDBLOCK to
indicate that the socket is connected. If our TCP FastOpen sendto() returns
success, then any subsequent read on a socket which has a TFO write in progress
returns EWOULDBLOCK, which means socket_->IsConnected returns true. If the
sendto() returns -1, with EINPROGRESS, that translates to IO_PENDING, which also
results in the same outcome on socket_->IsConnected. If the write fails
otherwise, then it's an error. So, as long as the Write() (connect) has started,
socket_->IsConnected() will return true. Does that answer your question?

       socket_->HasPeerAddress()) {
     // With TCP FastOpen, we pretend that the socket is connected.
     // This allows GetPeerAddress() to return peer_address_.
     return true;
   }
 
   return socket_->IsConnected();
 }
 
 bool TCPSocketLibevent::IsConnectedAndIdle() const {
   // TODO(wtc): should we also handle the TCP FastOpen case here,
   // as we do in IsConnected()?
   return socket_ && socket_->IsConnectedAndIdle();
 }
 
 int TCPSocketLibevent::Read(IOBuffer* buf,
                             int buf_len,
                             const CompletionCallback& callback) {
   DCHECK(socket_);
   DCHECK(!callback.is_null());
 
   int rv = socket_->Read(
       buf, buf_len,
       base::Bind(&TCPSocketLibevent::ReadCompleted,
                  // Grab a reference to |buf| so that ReadCompleted() can still
                  // use it when Read() completes, as otherwise, this transfers
                  // ownership of buf to socket.
                  base::Unretained(this), make_scoped_refptr(buf), callback));
-  if (rv >= 0)
-    RecordFastOpenStatus();
   if (rv != ERR_IO_PENDING)
     rv = HandleReadCompleted(buf, rv);
   return rv;
 }
 
 int TCPSocketLibevent::Write(IOBuffer* buf,
                              int buf_len,
                              const CompletionCallback& callback) {
   DCHECK(socket_);
   DCHECK(!callback.is_null());
 
   CompletionCallback write_callback =
       base::Bind(&TCPSocketLibevent::WriteCompleted,
                  // Grab a reference to |buf| so that WriteCompleted() can still
                  // use it when Write() completes, as otherwise, this transfers
                  // ownership of buf to socket.
                  base::Unretained(this), make_scoped_refptr(buf), callback);
   int rv;
-  if (use_tcp_fastopen_ && !tcp_fastopen_connected_) {
+
+  if (use_tcp_fastopen_ && !tcp_fastopen_write_attempted_) {
     rv = TcpFastOpenWrite(buf, buf_len, write_callback);
   } else {
     rv = socket_->Write(buf, buf_len, write_callback);
   }
 
   if (rv != ERR_IO_PENDING)
     rv = HandleWriteCompleted(buf, rv);
   return rv;
 }
 
@@ skipping 105 matching lines @@
   return SetTCPKeepAlive(socket_->socket_fd(), enable, delay);
 }
 
 bool TCPSocketLibevent::SetNoDelay(bool no_delay) {
   DCHECK(socket_);
   return SetTCPNoDelay(socket_->socket_fd(), no_delay);
 }
 
 void TCPSocketLibevent::Close() {
   socket_.reset();
+
+  // Record and reset TCP FastOpen state.
+  if (tcp_fastopen_write_attempted_ ||
+      tcp_fastopen_status_ == TCP_FASTOPEN_PREVIOUSLY_FAILED) {
+    UMA_HISTOGRAM_ENUMERATION("Net.TcpFastOpenSocketConnection",
+                              tcp_fastopen_status_, TCP_FASTOPEN_MAX_VALUE);
+  }
+  use_tcp_fastopen_ = false;
   tcp_fastopen_connected_ = false;
-  fast_open_status_ = FAST_OPEN_STATUS_UNKNOWN;
+  tcp_fastopen_write_attempted_ = false;
+  tcp_fastopen_status_ = TCP_FASTOPEN_STATUS_UNKNOWN;
 }
 
 bool TCPSocketLibevent::UsingTCPFastOpen() const {
   return use_tcp_fastopen_;
 }
 
 void TCPSocketLibevent::EnableTCPFastOpenIfSupported() {
-  if (IsTCPFastOpenSupported())
+  if (!IsTCPFastOpenSupported())
+    return;
+
+  // Do not enable TCP FastOpen if it had previously failed.
+  // This check conservatively avoids middleboxes that may blackhole
+  // TCP FastOpen SYN+Data packets; on such a failure, subsequent sockets
+  // should not use TCP FastOpen.
+  if(!g_tcp_fastopen_has_failed)
     use_tcp_fastopen_ = true;
+  else
+    tcp_fastopen_status_ = TCP_FASTOPEN_PREVIOUSLY_FAILED;
 }
 
 bool TCPSocketLibevent::IsValid() const {
   return socket_ != NULL && socket_->socket_fd() != kInvalidSocket;
 }
 
 void TCPSocketLibevent::StartLoggingMultipleConnectAttempts(
     const AddressList& addresses) {
   if (!logging_multiple_connect_attempts_) {
     logging_multiple_connect_attempts_ = true;
@@ skipping 108 matching lines @@
 
   net_log_.EndEvent(NetLog::TYPE_TCP_CONNECT,
                     CreateNetLogSourceAddressCallback(storage.addr,
                                                       storage.addr_len));
 }
 
 void TCPSocketLibevent::ReadCompleted(const scoped_refptr<IOBuffer>& buf,
                                       const CompletionCallback& callback,
                                       int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
-  // Records TCP FastOpen status regardless of error in asynchronous case.
-  // TODO(rdsmith,jri): Change histogram name to indicate it could be called on
-  // error.
-  RecordFastOpenStatus();
   callback.Run(HandleReadCompleted(buf.get(), rv));
 }
 
 int TCPSocketLibevent::HandleReadCompleted(IOBuffer* buf, int rv) {
+  if (tcp_fastopen_write_attempted_ && !tcp_fastopen_connected_) {
+    // A TCP FastOpen connect-with-write was attempted. This read was a
+    // subsequent read, which either succeeded or failed. If the read
+    // succeeded, the socket is considered connected via TCP FastOpen.
+    // If the read failed, TCP FastOpen is (conservatively) turned off for all
+    // subsequent connections. TCP FastOpen status is recorded in both cases.
+    // TODO (jri): This currently results in conservative behavior, where TCP
+    // FastOpen is turned off on _any_ error. Implement optimizations,
+    // such as turning off TCP FastOpen on more specific errors, and
+    // re-attempting TCP FastOpen after a certain amount of time has passed.
+    if (rv >= 0)

[mmenke, 2014/09/30 14:30:34]

Is 0 a reliable indicator of success here?

[Jana, 2014/10/01 01:54:00]

I think so. 0 indicates a close, not an error, which is what should happen on
any (including TFO) failure. As I understand it, if a TFO connect fails
underneath, read returns -1 with errno set to ECONNREFUSED.

[mmenke, 2014/10/01 02:11:12]

Right, my concern was just that a confused server could somehow managed to make
this look like a graceful close...  Can't really see how that would happen,
though.

+      tcp_fastopen_connected_ = true;
+    else
+      g_tcp_fastopen_has_failed = true;
+    UpdateTCPFastOpenStatusAfterRead();
+  }
+
   if (rv < 0) {
     net_log_.AddEvent(NetLog::TYPE_SOCKET_READ_ERROR,
                       CreateNetLogSocketErrorCallback(rv, errno));
     return rv;
   }
 
   base::StatsCounter read_bytes("tcp.read_bytes");
   read_bytes.Add(rv);
   net_log_.AddByteTransferEvent(NetLog::TYPE_SOCKET_BYTES_RECEIVED, rv,
                                 buf->data());
   return rv;
 }
 
 void TCPSocketLibevent::WriteCompleted(const scoped_refptr<IOBuffer>& buf,
                                        const CompletionCallback& callback,
-                                       int rv) const {
+                                       int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
   callback.Run(HandleWriteCompleted(buf.get(), rv));
 }
 
-int TCPSocketLibevent::HandleWriteCompleted(IOBuffer* buf, int rv) const {
+int TCPSocketLibevent::HandleWriteCompleted(IOBuffer* buf, int rv) {
   if (rv < 0) {
+    if (tcp_fastopen_write_attempted_ && !tcp_fastopen_connected_) {
+      // TCP FastOpen connect-with-write was attempted, and the write failed
+      // for unknown reasons. Record status and (conservatively) turn off
+      // TCP FastOpen for all subsequent connections.
+      // TODO (jri): This currently results in conservative behavior, where TCP
+      // FastOpen is turned off on _any_ error. Implement optimizations,
+      // such as turning off TCP FastOpen on more specific errors, and
+      // re-attempting TCP FastOpen after a certain amount of time has passed.
+      tcp_fastopen_status_ = TCP_FASTOPEN_ERROR;
+      g_tcp_fastopen_has_failed = true;
+    }
     net_log_.AddEvent(NetLog::TYPE_SOCKET_WRITE_ERROR,
                       CreateNetLogSocketErrorCallback(rv, errno));
     return rv;
   }
 
   base::StatsCounter write_bytes("tcp.write_bytes");
   write_bytes.Add(rv);
   net_log_.AddByteTransferEvent(NetLog::TYPE_SOCKET_BYTES_SENT, rv,
                                 buf->data());
   return rv;
 }
 
 int TCPSocketLibevent::TcpFastOpenWrite(
     IOBuffer* buf,
     int buf_len,
     const CompletionCallback& callback) {
   SockaddrStorage storage;
   int rv = socket_->GetPeerAddress(&storage);
   if (rv != OK)
     return rv;
 
   int flags = 0x20000000;  // Magic flag to enable TCP_FASTOPEN.
 #if defined(OS_LINUX)
-  // sendto() will fail with EPIPE when the system doesn't support TCP Fast
-  // Open. Theoretically that shouldn't happen since the caller should check
-  // for system support on startup, but users may dynamically disable TCP Fast
-  // Open via sysctl.
+  // sendto() will fail with EPIPE when the system doesn't implement TCP
+  // FastOpen, and with EOPNOTSUPP when the system implements TCP FastOpen
+  // but it is disabled. Theoretically these shouldn't happen
+  // since the caller should check for system support on startup, but
+  // users may dynamically disable TCP FastOpen via sysctl.
   flags |= MSG_NOSIGNAL;
 #endif // defined(OS_LINUX)
   rv = HANDLE_EINTR(sendto(socket_->socket_fd(),
                            buf->data(),
                            buf_len,
                            flags,
                            storage.addr,
                            storage.addr_len));
-  tcp_fastopen_connected_ = true;
+  tcp_fastopen_write_attempted_ = true;
 
   if (rv >= 0) {
-    fast_open_status_ = FAST_OPEN_FAST_CONNECT_RETURN;
+    tcp_fastopen_status_ = TCP_FASTOPEN_FAST_CONNECT_RETURN;
     return rv;
   }
 
   DCHECK_NE(EPIPE, errno);
 
   // If errno == EINPROGRESS, that means the kernel didn't have a cookie
   // and would block. The kernel is internally doing a connect() though.
   // Remap EINPROGRESS to EAGAIN so we treat this the same as our other
   // asynchronous cases. Note that the user buffer has not been copied to
   // kernel space.
   if (errno == EINPROGRESS) {
     rv = ERR_IO_PENDING;
   } else {
     rv = MapSystemError(errno);
   }
 
   if (rv != ERR_IO_PENDING) {
-    fast_open_status_ = FAST_OPEN_ERROR;
+    // TCP FastOpen connect-with-write was attempted, and the write failed
+    // since TCP FastOpen was not implemented or disabled in the OS.
+    // Record status and turn off TCP FastOpen for all subsequent connections.
+    // TODO (jri): This is almost certainly too conservative, since it blanket
+    // turns off TCP FastOpen on any write error. Two things need to be done
+    // here: (i) record a histogram of write errors; in particular, record
+    // occurrences of EOPNOTSUPP and EPIPE, and (ii) afterwards, consider
+    // turning off TCP FastOpen on more specific errors.
+    tcp_fastopen_status_ = TCP_FASTOPEN_ERROR;
+    g_tcp_fastopen_has_failed = true;
     return rv;
   }
 
-  fast_open_status_ = FAST_OPEN_SLOW_CONNECT_RETURN;
+  tcp_fastopen_status_ = TCP_FASTOPEN_SLOW_CONNECT_RETURN;
   return socket_->WaitForWrite(buf, buf_len, callback);
 }
 
-void TCPSocketLibevent::RecordFastOpenStatus() {
-  if (use_tcp_fastopen_ &&
-      (fast_open_status_ == FAST_OPEN_FAST_CONNECT_RETURN ||
-       fast_open_status_ == FAST_OPEN_SLOW_CONNECT_RETURN)) {
-    DCHECK_NE(FAST_OPEN_STATUS_UNKNOWN, fast_open_status_);
-    bool getsockopt_success(false);
-    bool server_acked_data(false);
+void TCPSocketLibevent::UpdateTCPFastOpenStatusAfterRead() {
+  if (!use_tcp_fastopen_ ||
+      (tcp_fastopen_status_ != TCP_FASTOPEN_FAST_CONNECT_RETURN &&
+       tcp_fastopen_status_ != TCP_FASTOPEN_SLOW_CONNECT_RETURN)) {

[mmenke, 2014/09/30 14:30:34]

I think it's a little weird that here we check tcp_fastopen_status_, but in
HandleReadCompleted we check tcp_fastopen_connected_.  In both places, we're
largely interested in the same thing.

Do we even need the tcp_fastopen_status_ check there?

[Jana, 2014/10/01 01:54:00]

We don't need the fastopen status check here at all... since we shouldn't be
calling this method unless we're in the ReadCompleted call with
tcp_fastopen_write_attempted && !tcp_fastopen_connected. This was leftover code
from earlier that I didn't clean up... I think its safe now to remove this, so
I'm removing it, and adding a DCHECK.

[mmenke, 2014/10/01 02:11:12]

Two read failures in a row could get us here...But yea, after one read failure,
no one should be reading from us again.

+    // Not using TCP FastOpen, or status has been finalized.
+    return;
+  }
+  DCHECK_NE(TCP_FASTOPEN_STATUS_UNKNOWN, tcp_fastopen_status_);
+
+  if (tcp_fastopen_write_attempted_ && !tcp_fastopen_connected_) {
+    // TCP FastOpen connect-with-write was attempted, and failed.
+    tcp_fastopen_status_ =
+        (tcp_fastopen_status_ == TCP_FASTOPEN_FAST_CONNECT_RETURN ?
+            TCP_FASTOPEN_FAST_CONNECT_READ_FAILED :
+            TCP_FASTOPEN_SLOW_CONNECT_READ_FAILED);
+    return;
+  }
+
+  bool getsockopt_success(false);
+  bool server_acked_data(false);

[mmenke, 2014/09/30 14:30:34]

nit:  While you're here, "=" is almost always used to initialize fundamental
types and enums

[Jana, 2014/10/01 01:54:00]

Done.

 #if defined(TCP_INFO)
-    // Probe to see the if the socket used TCP FastOpen.
-    tcp_info info;
-    socklen_t info_len = sizeof(tcp_info);
-    getsockopt_success =
-        getsockopt(socket_->socket_fd(), IPPROTO_TCP, TCP_INFO,
-                   &info, &info_len) == 0 &&
-        info_len == sizeof(tcp_info);
-    server_acked_data = getsockopt_success &&
-        (info.tcpi_options & TCPI_OPT_SYN_DATA);
+  // Probe to see the if the socket used TCP FastOpen.
+  tcp_info info;
+  socklen_t info_len = sizeof(tcp_info);
+  getsockopt_success = getsockopt(socket_->socket_fd(), IPPROTO_TCP, TCP_INFO,
+                                  &info, &info_len) == 0 &&
+                       info_len == sizeof(tcp_info);
+  server_acked_data = getsockopt_success &&
+                      (info.tcpi_options & TCPI_OPT_SYN_DATA);
 #endif
-    if (getsockopt_success) {
-      if (fast_open_status_ == FAST_OPEN_FAST_CONNECT_RETURN) {
-        fast_open_status_ = (server_acked_data ? FAST_OPEN_SYN_DATA_ACK :
-                             FAST_OPEN_SYN_DATA_NACK);
-      } else {
-        fast_open_status_ = (server_acked_data ? FAST_OPEN_NO_SYN_DATA_ACK :
-                             FAST_OPEN_NO_SYN_DATA_NACK);
-      }
+
+  if (getsockopt_success) {
+    if (tcp_fastopen_status_ == TCP_FASTOPEN_FAST_CONNECT_RETURN) {
+      tcp_fastopen_status_ = (server_acked_data ?
+                              TCP_FASTOPEN_SYN_DATA_ACK :
+                              TCP_FASTOPEN_SYN_DATA_NACK);
     } else {
-      fast_open_status_ = (fast_open_status_ == FAST_OPEN_FAST_CONNECT_RETURN ?
-                           FAST_OPEN_SYN_DATA_FAILED :
-                           FAST_OPEN_NO_SYN_DATA_FAILED);
+      tcp_fastopen_status_ = (server_acked_data ?
+                              TCP_FASTOPEN_NO_SYN_DATA_ACK :
+                              TCP_FASTOPEN_NO_SYN_DATA_NACK);
     }
+  } else {
+    tcp_fastopen_status_ =
+        (tcp_fastopen_status_ == TCP_FASTOPEN_FAST_CONNECT_RETURN ?
+         TCP_FASTOPEN_SYN_DATA_GETSOCKOPT_FAILED :
+         TCP_FASTOPEN_NO_SYN_DATA_GETSOCKOPT_FAILED);
   }
 }
 
 }  // namespace net