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;
+
 

[mmenke, 2014/09/29 17:49:34]

nit:  Remove extra blank line.

[Jana, 2014/09/29 23:01:46]

Done.

 // 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 69 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_ &&
       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);

[mmenke, 2014/09/29 17:49:34]

Previously, we didn't call RecordFastOpenStatus on synchronous read errors. 
That was a bug, right?

[Jana, 2014/09/29 23:01:46]

Yes, I believe so. That's why I moved it to HandleReadCompleted to ensure that
it was always called at the end of a read.

   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;

[mmenke, 2014/09/29 17:49:34]

This doesn't quite match the not-yet-failed case - it doesn't catch socket
reuses (Which is probably irrelevant), and does catch sockets that we never
actually call connect on.  I think this is coode enough, but maybe a comment
about it?

[Jana, 2014/09/29 23:01:47]

Yes, you're right on both accounts. I agree that it's reasonable to drop the
socket reuse cases (we're conditioning all of this on SSL sockets at this point
anyways), and I did think about doing it more accurately on a Write() call. I
estimated that this is reasonably adequate, especially since we're now only
measuring this for SSL sockets, and I expect that it should be uncommon for a
subsequent connect to not occur. I've added a comment to tcp_socket_libevent.h
and a TODO; PTAL.

 }
 
 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(rdsmith,jri): Change histogram name to indicate it could be called
+    // on error.
+    if (rv >= 0)
+      tcp_fastopen_connected_ = true;
+    else
+      g_tcp_fastopen_has_failed = true;
+    UpdateTCPFastOpenStatusAfterRead();

[mmenke, 2014/09/29 17:49:34]

Not relevant for this CL, but...  So if we ever get an error during connect,
like ERR_NO_ROUTE_TO_HOST, we disable fastopen until restart?

[mmenke, 2014/09/29 17:51:55]

Not relevant meaning I don't want to force a more complicated / smarter solution
in this CL, but I think we should think about it.

[Jana, 2014/09/29 23:01:46]

Yes we are more conservative than we need to be. We could condition turning TFO
off on more specific errors... I would also like to do optimizations to
re-enabling fastopen, such as re-enabling after a few hours, for instance.
Adding TODOs everywhere this flag is set to true to document this point. I've
also added a next step for filtering based on more specific write() errors. I
did think about this, and I think that data has to point the way here. I agree
that smarter machinery is in order, but we'll have to do this carefully to keep
the user from getting persistently blackholed.

+  }
+
   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.

[mmenke, 2014/09/29 17:49:34]

This comment seems to be making the assumption that it was the first write that
failed, but this isn't necessarily true.  We may end up writing to a socket
multiple times before we end up reading from it, and this code triggers as long
as we haven't completed a read yet (successfully or with error).  Do we care to
distinguish fails on the first write vs failures on a subsequent write?

Also, if an embedder tries to read from and write to a socket at once, and both
fail, the read error always takes precedence with this code, as long as the
socket is alive long enough, regardless of the order we learn about the errors
from.  Do we care?

[Jana, 2014/09/29 23:01:47]

Hmm... good point. I am comfortable, especially given the scope of our
experiment at this point (SSL only), to make this assumption. Either ways, this
seems to me to fall under the category of better resilience on failures with
FastOpen, and I'm comfortable leaving a note here to document this point.
There are other issues with an embedder attempting read before a write: the read
will fail on a TFO socket (this is the read-before-write issue on a TFO socket
that is yet unresolved, and a major reason for limiting this experiment to SSL).
That said, yes, I think the read() error is the one that I explicitly wanted to
give precedence to. I hadn't thought about the case you describe, but TFO
failure modes are better articulated on a read, since we get additional
information from the kernel there.

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

I should note I was wrong here.  UpdateTCPFastOpenStatusAfterRead checks
tcp_fastopen_status_, and does nothing on certain statuses.

+      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;
@@ skipping 15 matching lines @@
   // for system support on startup, but users may dynamically disable TCP Fast
   // Open 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 for
+    // unknown reasons. Record status and (conservatively) turn off
+    // TCP FastOpen for all subsequent connections.
+    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)) {
+    // 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);
 #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