Re: [PATCH v1] tcp: Handle errors from tcp_send_flag()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6533 bytes --]

On Fri, Apr 10, 2026 at 01:25:39PM +0530, Anshu Kumari wrote:
> tcp_send_flag() can return error codes from tcp_prepare_flags()
> failing TCP_INFO, or from failure to collect buffers on the
> vhost-user path. These errors indicate the connection requires
> resetting.
> 
> Most callers of tcp_send_flag() were ignoring the error code and
> carrying on as if nothing was wrong. Check the return value at
> each call site and handle the error appropriately:
>   - 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 (flow not yet active)
>   - in tcp_keepalive(), call tcp_rst() and continue the loop
>   - in tcp_flow_migrate_target_ext(), goto fail

Thanks for this detailed summary, it really helps a reviewer follow
the logic.

> The call in tcp_rst_do() is left unchecked: we are already
> resetting, and tcp_sock_rst() still needs to run regardless.
> 
> Bug: https://bugs.passt.top/show_bug.cgi?id=194
> Signed-off-by: Anshu Kumari <anskuma@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  tcp.c | 59 ++++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 44 insertions(+), 15 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 8ea9be8..9ce671a 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1917,7 +1917,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 +2045,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 +2074,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 +2089,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 +2128,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 +2317,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 +2399,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
> @@ -2478,7 +2491,11 @@ 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);
> +		return;
> +	}
> +
>  	conn_flag(c, conn, ACK_FROM_TAP_DUE);
>  
>  	tcp_get_sndbuf(conn);
> @@ -2585,7 +2602,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 +2618,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 +2685,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 +2929,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 +3953,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))) {
> -- 
> 2.53.0
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]