From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202602 header.b=UpDpWx6a; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 341EF5A0265 for ; Mon, 13 Apr 2026 08:22:58 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1776061374; bh=upArcnX1Jmm8YIUXiHIl7PBMZFJT6h8qNhUWKsioJEM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=UpDpWx6akDvBJ2F+rSNsYK0EWvH1PrGRKf4Wx5K+6Dr0WOh7sorpfROanARPZqmvS MIbXTrpxKfp+azMh1n+pySEaUvtEKSk9EQAk4QdvepI+A2c+uG5SBMrwHF4XxVN7M/ 3HdA/sx7kDawz35UPiwtj83Pn6IlqUnBKeREecPkYwhxLfzkvxb/mSfkhnTAkRsKVZ K/TWzzAL2FLo5XGRAMptYYcBNrDLDYvZ4lbFS0M0mJMqPltpIbjaSsm1moJayDZ/i1 riV00Mi2jx5EqDHimQpnEPmQnX8jH2U26l0K46wxW8hm77uEnj7gelEMArK6igvlKQ ZTjIyr/WfzOzw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4fvHQQ4DJPz4wL8; Mon, 13 Apr 2026 16:22:54 +1000 (AEST) Date: Mon, 13 Apr 2026 16:22:48 +1000 From: David Gibson To: Anshu Kumari Subject: Re: [PATCH v1] tcp: Handle errors from tcp_send_flag() Message-ID: References: <20260410075539.1566421-1-anskuma@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="PJdIuvJP9NuJL0KN" Content-Disposition: inline In-Reply-To: <20260410075539.1566421-1-anskuma@redhat.com> Message-ID-Hash: QBJDBDOGVY4BS5GA7B2CFHYQIQYVYAVI X-Message-ID-Hash: QBJDBDOGVY4BS5GA7B2CFHYQIQYVYAVI X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, sbrivio@redhat.com, lvivier@redhat.com X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --PJdIuvJP9NuJL0KN Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > Bug: https://bugs.passt.top/show_bug.cgi?id=3D194 > Signed-off-by: Anshu Kumari Reviewed-by: David Gibson > --- > tcp.c | 59 ++++++++++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 44 insertions(+), 15 deletions(-) >=20 > 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, s= truct tcp_tap_conn *conn, > "keep-alive sequence: %u, previous: %u", > seq, conn->seq_from_tap); > =20 > - tcp_send_flag(c, conn, ACK); > + if (tcp_send_flag(c, conn, ACK)) > + return -1; > + > tcp_timer_ctl(c, conn); > =20 > if (setsockopt(conn->sock, SOL_SOCKET, SO_KEEPALIVE, > @@ -2043,14 +2045,16 @@ eintr: > * Then swiftly looked away and left. > */ > conn->seq_from_tap =3D seq_from_tap; > - tcp_send_flag(c, conn, ACK); > + if (tcp_send_flag(c, conn, ACK)) > + return -1; > } > =20 > if (errno =3D=3D EINTR) > goto eintr; > =20 > if (errno =3D=3D EAGAIN || errno =3D=3D EWOULDBLOCK) { > - tcp_send_flag(c, conn, ACK | DUP_ACK); > + if (tcp_send_flag(c, conn, ACK | DUP_ACK)) > + return -1; > return p->count - idx; > =20 > } > @@ -2070,7 +2074,8 @@ out: > */ > if (conn->seq_dup_ack_approx !=3D (conn->seq_from_tap & 0xff)) { > conn->seq_dup_ack_approx =3D 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: > =20 > 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; > } > =20 > return p->count - idx; > @@ -2122,7 +2128,10 @@ static void tcp_conn_from_sock_finish(const struct= ctx *c, > return; > } > =20 > - tcp_send_flag(c, conn, ACK); > + if (tcp_send_flag(c, conn, ACK)) { > + tcp_rst(c, conn); > + return; > + } > =20 > /* 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 pi= f, sa_family_t af, > goto reset; > } > =20 > - tcp_send_flag(c, conn, ACK); > + if (tcp_send_flag(c, conn, ACK)) > + goto reset; > + > conn_event(c, conn, SOCK_FIN_SENT); > =20 > return 1; > @@ -2388,7 +2399,9 @@ int tcp_tap_handler(const struct ctx *c, uint8_t pi= f, sa_family_t af, > } > =20 > conn_event(c, conn, SOCK_FIN_SENT); > - tcp_send_flag(c, conn, ACK); > + if (tcp_send_flag(c, conn, ACK)) > + goto reset; > + > ack_due =3D 0; > =20 > /* 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 ct= x *c, union flow *flow, > =20 > conn->wnd_from_tap =3D WINDOW_DEFAULT; > =20 > - 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); > =20 > tcp_get_sndbuf(conn); > @@ -2585,7 +2602,10 @@ void tcp_timer_handler(const struct ctx *c, union = epoll_ref ref) > return; > =20 > 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 e= poll_ref ref, > tcp_data_from_sock(c, conn); > =20 > 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; > + } > } > =20 > return; > @@ -2903,7 +2929,8 @@ static void tcp_keepalive(struct ctx *c, const stru= ct 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); > } > =20 > /* Ready to check fot next interval */ > @@ -3926,7 +3953,9 @@ int tcp_flow_migrate_target_ext(struct ctx *c, stru= ct tcp_tap_conn *conn, int fd > if (tcp_set_peek_offset(conn, peek_offset)) > goto fail; > =20 > - tcp_send_flag(c, conn, ACK); > + if (tcp_send_flag(c, conn, ACK)) > + goto fail; > + > tcp_data_from_sock(c, conn); > =20 > if ((rc =3D tcp_epoll_ctl(conn))) { > --=20 > 2.53.0 >=20 --=20 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 --PJdIuvJP9NuJL0KN Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmnci6wACgkQzQJF27ox 2GegHA/+LeKGa1147Pz9Cuislk4ku/o6Wd7jn95D+K4AVbi5z5CmlqogkUyUNbBa CY+S1iGMy3DaI6sMQpmarmeqNPax1wA8znAd72qNqU5Ifl6thVh90ycK7oOmWiqX pCBpAoGuZfUfS0zivQnwsE081Wj6suXTfPQLHFUSFl4r3nE1hiHzQylneuQEqlTp j+GxmOYcOBXJv6rQkAzl1jK/rLfUmVEdP64JznpzU2eaEqXeS8Yr1pJKjqCIcniQ F7Zv29GyAeAsADuF7DzRvA0/pbvsYjUij5xM4EXxPm9Qi8hV940MLptgcIuKNWTJ GCIXQgJz+Bdv6hUGZTfrRpmIK0fqbMUK796E8+FP75Rz74sqsY0N/qB3FT4UsaqW utx6O7MLl/5Tdjx1M0uSJpvApZcXBp1gQ2JNzziQDDkTRgeIKGKgElNwK3saRvu+ o4ztCZdNqp0VW3ROXWPKM1VmCvSEK1pDJND1g952LOaTlqUr0b3gR/geXqd6GYxs G19Lx2LncvkexPxJSJBYM2n6qVdEzsLMVA7FCXUubqW7lZmnWFdhBhjIOe8mOkoj va7gg8HeB/w5Nm40mKzwPJvGwQd6B0b/5iG5nqBHZAIPIVySJFjVo9MJIHuq7rOO 9qzwcHyoKRDD4rixb7dbOaDhHszjj07BY1sCpL62vd3IclCY4jY= =SIUU -----END PGP SIGNATURE----- --PJdIuvJP9NuJL0KN--