[PATCH v2] tcp: Handle errors from tcp_send_flag()

[PATCH v2] tcp: Handle errors from tcp_send_flag()

[Anshu Kumari]

tcp_send_flag() can fail in two different ways:
   - tcp_prepare_flags() returns -ECONNRESET when getsockopt(TCP_INFO)
     fails: the socket is broken and the connection must be reset.
   - tcp_vu_send_flag() returns -EAGAIN when vu_collect() finds no
     available vhost-user buffers: this is a transient condition
     equivalent to a dropped packet on the wire.

Have tcp_vu_send_flag() return -EAGAIN instead of a bare -1 for the
buffer-unavailable case. Absorb -EAGAIN in the tcp_send_flag()
dispatcher so that callers only see fatal errors.

Check the return value at each call site and handle fatal errors:
   - in tcp_data_from_tap(), return -1 so the caller resets
   - in tcp_tap_handler(), goto reset
   - in tcp_timer_handler()/tcp_sock_handler()/tcp_conn_from_sock_finish(),
     call tcp_rst() and return
   - in tcp_tap_conn_from_sock(), set CLOSING flag, call
     FLOW_ACTIVATE() to avoid leaving the flow in TYPED state, and
     return
   - in tcp_connect_finish(), call tcp_rst() and return
   - in tcp_keepalive(), call tcp_rst() and continue the loop
   - in tcp_flow_migrate_target_ext(), goto fail

The call in tcp_rst_do() is left unchecked: we are already
resetting, and tcp_sock_rst() still needs to run regardless.

Link: https://bugs.passt.top/show_bug.cgi?id=194
Signed-off-by: Anshu Kumari <anskuma@redhat.com>
---
v2:
  - updated commit message.
  - update tcp_vu_send_flag() to return -EAGAIN instead of -1 for
    buffer-unavailable case.
  - in tcp_tap_conn_from_sock(), added FLOW_ACTIVATE call.
  - inside tcp_send_flag(), updated the return statement. function will
    return 0 incase of -EAGAIN error.
---
 tcp.c    | 74 +++++++++++++++++++++++++++++++++++++++++---------------
 tcp_vu.c |  6 +++--
 2 files changed, 59 insertions(+), 21 deletions(-)

diff --git a/tcp.c b/tcp.c
index 8ea9be8..28139d8 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1378,15 +1378,19 @@ int tcp_prepare_flags(const struct ctx *c, struct tcp_tap_conn *conn,
  * @conn:      Connection pointer
  * @flags:     TCP flags: if not set, send segment only if ACK is due
  *
- * Return: negative error code on connection reset, 0 otherwise
+ * Return: negative error code on fatal connection failure, 0 otherwise
  */
 static int tcp_send_flag(const struct ctx *c, struct tcp_tap_conn *conn,
 			 int flags)
 {
+	int ret;
+
 	if (c->mode == MODE_VU)
-		return tcp_vu_send_flag(c, conn, flags);
+		ret = tcp_vu_send_flag(c, conn, flags);
+	else
+		ret = tcp_buf_send_flag(c, conn, flags);
 
-	return tcp_buf_send_flag(c, conn, flags);
+	return ret == -EAGAIN ? 0 : ret;
 }
 
 /**
@@ -1917,7 +1921,9 @@ static int tcp_data_from_tap(const struct ctx *c, struct tcp_tap_conn *conn,
 				   "keep-alive sequence: %u, previous: %u",
 				   seq, conn->seq_from_tap);
 
-			tcp_send_flag(c, conn, ACK);
+			if (tcp_send_flag(c, conn, ACK))
+				return -1;
+
 			tcp_timer_ctl(c, conn);
 
 			if (setsockopt(conn->sock, SOL_SOCKET, SO_KEEPALIVE,
@@ -2043,14 +2049,16 @@ eintr:
 			 *   Then swiftly looked away and left.
 			 */
 			conn->seq_from_tap = seq_from_tap;
-			tcp_send_flag(c, conn, ACK);
+			if (tcp_send_flag(c, conn, ACK))
+				return -1;
 		}
 
 		if (errno == EINTR)
 			goto eintr;
 
 		if (errno == EAGAIN || errno == EWOULDBLOCK) {
-			tcp_send_flag(c, conn, ACK | DUP_ACK);
+			if (tcp_send_flag(c, conn, ACK | DUP_ACK))
+				return -1;
 			return p->count - idx;
 
 		}
@@ -2070,7 +2078,8 @@ out:
 		 */
 		if (conn->seq_dup_ack_approx != (conn->seq_from_tap & 0xff)) {
 			conn->seq_dup_ack_approx = conn->seq_from_tap & 0xff;
-			tcp_send_flag(c, conn, ACK | DUP_ACK);
+			if (tcp_send_flag(c, conn, ACK | DUP_ACK))
+				return -1;
 		}
 		return p->count - idx;
 	}
@@ -2084,7 +2093,8 @@ out:
 
 		conn_event(c, conn, TAP_FIN_RCVD);
 	} else {
-		tcp_send_flag(c, conn, ACK_IF_NEEDED);
+		if (tcp_send_flag(c, conn, ACK_IF_NEEDED))
+			return -1;
 	}
 
 	return p->count - idx;
@@ -2122,7 +2132,10 @@ static void tcp_conn_from_sock_finish(const struct ctx *c,
 		return;
 	}
 
