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=202502 header.b=DjSvE8ft; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id CFF2D5A0272 for ; Thu, 27 Feb 2025 06:55:35 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202502; t=1740635720; bh=6wo6QliIKW3D/zuSkHvf9Rch+H7rmeXnnPM1iz0qtlg=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=DjSvE8ftoD4hakoFfMjATCxE3fegxUUXcwnthqXjKghwQnmOSz2YD/vJrI/Nrr9ql NWnWCAKB7/NJ2ATesRLUiOK+Q8q7S7vrkyxCl2qLMDjVb8Cb74NVC/BIw7n2zHSmxT sgLeEUE6CIDKsKyL7AL3pKGa8ENdiDKlub45nMqTGSOGjOnhI6cEaQ9mVMPZ3NDAiu noC44+snmRiSlaukY11MflRuYpolAlSzLZzmIcYQ53gdbUjnRWHrVo6J4TVxh6CSfX xMCzaae0omh9f6QG2YdM3+s+jSPX7cenZulkJKl+H5Lw2z/6EJ6PEY6fpvHwfxSOuk Mo/fk7I9SozpQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Z3LCr0qPVz4x3p; Thu, 27 Feb 2025 16:55:20 +1100 (AEDT) From: David Gibson To: Stefano Brivio , passt-dev@passt.top Subject: [PATCH v4 5/5] migrate, tcp: Don't flow_alloc_cancel() during incoming migration Date: Thu, 27 Feb 2025 16:55:17 +1100 Message-ID: <20250227055517.497347-6-david@gibson.dropbear.id.au> X-Mailer: git-send-email 2.48.1 In-Reply-To: <20250227055517.497347-1-david@gibson.dropbear.id.au> References: <20250227055517.497347-1-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: QNA22NCADX466DKUGG6O2YRT2BQ735G2 X-Message-ID-Hash: QNA22NCADX466DKUGG6O2YRT2BQ735G2 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: David Gibson 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: In tcp_flow_migrate_target(), if we're unable to create and bind the new socket, we print an error, cancel the flow and carry on. This seems to make sense based on our policy of generally letting the migration complete even if some or all flows are lost in the process. But it doesn't quite work: the flow_alloc_cancel() means that the flows in the target's flow table are no longer one to one match to the flows which the source is sending data for. This means that data for later flows will be mismatched to a different flow. Most likely that will cause some nasty error later, but even worse it might appear to succeed but lead to data corruption due to incorrectly restoring one of the flows. Instead, we should leave the flow in the table until we've read all the data for it, *then* discard it. Technically removing the flow_alloc_cancel() would be enough for this: if tcp_flow_repair_socket() fails it leaves conn->sock == -1, which will cause the restore functions in tcp_flow_migrate_target_ext() to fail, discarding the flow. To make what's going on clearer (and with less extraneous error messages), put several explicit tests for a missing socket later in the migration path to read the data associated with the flow but explicitly discard it. Signed-off-by: David Gibson --- tcp.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/tcp.c b/tcp.c index d23b6d94..b3aa9a2c 100644 --- a/tcp.c +++ b/tcp.c @@ -2708,6 +2708,9 @@ int tcp_flow_repair_on(struct ctx *c, const struct tcp_tap_conn *conn) { int rc = 0; + if (conn->sock < 0) + return 0; + if ((rc = repair_set(c, conn->sock, TCP_REPAIR_ON))) err("Failed to set TCP_REPAIR"); @@ -2725,6 +2728,9 @@ int tcp_flow_repair_off(struct ctx *c, const struct tcp_tap_conn *conn) { int rc = 0; + if (conn->sock < 0) + return 0; + if ((rc = repair_set(c, conn->sock, TCP_REPAIR_OFF))) err("Failed to clear TCP_REPAIR"); @@ -3377,7 +3383,8 @@ int tcp_flow_migrate_target(struct ctx *c, int fd) if ((rc = tcp_flow_repair_socket(c, conn))) { flow_err(flow, "Can't set up socket: %s, drop", strerror_(-rc)); - flow_alloc_cancel(flow); + /* Can't leave the flow in an incomplete state */ + FLOW_ACTIVATE(conn); return 0; } @@ -3453,6 +3460,10 @@ int tcp_flow_migrate_target_ext(struct ctx *c, struct tcp_tap_conn *conn, int fd return rc; } + if (conn->sock < 0) + /* We weren't able to create the socket, discard flow */ + goto fail; + if (tcp_flow_select_queue(s, TCP_SEND_QUEUE)) goto fail; @@ -3540,8 +3551,10 @@ int tcp_flow_migrate_target_ext(struct ctx *c, struct tcp_tap_conn *conn, int fd return 0; fail: - tcp_flow_repair_off(c, conn); - repair_flush(c); + if (conn->sock >= 0) { + tcp_flow_repair_off(c, conn); + repair_flush(c); + } conn->flags = 0; /* Not waiting for ACK, don't schedule timer */ tcp_rst(c, conn); -- 2.48.1