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: 9065 bytes --]

On Thu, Feb 05, 2026 at 01:17:52AM +0100, Stefano Brivio wrote:
> On Wed, 4 Feb 2026 21:57:03 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Feb 02, 2026 at 11:12:08PM +0100, Stefano Brivio wrote:
> > > On Mon, 2 Feb 2026 10:24:14 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > 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.  
> > > 
> > > Well it's not entirely true that we can't roll back failed migrations.
> > > 
> > > Depending on the stage we're at, we might close connections, but other
> > > than that we can always continue, somehow. Losing some connections
> > > looks relatively cheap as well.  
> > 
> > I guess that's true. I'm not sure we currently handle this even as
> > well as is possible within the constraints.
> 
> I actually checked a while ago, nothing unexpected happen.
> 
> > > But sure, it's not ideal. Maybe yes, we shouldn't have that as default,
> > > add a new option and update QEMU's documentation (see below) with
> > > it.  
> > 
> > There's two layers to it.  Dropping connections on what's otherwise a
> > no-op migration failure is ugly.  Opening the possibility that we
> > might not be able to rebind ports we were already listening on is
> > worse.
> 
> But that would only happen if somebody starts an unrelated process
> binding the same ports, right? I'm not sure if it's something we really
> need to take care of.

Maybe.  Actually, arguably more important than the possibility of
losing the port is that there may be an interval where the virtual
server will reject connections, making the migration not really
seamless from the point of view of peers (which is, after all, the
point of live migration).


> > > > > 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.  
> > > 
> > > Ah, right, we don't set listening sockets to repair mode... should we,
> > > by the way?  
> > 
> > Uh.. I don't think so?  I'm not really sure how repair mode operates
> > for listening sockets, or if it should even allow that.  The relevant
> > state of a listening socket is pretty much limited to its bound
> > address, so I don't think we need any additional mechanism to extract
> > state.
> 
> I wasn't suggesting that for any functional purpose. Repair mode has no
> meaning there.
> 
> But it would have the advantage, if we fix/change this in the kernel:
> 
>   https://pad.passt.top/p/TcpRepairTodo#L3
>   Repair mode sockets should not have address conflicts with
>   non-repair sockets (both bind() and connect())
> 
> that bind() calls wouldn't conflict with bound sockets in repair mode,
> and let us test a bit more of local migration.

I guess.  The key difference with listening sockets is that in a sense
they really do conflict, or at least really will.  The problem with
address conflicts between listening sockets and connected sockets, is
that the conflict is reported when we've bound, but not yet connected
the socket we're reconstructing.  The conflict will go away once the
socket state is fully restored, but we can't get there without hitting
an error because we must bind() before we connect().  That's the
specific problem I was looking to address by not caring about
conflicts between repair mode and non-repair-mode sockets.

> On the other hand I'm not sure what should happen once you bring a
> socket, which was originally bound to a given port, out of repair mode,
> once there's a new socket bound to it.

Right.  This is potentially an issue with connect()ed sockets, too.  I
think we have to fail the exit from repair mode if there's an address
conflict.  That's introduces a new somewhat awkward case, but I can't
see how else to address the ephemeral address conflict.

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