From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202502 header.b=KkE0c0wX; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 30F225A0272 for ; Mon, 03 Feb 2025 10:25:11 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202502; t=1738574694; bh=k3G//4KGvVojUkmEt9fmt+4tMPQAcmd7Nm6E2mzZLIg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=KkE0c0wXtE0zm4OrGP00pDVa+vXWG7hynGbfOAReE+9mVbME/eWzQbOH6eZlYUoCs sibskTO6YPvnUvxTGExpk6DqtAbhxunSILAKF6Ycjnvr4nwNCNeWMYlwnZeggKxp+U glitn/N+REt3ho+ioWhWhvy5mwcxLEkWNSDfLJwH1dwip++U7rltSU7hnICaGnYICR k2sPcCwnexNIsA03qDUCVFZNd4oOJvE+BZL3Ew/9uZ9fKRFVl9+9kcmBLqkWQibsMO y45WFNh/Pd2UGj8dYg4WqhLEOm48136DpOPbN/8cHiSErqhEx7AWZQbHKcwIRmlE8e FwcnUpx2bziIw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Ymh0k39TVz4wy9; Mon, 3 Feb 2025 20:24:54 +1100 (AEDT) Date: Mon, 3 Feb 2025 19:59:12 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 18/20] tcp: Get our socket port using getsockname() when connecting from guest Message-ID: References: <20250131193953.3034031-1-sbrivio@redhat.com> <20250131193953.3034031-19-sbrivio@redhat.com> <20250203070937.61c30df9@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Zp0HsHAmdwzoHx7V" Content-Disposition: inline In-Reply-To: <20250203070937.61c30df9@elisabeth> Message-ID-Hash: YPFYT7FFXINVOBVYCXKK67U4FUIEYV5T X-Message-ID-Hash: YPFYT7FFXINVOBVYCXKK67U4FUIEYV5T X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Laurent Vivier X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --Zp0HsHAmdwzoHx7V Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Feb 03, 2025 at 07:09:37AM +0100, Stefano Brivio wrote: > On Mon, 3 Feb 2025 13:05:33 +1100 > David Gibson wrote: >=20 > > 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. > > >=20 > > > Use getsockname() to fetch that. > > >=20 > > > Signed-off-by: Stefano Brivio > > > --- > > > flow.c | 4 ++-- > > > flow_table.h | 4 ++-- > > > tcp.c | 24 +++++++++++++++++++++++- > > > 3 files changed, 27 insertions(+), 5 deletions(-) > > >=20 > > > 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 flo= w *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 =3D &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); > > > =20 > > > 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 =3D ntohs(th->source); > > > in_port_t dstport =3D 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 =3D -1, mss; > > > uint64_t hash; > > > @@ -1586,6 +1589,25 @@ static void tcp_conn_from_tap(const struct ctx= *c, sa_family_t af, > > > } > > > =20 > > > tcp_epoll_ctl(c, conn); > > > + > > > + if (c->mode =3D=3D MODE_VU) { /* To rebind to same oport after migr= ation */ =20 > >=20 > > I suspect we'll want this local side information in more places in > > future, but this is fine for now. > >=20 > > > + if (af =3D=3D AF_INET) { > > > + struct sockaddr_in s_in; > > > + socklen_t sl; > > > + > > > + sl =3D sizeof(s_in); > > > + getsockname(s, (struct sockaddr *)&s_in, &sl); > > > + tgt->oport =3D ntohs(s_in.sin_port); =20 > >=20 > > 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. >=20 > I had that in a previous version, because I was actually restoring it > as I thought it was needed, then I dropped it. >=20 > 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. We could do this lazily - one of the pre-migrate steps could be a getsockname() on any flows which have an unassigned oaddr/oport. >=20 > > > + } else { > > > + struct sockaddr_in6 s_in6; > > > + socklen_t sl; > > > + > > > + sl =3D sizeof(s_in6); > > > + getsockname(s, (struct sockaddr *)&s_in6, &sl); > > > + tgt->oport =3D ntohs(s_in6.sin6_port); > > > + } =20 > >=20 > > 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. >=20 > I guess it can be a follow-up. Noted, anyway. >=20 --=20 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 --Zp0HsHAmdwzoHx7V Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmeghV8ACgkQzQJF27ox 2GfVzg/9E3XZ+k0lQcA75QQE5eBon/c0fxq8jVNdYpuIS1SW90c9qd1cxVZf/NI5 ya1uZvz0LgBd38d368wfWbTLNzuRG9PnUfaSSGnIIBm4EdYOOQ9c0mXOeYfXkulF BmV0AzQ+ah3R/uwl8ljsRGnbQDW77pEDQn/PffAuRxCdSrewebbuslUYyDt3gxTF krPSD7GZOriYRgG5Y37AQUGnVAiCmx2yPQpDHEew4OcEvh1r0VoS4ugTUyTCE2TY R08uoz9SjPptbYoNcQn8fno3JK/Syb0iJsqQ9miltVR4V2dCQzbTtzlxgZO0oSPM 7aQ7aidP2faYJZa8Jv4Vg727sXCc+CWCvhaZy4Jh+8oYp202qaF4xHFgqg93nsoV kV1wYfT1m4NPvRHEE51yNk1AkhrDmmifck4ljlglcQJd7SNXotpOahMundiYHK4p 8rqlmo+34/Ma5g4S5ponogYWIeAPQfkcN0/Bk/aD6OP0dbkZWsp1TxXewzSkMH/r ntnwSi/9pok2baF2SJlAWIJmsV0rdQ5ieqJrmnnMe0mB0VxAyub3CWlMKDy//b8G IOwhTZK1IqoTQFfeHpDeMOJzW6n9/FwPS5Cs6+ol0E5tTmqPImLmbt8Z3NBsCj+F 16mmbNBGGyEFGzz+FWOuhjqQ3wdmo0BeV1durLY4MJ65LBwwSls= =jEab -----END PGP SIGNATURE----- --Zp0HsHAmdwzoHx7V--