Re: [PATCH 1/1] migrate: Use forward table information to close() listening sockets

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 30 Jan 2026 16:58:11 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On incoming migrations we need to bind() reconstructed sockets to their
> correct local address.  We can't do this if the origin passt instance is
> in the same namespace and still has those addresses bound.  Arguably that's
> a bug in bind()s operation during repair mode, but for now we have to work
> around it.
> 
> So, to allow local-to-local migrations we close() sockets on the outgoing
> side as we process them.  In addition to closing the connected socket we
> also have to close the associated listen()ing socket, because that can also
> cause an address conflict.
> 
> To do that, we introduced the listening_sock field in the connection
> state, because we had no other way to find the right listening sockets.
> Now that we have the forwarding table, we have a complete list of
> listening sockets elsewhere.  We can use that instead, to close all
> listening sockets on outbound migration, rather than just the ones that
> might conflict.
> 
> This is cleaner and, importantly, saves a valuable 32-bits in the flow
> state structure.  It does mean that there is a longer window where a peer
> attempting to connect during migration might get a Connection Refused.
> I think this is an acceptable trade-off for now: arguably we should not
> allow local-to-local migrations in any case, since the socket closes make
> it impossible to safely roll back migration as per the qemu model.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  flow.c     | 12 ++++++++++++
>  fwd.c      | 21 +++++++++++++++++++++
>  fwd.h      |  1 +
>  tcp.c      |  9 ---------
>  tcp_conn.h |  3 ---
>  5 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/flow.c b/flow.c
> index fd4d5f38..5207143d 100644
> --- a/flow.c
> +++ b/flow.c
> @@ -1023,6 +1023,9 @@ static int flow_migrate_source_rollback(struct ctx *c, unsigned bound, int ret)
>  
>  	debug("...roll back migration");
>  
> +	if (fwd_listen_sync(c, &c->tcp.fwd_in, PIF_HOST, IPPROTO_TCP) < 0)
> +		die("Failed to re-establish listening sockets");
> +
>  	foreach_established_tcp_flow(flow) {
>  		if (FLOW_IDX(flow) >= bound)
>  			break;
> @@ -1147,6 +1150,15 @@ int flow_migrate_source(struct ctx *c, const struct migrate_stage *stage,

Nit: the comment to this function currently says "Send data (flow
table) for flow, close listening". I fixed that up (dropped ", close listening").

>  		return flow_migrate_source_rollback(c, FLOW_MAX, rc);
>  	}
>  
> +	/* HACK: A local to local migrate will fail if the origin passt has the
> +	 * listening sockets still open when the destination passt tries to bind
> +	 * them.  This does mean there's a window where we lost our listen()s,
> +	 * even if the migration is rolled back later.  The only way to really
> +	 * fix that is to not allow local to local migration, which arguably we
> +	 * should (use namespaces for testing instead). */

Actually, we already use namespaces in the current tests, but we didn't
(always) do that during development, and it might be convenient in
general to have the possibility to test *a part* of the implementation
using the same namespace as long as it's reasonably cheap (it seems to
be).

That's just a part because anyway bind() and connect() will conflict,
if we're in the same namespace, which is a kernel issue you already
noted:

  https://pad.passt.top/p/TcpRepairTodo#L3
  Repair mode sockets should not have address conflicts with non-repair
  sockets (both bind() and connect())

but even that part is convenient to have, I think, so I'm a bit worried
that somebody might take this comment as a to-do item, while I don't
think it should be.

Patch applied anyway, to give this as much testing time and exposure as
possible.

-- 
Stefano