[PATCH 0/3] Fix bugs with RST propagation

[PATCH 0/3] Fix bugs with RST propagation

[David Gibson]

In various circumstances we may not properly report abrupt ends to a
TCP connection to peers with an RST.  Fix them.

Link: https://bugs.passt.top/show_bug.cgi?id=191
Link: https://bugs.passt.top/show_bug.cgi?id=193

David Gibson (3):
  doc: Add test program verifying socket RST behaviour
  tcp: Properly propagate tap-side RST to socket side
  tcp_splice: Force TCP RST on abnormal close conditions

 doc/platform-requirements/.gitignore      |   1 +
 doc/platform-requirements/Makefile        |   8 +-
 doc/platform-requirements/tcp-close-rst.c | 204 ++++++++++++++++++++++
 tcp.c                                     |  37 +++-
 tcp_splice.c                              |  58 ++++--
 5 files changed, 290 insertions(+), 18 deletions(-)
 create mode 100644 doc/platform-requirements/tcp-close-rst.c

-- 
2.52.0

[PATCH 1/3] doc: Add test program verifying socket RST behaviour

[David Gibson]

Add a program to doc/platform-requirements testing / documenting the
RST related behaviour of TCP sockets under various conditions.

This doesn't fix bug 191 of itself, but documents the kernel side
behaviour we'll be using in order to fix it.

Link: https://bugs.passt.top/show_bug.cgi?id=191

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 doc/platform-requirements/.gitignore      |   1 +
 doc/platform-requirements/Makefile        |   8 +-
 doc/platform-requirements/tcp-close-rst.c | 204 ++++++++++++++++++++++
 3 files changed, 211 insertions(+), 2 deletions(-)
 create mode 100644 doc/platform-requirements/tcp-close-rst.c

diff --git a/doc/platform-requirements/.gitignore b/doc/platform-requirements/.gitignore
index f6272cf0..b2a0069a 100644
--- a/doc/platform-requirements/.gitignore
+++ b/doc/platform-requirements/.gitignore
@@ -1,4 +1,5 @@
 /listen-vs-repair
 /reuseaddr-priority
 /recv-zero
+/tcp-close-rst
 /udp-close-dup
diff --git a/doc/platform-requirements/Makefile b/doc/platform-requirements/Makefile
index 83930ef8..204341bb 100644
--- a/doc/platform-requirements/Makefile
+++ b/doc/platform-requirements/Makefile
@@ -3,8 +3,10 @@
 # Copyright Red Hat
 # Author: David Gibson <david@gibson.dropbear.id.au>
 
-TARGETS = reuseaddr-priority recv-zero udp-close-dup listen-vs-repair
-SRCS = reuseaddr-priority.c recv-zero.c udp-close-dup.c listen-vs-repair.c
+TARGETS = reuseaddr-priority recv-zero udp-close-dup listen-vs-repair \
+	tcp-close-rst
+SRCS = reuseaddr-priority.c recv-zero.c udp-close-dup.c listen-vs-repair.c \
+	tcp-close-rst.c
 CFLAGS = -Wall
 
 all: cppcheck clang-tidy $(TARGETS:%=check-%)
@@ -25,6 +27,7 @@ clang-tidy:
 	clang-tidy --checks=*,\
 	-altera-id-dependent-backward-branch,\
 	-altera-unroll-loops,\
+	-android-cloexec-accept,\
 	-bugprone-easily-swappable-parameters,\
 	-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling,\
 	-concurrency-mt-unsafe,\
@@ -37,6 +40,7 @@ clang-tidy:
 	-misc-include-cleaner,\
 	-modernize-macro-to-enum,\
 	-readability-braces-around-statements,\
+	-readability-function-cognitive-complexity,\
 	-readability-identifier-length,\
 	-readability-isolate-declaration \
 	$(SRCS)
