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 --- 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 -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 + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#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

On Tue, 27 Jan 2026 19:39:52 +1100 David Gibson 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 --- 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