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. However, it can'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, though, put an explicit test for a bad socket fd
in tcp_flow_migrate_target_ext() and discard at that point.
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
tcp.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tcp.c b/tcp.c
index e3c0a53b..f713fa99 100644
--- a/tcp.c
+++ b/tcp.c
@@ -3376,7 +3376,6 @@ 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);
return 0;
}
@@ -3452,6 +3451,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;
--
2.48.1