diff --git a/doc/platform-requirements/tcp-close-rst.c b/doc/platform-requirements/tcp-close-rst.c
new file mode 100644
index 00000000..0e508f67
--- /dev/null
+++ b/doc/platform-requirements/tcp-close-rst.c
@@ -0,0 +1,204 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* tcp-close-rst.c
+ *
+ * Check what operations on a TCP socket will trigger an RST.
+ *
+ * Copyright Red Hat
+ * Author: David Gibson <david@gibson.dropbear.id.au>
+ */
+
+#include <arpa/inet.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <net/if.h>
+#include <netinet/in.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/socket.h>
+#include <unistd.h>
+
+#include "common.h"
+
+#define DSTPORT	13258U
+
+#define SRCADDR(n) \
+	(0x7f000000U | (n) << 16U | (n) << 8U | 0x1U)
+
+#define BASENUM	100
+
+/* 127.0.0.1:DSTPORT */
+static const struct sockaddr_in lo_dst = SOCKADDR_INIT(INADDR_LOOPBACK, DSTPORT);
+
+#define	LINGER		0x01U
+#define	SHUT_CLIENT	0x02U
+#define SHUT_SERVER	0x04U
+
+#define NUM_OPTIONS	(SHUT_SERVER << 1U)
+
+static void client_close(int sl, unsigned flags)
+{
+	struct sockaddr_in src = SOCKADDR_INIT(SRCADDR(flags), 0);
+	struct linger linger0 = {
+		.l_onoff = 1,
+		.l_linger = 0,
+	};
+	int sockerr, sc, sa;
+	socklen_t errlen = sizeof(sockerr);
+
+	printf("Client close %u:%s%s%s\n", flags,
+	       flags & LINGER ? " LINGER" : "",
+	       flags & SHUT_CLIENT ? " SHUT_CLIENT" : "",
+	       flags & SHUT_SERVER ? " SHUT_SERVER" : "");
+
+	sc = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+	if (sc < 0)
+		die("socket() for connect(): %s\n", strerror(errno));
+
+	if (bind(sc, (struct sockaddr *)&src, sizeof(src)) < 0)
+		die("bind() for connect: %s\n", strerror(errno));
+
+	if (connect(sc, (struct sockaddr *)&lo_dst, sizeof(lo_dst)) < 0)
+		die("connect(): %s\n", strerror(errno));
+
+	/* cppcheck-suppress [android-cloexec-accept,unmatchedSuppression] */
+	sa = accept(sl, NULL, NULL);
+	if (sa < 0)
+		die("accept(): %s\n", strerror(errno));
+
+	if (flags & SHUT_SERVER)
+		if (shutdown(sa, SHUT_WR) < 0)
+			die("shutdown() server: %s\n", strerror(errno));
+
+	if (flags & SHUT_CLIENT)
+		if (shutdown(sc, SHUT_WR) < 0)
+			die("shutdown() client: %s\n", strerror(errno));
+
+	if (flags & LINGER)
+		if (setsockopt(sc, SOL_SOCKET, SO_LINGER,
+			       &linger0, sizeof(linger0)) < 0)
+			die("SO_LINGER: %s\n", strerror(errno));
+
+	close(sc);
+
+	if (getsockopt(sa, SOL_SOCKET, SO_ERROR, &sockerr, &errlen) < 0)
+		die("SO_ERROR: %s\n", strerror(errno));
+
+	if (errlen != sizeof(sockerr))
+		die("SO_ERROR: bad option length\n");
+
+	printf("Server error: %s\n", strerror(sockerr));
+
+	if (flags & LINGER) {
+		if (!(flags & SHUT_SERVER) || !(flags & SHUT_CLIENT)) {
+			if (sockerr == 0)
+				die("No error after abrupt close(), no RST?\n");
+		} else {
+			if (sockerr != 0)
+				die("Error after full shutdown, bogus RST?\n");
+		}
+	}
+
+	close(sa);
+}
+
+static void server_close(int sl, unsigned flags)
+{
+	struct sockaddr_in src = SOCKADDR_INIT(SRCADDR(flags), 0);
+	struct linger linger0 = {
+		.l_onoff = 1,
+		.l_linger = 0,
+	};
+	int sockerr, sc, sa;
+	socklen_t errlen = sizeof(sockerr);
+
+	printf("Server close %u:%s%s%s\n", flags,
+	       flags & LINGER ? " LINGER" : "",
+	       flags & SHUT_CLIENT ? " SHUT_CLIENT" : "",
+	       flags & SHUT_SERVER ? " SHUT_SERVER" : "");
+
+	sc = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+	if (sc < 0)
+		die("socket() for connect(): %s\n", strerror(errno));
+
+	if (bind(sc, (struct sockaddr *)&src, sizeof(src)) < 0)
+		die("bind() for connect: %s\n", strerror(errno));
+
+	if (connect(sc, (struct sockaddr *)&lo_dst, sizeof(lo_dst)) < 0)
+		die("connect(): %s\n", strerror(errno));
+
+	/* cppcheck-suppress [android-cloexec-accept,unmatchedSuppression] */
+	sa = accept(sl, NULL, NULL);
+	if (sa < 0)
+		die("accept(): %s\n", strerror(errno));
+
+	if (flags & SHUT_SERVER)
+		if (shutdown(sa, SHUT_WR) < 0)
+			die("shutdown() server: %s\n", strerror(errno));
+
+	if (flags & SHUT_CLIENT)
+		if (shutdown(sc, SHUT_WR) < 0)
+			die("shutdown() client: %s\n", strerror(errno));
+
+	if (flags & LINGER)
+		if (setsockopt(sa, SOL_SOCKET, SO_LINGER,
+			       &linger0, sizeof(linger0)) < 0)
+			die("SO_LINGER: %s\n", strerror(errno));
+
+	close(sa);
+
+	if (getsockopt(sc, SOL_SOCKET, SO_ERROR, &sockerr, &errlen) < 0)
+		die("SO_ERROR: %s\n", strerror(errno));
+
+	if (errlen != sizeof(sockerr))
+		die("SO_ERROR: bad option length\n");
+
+	printf("Client error: %s\n", strerror(sockerr));
+
+	if (flags & LINGER) {
+		if (!(flags & SHUT_SERVER) || !(flags & SHUT_CLIENT)) {
+			if (sockerr == 0)
+				die("No error after abrupt close(), no RST?\n");
+		} else {
+			if (sockerr != 0)
+				die("Error after full shutdown, bogus RST?\n");
+		}
+	}
+
+	close(sc);
+}
+
+int main(int argc, char *argv[])
+{
+	unsigned flags;
+	int y = 1;
+	int sl;
+
+	(void)argc;
+	(void)argv;
+
+	sl = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+	if (sl < 0)
+		die("socket() for listen: %s\n", strerror(errno));
+
+	if (setsockopt(sl, SOL_SOCKET, SO_REUSEADDR, &y, sizeof(y)) < 0)
+		die("SO_REUSEADDR for listen: %s\n", strerror(errno));
+
+	if (bind(sl, (struct sockaddr *)&lo_dst, sizeof(lo_dst)) < 0)
+		die("bind() for listen: %s\n", strerror(errno));
+
+	if (listen(sl, 1) < 0)
+		die("listen(): %s\n", strerror(errno));
+
+	printf("Listening on port %u\n", DSTPORT);
+
+	for (flags = 0; flags < NUM_OPTIONS; flags++) {
+		client_close(sl, flags);
+		server_close(sl, flags);
+	}
+
+	close(sl);
+	exit(0);
+}
-- 
2.52.0

