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

> > +		} else {
> > +			struct sockaddr_in6 s_in6;
> > +			socklen_t sl;
> > +
> > +			sl = sizeof(s_in6);
> > +			getsockname(s, (struct sockaddr *)&s_in6, &sl);
> > +			tgt->oport = ntohs(s_in6.sin6_port);
> > +		}  
> 
> We should add an inany_getsockname() or something helper for this.  In
> fact I'm pretty sure I wrote one at some point, but it was lost in the
> shuffles of various flow table iterations.

I guess it can be a follow-up. Noted, anyway.

-- 
Stefano