From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 5596F5A0319 for ; Thu, 11 Jul 2024 03:54:06 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1720662836; bh=j037drI5yfzhp/OyqvwwC4CKc6R/vvT8hTOHMJMB0UY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gkdg8SzfC8TeBSOJ9vAQQd0KeyagAxZ2hI4v02ppUiLRin2o5ycA7xq5R23qk/j4z Gb7ixR9Fz56v0oXlKUiSwKRr9/trvP9lT1dmEsuwi10mE6FvV0GrstRiQ+DrVxrIWK xq1sEvJL0uk+GLe963BXfB6XupBA8dwuqTlqcc5x+M1uYjNBG+BV7BGYc7fwTwTQJk AZlTsKf70Ul3ToAJfvIVZU7dpGcru3LdB9XRgj5BbpRpZmZse9Pm18GR6wMVawoige XN16g8BrZYAw1GsAwl184RtBhNULmXc+CLoIQGbXVNiXCrVCuSUDjFphp196Bl/nik gkGxeMnG7cO+A== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4WKHnw3qjNz4wc4; Thu, 11 Jul 2024 11:53:56 +1000 (AEST) Date: Thu, 11 Jul 2024 10:19:41 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v7 02/27] flow: Common address information for target side Message-ID: References: <20240705020724.3447719-1-david@gibson.dropbear.id.au> <20240705020724.3447719-3-david@gibson.dropbear.id.au> <20240710233038.6275c284@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="06rp00bAYxAPU/40" Content-Disposition: inline In-Reply-To: <20240710233038.6275c284@elisabeth> Message-ID-Hash: QAT6D2C6A32AVR2SUJUBDBTKEZ3BCXWK X-Message-ID-Hash: QAT6D2C6A32AVR2SUJUBDBTKEZ3BCXWK 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, jmaloy@redhat.com 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: --06rp00bAYxAPU/40 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jul 10, 2024 at 11:30:38PM +0200, Stefano Brivio wrote: > Two minor details: >=20 > On Fri, 5 Jul 2024 12:06:59 +1000 > David Gibson wrote: >=20 > > Require the address and port information for the target (non > > initiating) side to be populated when a flow enters TGT state. > > Implement that for TCP and ICMP. For now this leaves some information > > redundantly recorded in both generic and type specific fields. We'll > > fix that in later patches. > >=20 > > For TCP we now use the information from the flow to construct the > > destination socket address in both tcp_conn_from_tap() and > > tcp_splice_connect(). > >=20 > > Signed-off-by: David Gibson > > --- > > flow.c | 38 ++++++++++++++++++------ > > flow_table.h | 5 +++- > > icmp.c | 3 +- > > inany.h | 1 - > > pif.c | 45 ++++++++++++++++++++++++++++ > > pif.h | 17 +++++++++++ > > tcp.c | 82 ++++++++++++++++++++++++++++------------------------ > > tcp_splice.c | 45 +++++++++++----------------- > > 8 files changed, 158 insertions(+), 78 deletions(-) > >=20 > > diff --git a/flow.c b/flow.c > > index 44e7b3b8..f064fad1 100644 > > --- a/flow.c > > +++ b/flow.c > > @@ -165,8 +165,10 @@ void flow_log_(const struct flow_common *f, int pr= i, const char *fmt, ...) > > */ > > static void flow_set_state(struct flow_common *f, enum flow_state stat= e) > > { > > - char estr[INANY_ADDRSTRLEN], fstr[INANY_ADDRSTRLEN]; > > + char estr0[INANY_ADDRSTRLEN], fstr0[INANY_ADDRSTRLEN]; > > + char estr1[INANY_ADDRSTRLEN], fstr1[INANY_ADDRSTRLEN]; > > const struct flowside *ini =3D &f->side[INISIDE]; > > + const struct flowside *tgt =3D &f->side[TGTSIDE]; > > uint8_t oldstate =3D f->state; > > =20 > > ASSERT(state < FLOW_NUM_STATES); > > @@ -177,19 +179,24 @@ static void flow_set_state(struct flow_common *f,= enum flow_state state) > > FLOW_STATE(f)); > > =20 > > if (MAX(state, oldstate) >=3D FLOW_STATE_TGT) > > - flow_log_(f, LOG_DEBUG, "%s [%s]:%hu -> [%s]:%hu =3D> %s", > > + flow_log_(f, LOG_DEBUG, > > + "%s [%s]:%hu -> [%s]:%hu =3D> %s [%s]:%hu -> [%s]:%hu", > > pif_name(f->pif[INISIDE]), > > - inany_ntop(&ini->eaddr, estr, sizeof(estr)), > > + inany_ntop(&ini->eaddr, estr0, sizeof(estr0)), > > ini->eport, > > - inany_ntop(&ini->faddr, fstr, sizeof(fstr)), > > + inany_ntop(&ini->faddr, fstr0, sizeof(fstr0)), > > ini->fport, > > - pif_name(f->pif[TGTSIDE])); > > + pif_name(f->pif[TGTSIDE]), > > + inany_ntop(&tgt->faddr, fstr1, sizeof(fstr1)), > > + tgt->fport, > > + inany_ntop(&tgt->eaddr, estr1, sizeof(estr1)), > > + tgt->eport); > > else if (MAX(state, oldstate) >=3D FLOW_STATE_INI) > > flow_log_(f, LOG_DEBUG, "%s [%s]:%hu -> [%s]:%hu =3D> ?", > > pif_name(f->pif[INISIDE]), > > - inany_ntop(&ini->eaddr, estr, sizeof(estr)), > > + inany_ntop(&ini->eaddr, estr0, sizeof(estr0)), > > ini->eport, > > - inany_ntop(&ini->faddr, fstr, sizeof(fstr)), > > + inany_ntop(&ini->faddr, fstr0, sizeof(fstr0)), > > ini->fport); > > } > > =20 > > @@ -261,21 +268,34 @@ const struct flowside *flow_initiate_sa(union flo= w *flow, uint8_t pif, > > } > > =20 > > /** > > - * flow_target() - Move flow to TGT, setting TGTSIDE details > > + * flow_target_af() - Move flow to TGT, setting TGTSIDE details > > * @flow: Flow to change state > > * @pif: pif of the target side > > + * @af: Address family for @eaddr and @faddr > > + * @saddr: Source address (pointer to in_addr or in6_addr) > > + * @sport: Endpoint port > > + * @daddr: Destination address (pointer to in_addr or in6_addr) > > + * @dport: Destination port > > + * > > + * Return: pointer to the target flowside information > > */ > > -void flow_target(union flow *flow, uint8_t pif) > > +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) > > { > > struct flow_common *f =3D &flow->f; > > + struct flowside *tgt =3D &f->side[TGTSIDE]; > > =20 > > ASSERT(pif !=3D PIF_NONE); > > ASSERT(flow_new_entry =3D=3D flow && f->state =3D=3D FLOW_STATE_INI); > > ASSERT(f->type =3D=3D FLOW_TYPE_NONE); > > ASSERT(f->pif[INISIDE] !=3D PIF_NONE && f->pif[TGTSIDE] =3D=3D PIF_NO= NE); > > =20 > > + flowside_from_af(tgt, af, daddr, dport, saddr, sport); > > f->pif[TGTSIDE] =3D pif; > > flow_set_state(f, FLOW_STATE_TGT); > > + return tgt; > > } > > =20 > > /** > > diff --git a/flow_table.h b/flow_table.h > > index ad1bc787..00dca4b2 100644 > > --- a/flow_table.h > > +++ b/flow_table.h > > @@ -114,7 +114,10 @@ const struct flowside *flow_initiate_af(union flow= *flow, uint8_t pif, > > const struct flowside *flow_initiate_sa(union flow *flow, uint8_t pif, > > const union sockaddr_inany *ssa, > > in_port_t dport); > > -void flow_target(union flow *flow, uint8_t pif); > > +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); > > =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/icmp.c b/icmp.c > > index cf88ac1f..fd92c7da 100644 > > --- a/icmp.c > > +++ b/icmp.c > > @@ -167,7 +167,8 @@ static struct icmp_ping_flow *icmp_ping_new(const s= truct ctx *c, > > return NULL; > > =20 > > flow_initiate_af(flow, PIF_TAP, af, saddr, id, daddr, id); > > - flow_target(flow, PIF_HOST); > > + /* FIXME: Record outbound source address when known */ > > + flow_target_af(flow, PIF_HOST, af, NULL, 0, daddr, 0); > > pingf =3D FLOW_SET_TYPE(flow, flowtype, ping); > > =20 > > pingf->seq =3D -1; > > diff --git a/inany.h b/inany.h > > index 47b66fa9..8eaf5335 100644 > > --- a/inany.h > > +++ b/inany.h > > @@ -187,7 +187,6 @@ static inline bool inany_is_unspecified(const union= inany_addr *a) > > * > > * Return: true if @a is in fe80::/10 (IPv6 link local unicast) > > */ > > -/* cppcheck-suppress unusedFunction */ > > static inline bool inany_is_linklocal6(const union inany_addr *a) > > { > > return IN6_IS_ADDR_LINKLOCAL(&a->a6); > > diff --git a/pif.c b/pif.c > > index ebf01cc8..9f2d39cc 100644 > > --- a/pif.c > > +++ b/pif.c > > @@ -7,9 +7,14 @@ > > =20 > > #include > > #include > > +#include > > =20 > > #include "util.h" > > #include "pif.h" > > +#include "siphash.h" > > +#include "ip.h" > > +#include "inany.h" > > +#include "passt.h" > > =20 > > const char *pif_type_str[] =3D { > > [PIF_NONE] =3D "", > > @@ -19,3 +24,43 @@ const char *pif_type_str[] =3D { > > }; > > static_assert(ARRAY_SIZE(pif_type_str) =3D=3D PIF_NUM_TYPES, > > "pif_type_str[] doesn't match enum pif_type"); > > + > > + > > +/** pif_sockaddr() - Construct a socket address suitable for an interf= ace > > + * @c: Execution context > > + * @sa: Pointer to sockaddr to fill in > > + * @sl: Updated to relevant of length of initialised @sa >=20 > to relevant length Done. > > + * @pif: Interface to create the socket address > > + * @addr: IPv[46] address > > + * @port: Port (host byte order) > > + * > > + * Return: true if resulting socket address is non-trivial (specified = address or > > + * non-zero port), false otherwise >=20 > This is not really intuitive in the only caller using this, > tcp_bind_outbound(). I wonder if it would make more sense to perform > this check directly there, and have this returning void instead. Yeah, done. When I implemented the return value I thought I was going to want it in more places than turned out to be the case. > > + */ > > +bool pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa, sockl= en_t *sl, > > + uint8_t pif, const union inany_addr *addr, in_port_t port) > > +{ > > + const struct in_addr *v4 =3D inany_v4(addr); > > + > > + ASSERT(pif_is_socket(pif)); > > + > > + if (v4) { > > + sa->sa_family =3D AF_INET; > > + sa->sa4.sin_addr =3D *v4; > > + sa->sa4.sin_port =3D htons(port); > > + memset(&sa->sa4.sin_zero, 0, sizeof(sa->sa4.sin_zero)); > > + *sl =3D sizeof(sa->sa4); > > + return !IN4_IS_ADDR_UNSPECIFIED(v4) || port; > > + } > > + > > + sa->sa_family =3D AF_INET6; > > + sa->sa6.sin6_addr =3D addr->a6; > > + sa->sa6.sin6_port =3D htons(port); > > + if (pif =3D=3D PIF_HOST && IN6_IS_ADDR_LINKLOCAL(&addr->a6)) > > + sa->sa6.sin6_scope_id =3D c->ifi6; > > + else > > + sa->sa6.sin6_scope_id =3D 0; > > + sa->sa6.sin6_flowinfo =3D 0; > > + *sl =3D sizeof(sa->sa6); > > + return !IN6_IS_ADDR_UNSPECIFIED(&addr->a6) || port; > > +} >=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 --06rp00bAYxAPU/40 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmaPJQ4ACgkQzQJF27ox 2GcXwQ/8CzyYFu2EAlBtwuAD2HLwtLfaBed/oKvo26VfGM+0FxhzWnjN9kQb5cVr EnL5fm6nio/czoXfnojY/pVYxu45/gB62WTYy0Xn4GHzeRnGGZZYON1aCT+VkBev ISSLT2Rx1kBXYAuQqyxqXZw2Ml0v5WuPtuMmKWFNQQ5gctY0o4pMATjPPszMKKwt TUNaWb7FDcRz6Gk+GNg5/a334kXdjsqZ6jWhUdD7pbtbBYBbBXb7rsQsFZbjB4Z6 Q9FfEtYlTuAiNpq75SnUAQuJqstZ+Pca1wUZj0h32ADGJCRfXMofdFivXJsWl2gX TXbE9BcA824KHVs+9d6KcdOT4Uivg0ZG2EJ9+KJYTcojBvfM9lAUdaDeR4m4Ah8b nKuE1U5QhfZa8JSGb5YtO/0R9NdbOHesLoqDfANOiE1MN1ZsMcj7nX4D8GOQ3luf nDDDSld0p1gBOek+97EDuHag6IQWGDHOKDrvsvGdBY4iHYKlV+vDSkw7mBVQMuqu 3G+bv93pnmCYgSKrQZiRFbCtJhr+vwU2D3oWKzBEFhgIaJzdulqnuEGSuYYCfgNC h5qVcWDDCrfSwfelSxdEpvGV2NKSV3E3x/Nmco4H8xrFDtkc0yJV5bT4bHyyLjVq oB2kQsxrK6NErfTY0rb5PzgxDcv7KTJ6b0W4vLzZ0FjGVhbFhHA= =1vZD -----END PGP SIGNATURE----- --06rp00bAYxAPU/40--