[PATCH 2/3] tcp: Properly propagate tap-side RST to socket side

[David Gibson]

When the guest sends a TCP RST, or on certain error conditions, we want to
signal the abnormal termination of a TCP connection to the peer with an
RST as well.  We attempt to do that by close()ing the socket.

That doesn't work: a close() will usually send a FIN, rather than an RST.
The standard method of forcing an RST on a socket is to set the SO_LINGER
socket option with a 0 timeout, then close().

Update the tcp_rst() path to do this, so it forces a socket side RST.
Update the handling of a guest side RST to use the same path (minus
sending a tap side RST) so that we properly propagate guest RSTs to the
peer.

Link: https://bugs.passt.top/show_bug.cgi?id=191

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp.c | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/tcp.c b/tcp.c
index 45dde5a0..9da37c2f 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1403,7 +1403,34 @@ static int tcp_send_flag(const struct ctx *c, struct tcp_tap_conn *conn,
 }
 
 /**
- * tcp_rst_do() - Reset a tap connection: send RST segment to tap, close socket
+ * tcp_sock_rst() - Close TCP connection forcing RST on socket side
+ * @c:		Execution context
+ * @conn:	Connection pointer
+ */
+static void tcp_sock_rst(const struct ctx *c, struct tcp_tap_conn *conn)
+{
+	const struct linger linger0 = {
+		.l_onoff = 1,
+		.l_linger = 0,
+	};
+
+	/* Force RST on socket to inform the peer
+	 *
+	 * We do this by setting SO_LINGER with 0 timeout, which means that
+	 * close() will send an RST (unless the connection is already closed in
+	 * both directions).
+	 */
+	if (setsockopt(conn->sock, SOL_SOCKET,
+		       SO_LINGER, &linger0, sizeof(linger0)) < 0) {
+		flow_dbg_perror(conn,
+				"SO_LINGER failed, may not send RST to peer");
+	}
+
+	conn_event(c, conn, CLOSED);
+}
+
+/**
+ * tcp_rst_do() - Reset a tap connection: send RST segment on both sides, close
  * @c:		Execution context
  * @conn:	Connection pointer
  */
