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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5125 bytes --]

On Sat, Jan 31, 2026 at 10:47:28AM +0100, Stefano Brivio wrote:
> 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").

Good point, thanks.

> >  		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,

Oh, nice.

> 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).

Depends what cost you're talking about.  It's cheap in terms of
computational complexity, and code compexity.  It means, however, that
we can't necessarily roll back failed migrations - i.e. resume on the
origin system.  That isn't really correct for the qemu migration
model, which is why I think allowing local migrations probably isn't
the best idea, at least by default.

> 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:

Well, it's a kernel issue that the bound listen()ing sockets conflict
with the half-constructed flow sockets.  Having the listening sockets
of the origin passt conflict with the listening sockets of the
destination passt is pretty much expected, and would still be an
impediment to local migration.

>   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,

I'm not really sure what you mean by that.

> 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
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]