Re: [PATCH v3 18/20] tcp: Get our socket port using getsockname() when connecting from guest

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 3 Feb 2025 19:59:12 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Feb 03, 2025 at 07:09:37AM +0100, Stefano Brivio wrote:
> > On Mon, 3 Feb 2025 13:05:33 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Fri, Jan 31, 2025 at 08:39:51PM +0100, Stefano Brivio wrote:  
> > > > For migration only: we need to store 'oport', our socket-side port,
> > > > as we establish a connection from the guest, so that we can bind the
> > > > same oport as source port in the migration target.
> > > > 
> > > > Use getsockname() to fetch that.
> > > > 
> > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > > ---
> > > >  flow.c       |  4 ++--
> > > >  flow_table.h |  4 ++--
> > > >  tcp.c        | 24 +++++++++++++++++++++++-
> > > >  3 files changed, 27 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/flow.c b/flow.c
> > > > index 5638ff1..506cbac 100644
> > > > --- a/flow.c
> > > > +++ b/flow.c
> > > > @@ -411,8 +411,8 @@ const struct flowside *flow_initiate_sa(union flow *flow, uint8_t pif,
> > > >   *
> > > >   * Return: pointer to the target flowside information
> > > >   */
> > > > -const struct flowside *flow_target(const struct ctx *c, union flow *flow,
> > > > -				   uint8_t proto)
> > > > +struct flowside *flow_target(const struct ctx *c, union flow *flow,
> > > > +			     uint8_t proto)
> > > >  {
> > > >  	char estr[INANY_ADDRSTRLEN], fstr[INANY_ADDRSTRLEN];
> > > >  	struct flow_common *f = &flow->f;
> > > > diff --git a/flow_table.h b/flow_table.h
> > > > index 633805d..b107107 100644
> > > > --- a/flow_table.h
> > > > +++ b/flow_table.h
> > > > @@ -178,8 +178,8 @@ const struct flowside *flow_target_af(union flow *flow, uint8_t pif,
> > > >  				      sa_family_t af,
> > > >  				      const void *saddr, in_port_t sport,
> > > >  				      const void *daddr, in_port_t dport);
> > > > -const struct flowside *flow_target(const struct ctx *c, union flow *flow,
> > > > -				   uint8_t proto);
> > > > +struct flowside *flow_target(const struct ctx *c, union flow *flow,
> > > > +			     uint8_t proto);
> > > >  
> > > >  union flow *flow_set_type(union flow *flow, enum flow_type type);
> > > >  #define FLOW_SET_TYPE(flow_, t_, var_)	(&flow_set_type((flow_), (t_))->var_)
> > > > diff --git a/tcp.c b/tcp.c
> > > > index 0bd2a02..4fd405b 100644
> > > > --- a/tcp.c
> > > > +++ b/tcp.c
> > > > @@ -1471,6 +1471,8 @@ static void tcp_bind_outbound(const struct ctx *c,
> > > >   * @opts:	Pointer to start of options
> > > >   * @optlen:	Bytes in options: caller MUST ensure available length
> > > >   * @now:	Current timestamp
> > > > + *
> > > > + * #syscalls:vu getsockname
> > > >   */
> > > >  static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af,
> > > >  			      const void *saddr, const void *daddr,
> > > > @@ -1479,9 +1481,10 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af,
> > > >  {
> > > >  	in_port_t srcport = ntohs(th->source);
> > > >  	in_port_t dstport = ntohs(th->dest);
> > > > -	const struct flowside *ini, *tgt;
> > > > +	const struct flowside *ini;
> > > >  	struct tcp_tap_conn *conn;
> > > >  	union sockaddr_inany sa;
> > > > +	struct flowside *tgt;
> > > >  	union flow *flow;
> > > >  	int s = -1, mss;
> > > >  	uint64_t hash;
> > > > @@ -1586,6 +1589,25 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af,
> > > >  	}
> > > >  
> > > >  	tcp_epoll_ctl(c, conn);
> > > > +
> > > > +	if (c->mode == MODE_VU) { /* To rebind to same oport after migration */    
> > > 
> > > I suspect we'll want this local side information in more places in
> > > future, but this is fine for now.
> > >   
> > > > +		if (af == AF_INET) {
> > > > +			struct sockaddr_in s_in;
> > > > +			socklen_t sl;
> > > > +
> > > > +			sl = sizeof(s_in);
> > > > +			getsockname(s, (struct sockaddr *)&s_in, &sl);
> > > > +			tgt->oport = ntohs(s_in.sin_port);    
> > > 
> > > Since we're already doing the getsockname() we should also update
> > > tgt->oaddr, and that might matter in cases where the host has multiple
> > > local addresses.  
> > 
> > I had that in a previous version, because I was actually restoring it
> > as I thought it was needed, then I dropped it.
> > 
> > We expect the configuration of the target to be the same as the source,  
> 
> Eh... up to a point.  I'm not sure about the kubevirt case, but in
> general when migrating amongst managed hosts it would not surprise me
> to find that the destination has multiple IPs.  One is the "public" IP
> used by the workload and will indeed be the same between the ends.
> However, there could also be a "management" IP that's different
> between them - after all the management system will need to talk to
> both source and destination simultaneously for a little while.
> 
> > so the same connect() should yield to the same source address being
> > used (minus the fact that we don't set socket options yet (point 9. of
> > the to-do list from cover letter). So should we really bind() to a
> > specific source address just because we happen to know it? I'm not
> > quite sure.  
> 
> Yes, we should.  Note that right now, the outbound socket already has
> a fixed bound address, assigned at connect() time.  We don't track it
> in our internal data structures, it's still there as part of the
> socket state, and our behaviour reflects that.  That forms part of the
> socket state which we should maintain across migration.

Okay, fine, then let me add that back.

> We could do this lazily - one of the pre-migrate steps could be a
> getsockname() on any flows which have an unassigned oaddr/oport.

Sure, on the other hand that's an optimisation, and I already have code
ready for this.

-- 
Stefano