@@ -1412,8 +1439,10 @@ void tcp_rst_do(const struct ctx *c, struct tcp_tap_conn *conn)
 	if (conn->events == CLOSED)
 		return;
 
+	/* Send RST on tap */
 	tcp_send_flag(c, conn, RST);
-	conn_event(c, conn, CLOSED);
+
+	tcp_sock_rst(c, conn);
 }
 
 /**
@@ -1884,7 +1913,7 @@ static int tcp_data_from_tap(const struct ctx *c, struct tcp_tap_conn *conn,
 			return -1;
 
 		if (th->rst) {
-			conn_event(c, conn, CLOSED);
+			tcp_sock_rst(c, conn);
 			return 1;
 		}
 
@@ -2248,7 +2277,7 @@ int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
 	flow_trace(conn, "packet length %zu from tap", l4len);
 
 	if (th->rst) {
-		conn_event(c, conn, CLOSED);
+		tcp_sock_rst(c, conn);
 		return 1;
 	}
 
-- 
2.52.0

[PATCH 3/3] tcp_splice: Force TCP RST on abnormal close conditions

[David Gibson]

When we need to prematurely close a spliced connection, we use:
    conn_flag(conn, CLOSING);
This does destroy the flow, but does so in the same way as a clean close
from both ends.  That's not what we want in error conditions, or when one
side of the flow has signalled an abnormal exit with an EPOLLHUP event.

Replace all places where we close the connection - except for the happy
path close - with calls to a new tcp_splice_rst() function, which forces
the sockets to emit a TCP RST on each side.

Link: https://bugs.passt.top/show_bug.cgi?id=193

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp_splice.c | 58 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 46 insertions(+), 12 deletions(-)

diff --git a/tcp_splice.c b/tcp_splice.c
index a7c04ca8..5f35c045 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -242,6 +242,37 @@ static void conn_event_do(struct tcp_splice_conn *conn, unsigned long event)
 		conn_event_do(conn, event);				\
 	} while (0)
 
+/**
+ * tcp_splice_rst() - Close spliced connection forcing RST on each side
+ * @conn:	Connection pointer
+ */
+static void tcp_splice_rst(struct tcp_splice_conn *conn)
+{
+	const struct linger linger0 = {
+		.l_onoff = 1,
+		.l_linger = 0,
+	};
+	unsigned sidei;
+
+	if (conn->flags & CLOSING)
+		return; /* Nothing to do */
+
+	/* Force RST on sockets to inform the peer
+	 *
+	 * We do this by setting SO_LINGER with 0 timeout, which means that
+	 * close() will send an RST (unless the connection is already closed in
+	 * both directions).
+	 */
+	flow_foreach_sidei(sidei) {
+		if (setsockopt(conn->s[sidei], SOL_SOCKET,
+			       SO_LINGER, &linger0, sizeof(linger0)) < 0) {
+			flow_dbg_perror(conn,
+"SO_LINGER failed, may not send RST to peer");
+		}
+	}
+
+	conn_flag(conn, CLOSING);
+}
 
 /**
  * tcp_splice_flow_defer() - Deferred per-flow handling (clean up closed)
@@ -307,7 +338,7 @@ static int tcp_splice_connect_finish(const struct ctx *c,
 			if (pipe2(conn->pipe[sidei], O_NONBLOCK | O_CLOEXEC)) {
 				flow_perror(conn, "cannot create %d->%d pipe",
 					    sidei, !sidei);
-				conn_flag(conn, CLOSING);
+				tcp_splice_rst(conn);
 				return -EIO;
 			}
 
@@ -437,7 +468,7 @@ void tcp_splice_conn_from_sock(const struct ctx *c, union flow *flow, int s0)
 		flow_trace(conn, "failed to set TCP_QUICKACK on %i", s0);
 
 	if (tcp_splice_connect(c, conn))
-		conn_flag(conn, CLOSING);
+		tcp_splice_rst(conn);
 
 	FLOW_ACTIVATE(conn);
 }
@@ -474,14 +505,14 @@ void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref,
 			flow_trace(conn, "Error event on socket: %s",
 				   strerror_(err));
 
-		goto close;
+		goto reset;
 	}
 
 	if (conn->events == SPLICE_CONNECT) {
 		if (!(events & EPOLLOUT))
-			goto close;
+			goto reset;
 		if (tcp_splice_connect_finish(c, conn))
-			goto close;
+			goto reset;
 	}
 
 	if (events & EPOLLOUT) {
@@ -515,7 +546,7 @@ retry:
 		while (readlen < 0 && errno == EINTR);
 
 		if (readlen < 0 && errno != EAGAIN)
-			goto close;
+			goto reset;
 
 		flow_trace(conn, "%zi from read-side call", readlen);
 
@@ -539,7 +570,7 @@ retry:
 		while (written < 0 && errno == EINTR);
 
 		if (written < 0 && errno != EAGAIN)
-			goto close;
+			goto reset;
 
 		flow_trace(conn, "%zi from write-side call (passed %zi)",
 			   written, c->tcp.pipe_size);
@@ -602,8 +633,11 @@ retry:
 		}
 	}
 
-	if (CONN_HAS(conn, FIN_SENT(0) | FIN_SENT(1)))
-		goto close;
+	if (CONN_HAS(conn, FIN_SENT(0) | FIN_SENT(1))) {
+		/* Clean close, no reset */
+		conn_flag(conn, CLOSING);
+		return;
+	}
 
 	if ((events & (EPOLLIN | EPOLLOUT)) == (EPOLLIN | EPOLLOUT)) {
 		events = EPOLLIN;
@@ -613,12 +647,12 @@ retry:
 	}
 
 	if (events & EPOLLHUP)