-	tcp_send_flag(c, conn, ACK);
+	if (tcp_send_flag(c, conn, ACK)) {
+		tcp_rst(c, conn);
+		return;
+	}
 
 	/* The client might have sent data already, which we didn't
 	 * dequeue waiting for SYN,ACK from tap -- check now.
@@ -2308,7 +2321,9 @@ int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
 				goto reset;
 			}
 
-			tcp_send_flag(c, conn, ACK);
+			if (tcp_send_flag(c, conn, ACK))
+				goto reset;
+
 			conn_event(c, conn, SOCK_FIN_SENT);
 
 			return 1;
@@ -2388,7 +2403,9 @@ int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
 		}
 
 		conn_event(c, conn, SOCK_FIN_SENT);
-		tcp_send_flag(c, conn, ACK);
+		if (tcp_send_flag(c, conn, ACK))
+			goto reset;
+
 		ack_due = 0;
 
 		/* If we received a FIN, but the socket is in TCP_ESTABLISHED
@@ -2436,8 +2453,10 @@ static void tcp_connect_finish(const struct ctx *c, struct tcp_tap_conn *conn)
 		return;
 	}
 
-	if (tcp_send_flag(c, conn, SYN | ACK))
+	if (tcp_send_flag(c, conn, SYN | ACK)) {
+		tcp_rst(c, conn);
 		return;
+	}
 
 	conn_event(c, conn, TAP_SYN_ACK_SENT);
 	conn_flag(c, conn, ACK_FROM_TAP_DUE);
@@ -2478,7 +2497,12 @@ static void tcp_tap_conn_from_sock(const struct ctx *c, union flow *flow,
 
 	conn->wnd_from_tap = WINDOW_DEFAULT;
 
-	tcp_send_flag(c, conn, SYN);
+	if (tcp_send_flag(c, conn, SYN)) {
+		conn_flag(c, conn, CLOSING);
+		FLOW_ACTIVATE(conn);
+		return;
+	}
+
 	conn_flag(c, conn, ACK_FROM_TAP_DUE);
 
 	tcp_get_sndbuf(conn);
@@ -2585,7 +2609,10 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
 		return;
 
 	if (conn->flags & ACK_TO_TAP_DUE) {
-		tcp_send_flag(c, conn, ACK_IF_NEEDED);
+		if (tcp_send_flag(c, conn, ACK_IF_NEEDED)) {
+			tcp_rst(c, conn);
+			return;
+		}
 		tcp_timer_ctl(c, conn);
 	} else if (conn->flags & ACK_FROM_TAP_DUE) {
 		if (!(conn->events & ESTABLISHED)) {
@@ -2598,7 +2625,10 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
 				tcp_rst(c, conn);
 			} else {
 				flow_trace(conn, "SYN timeout, retry");
-				tcp_send_flag(c, conn, SYN);
+				if (tcp_send_flag(c, conn, SYN)) {
+					tcp_rst(c, conn);
+					return;
+				}
 				conn->retries++;
 				conn_flag(c, conn, SYN_RETRIED);
 				tcp_timer_ctl(c, conn);
@@ -2662,8 +2692,11 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref,
 			tcp_data_from_sock(c, conn);
 
 		if (events & EPOLLOUT) {
-			if (tcp_update_seqack_wnd(c, conn, false, NULL))
-				tcp_send_flag(c, conn, ACK);
+			if (tcp_update_seqack_wnd(c, conn, false, NULL) &&
+			    tcp_send_flag(c, conn, ACK)) {
+				tcp_rst(c, conn);
+				return;
+			}
 		}
 
 		return;
@@ -2903,7 +2936,8 @@ static void tcp_keepalive(struct ctx *c, const struct timespec *now)
 		if (conn->tap_inactive) {
 			flow_dbg(conn, "No tap activity for least %us, send keepalive",
 				 KEEPALIVE_INTERVAL);
-			tcp_send_flag(c, conn, KEEPALIVE);
+			if (tcp_send_flag(c, conn, KEEPALIVE))
+				tcp_rst(c, conn);
 		}
 
 		/* Ready to check fot next interval */
@@ -3926,7 +3960,9 @@ int tcp_flow_migrate_target_ext(struct ctx *c, struct tcp_tap_conn *conn, int fd
 	if (tcp_set_peek_offset(conn, peek_offset))
 		goto fail;
 
-	tcp_send_flag(c, conn, ACK);
+	if (tcp_send_flag(c, conn, ACK))
+		goto fail;
+
 	tcp_data_from_sock(c, conn);
 
 	if ((rc = tcp_epoll_ctl(conn))) {
diff --git a/tcp_vu.c b/tcp_vu.c
index dc0e17c..9d63a30 100644
--- a/tcp_vu.c
+++ b/tcp_vu.c
@@ -65,7 +65,9 @@ static size_t tcp_vu_hdrlen(bool v6)
  * @conn:	Connection pointer
  * @flags:	TCP flags: if not set, send segment only if ACK is due
  *
- * Return: negative error code on connection reset, 0 otherwise
+ * Return: -ECONNRESET on fatal connection error,
+ *         -EAGAIN if vhost-user buffers are unavailable,
+ *         0 otherwise
  */
 int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
 {
@@ -91,7 +93,7 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
 			      &flags_iov[0], 1, NULL,
 			      MAX(hdrlen + sizeof(*opts), ETH_ZLEN + VNET_HLEN), NULL);
 	if (elem_cnt != 1)
-		return -1;
+		return -EAGAIN;
 
 	assert(flags_elem[0].in_num == 1);
 	assert(flags_elem[0].in_sg[0].iov_len >=
-- 
2.53.0