-		goto close;
+		goto reset;
 
 	return;
 
-close:
-	conn_flag(conn, CLOSING);
+reset:
+	tcp_splice_rst(conn);
 }
 
 /**
-- 
2.52.0

Re: [PATCH 2/3] tcp: Properly propagate tap-side RST to socket side

[Stefano Brivio]

On Tue, 27 Jan 2026 19:39:52 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> When the guest sends a TCP RST, or on certain error conditions, we want to
> signal the abnormal termination of a TCP connection to the peer with an
> RST as well.  We attempt to do that by close()ing the socket.
> 
> That doesn't work: a close() will usually send a FIN, rather than an RST.
> The standard method of forcing an RST on a socket is to set the SO_LINGER
> socket option with a 0 timeout, then close().
> 
> Update the tcp_rst() path to do this, so it forces a socket side RST.
> Update the handling of a guest side RST to use the same path (minus
> sending a tap side RST) so that we properly propagate guest RSTs to the
> peer.
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=191
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tcp.c | 37 +++++++++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 45dde5a0..9da37c2f 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1403,7 +1403,34 @@ static int tcp_send_flag(const struct ctx *c, struct tcp_tap_conn *conn,
>  }
>  
>  /**
> - * tcp_rst_do() - Reset a tap connection: send RST segment to tap, close socket
> + * tcp_sock_rst() - Close TCP connection forcing RST on socket side
> + * @c:		Execution context
> + * @conn:	Connection pointer
> + */
> +static void tcp_sock_rst(const struct ctx *c, struct tcp_tap_conn *conn)
> +{
> +	const struct linger linger0 = {
> +		.l_onoff = 1,
> +		.l_linger = 0,
> +	};
> +
> +	/* Force RST on socket to inform the peer
> +	 *
> +	 * We do this by setting SO_LINGER with 0 timeout, which means that
> +	 * close() will send an RST (unless the connection is already closed in
> +	 * both directions).
> +	 */
> +	if (setsockopt(conn->sock, SOL_SOCKET,
> +		       SO_LINGER, &linger0, sizeof(linger0)) < 0) {
> +		flow_dbg_perror(conn,
> +				"SO_LINGER failed, may not send RST to peer");
> +	}
> +
> +	conn_event(c, conn, CLOSED);
> +}
> +
> +/**
> + * tcp_rst_do() - Reset a tap connection: send RST segment on both sides, close
>   * @c:		Execution context
>   * @conn:	Connection pointer
>   */
> @@ -1412,8 +1439,10 @@ void tcp_rst_do(const struct ctx *c, struct tcp_tap_conn *conn)
>  	if (conn->events == CLOSED)
>  		return;
>  
> +	/* Send RST on tap */
>  	tcp_send_flag(c, conn, RST);
> -	conn_event(c, conn, CLOSED);
> +
> +	tcp_sock_rst(c, conn);
>  }
>  
>  /**
> @@ -1884,7 +1913,7 @@ static int tcp_data_from_tap(const struct ctx *c, struct tcp_tap_conn *conn,
>  			return -1;
>  
>  		if (th->rst) {
> -			conn_event(c, conn, CLOSED);
> +			tcp_sock_rst(c, conn);

The whole series looks good to me, except for one exceedingly minor
aspect: should we do this also in the getsockopt() error handling path
of tcp_prepare_flags()?

I would be inclined to apply it regardless of that, the fix is critical
enough. I'll start the usual test run in a few hours.

-- 
Stefano