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 B602F5A004E for ; Wed, 26 Jun 2024 02:34:16 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1719362053; bh=M3iRjGMbLf2ziNBiCC3GtF8FRfXDrhgUNgbobCLovNo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=jjzDlNKoxSJEy8qDn1xNtOHrFo83OqX9pCGQrlBdNTl/JW3TGF9uFOf/n1JB09lJg eK99ARDqvvqwLklh2nm+D6rGSJbXfqqgTbypNDRBcrU0ulhB6oZ4jidzCdY1K+AYPk nxTyX0i1PV0SkEYtzamR7zrKIcnWmOlBfhoycroUP6fqmWnoBXB9WTgsowz/NUXCog HAWKe+pWIkeqApXbLFnEXTsiwbPewf4AqJV1NJc1lbsNEfGSCfgAZeB/GD24RwW/0M XsMx2HgOibBWOg5qsQxgZsytQMKYSAKpOqBqt/RlZLDCqqChz85iNDmcsH1y602yPK Vt1g23aTlfNPg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4W82ks1v9tz4w2P; Wed, 26 Jun 2024 10:34:13 +1000 (AEST) Date: Wed, 26 Jun 2024 10:19:31 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v6 01/26] flow: Common address information for initiating side Message-ID: References: <20240614061348.3814736-1-david@gibson.dropbear.id.au> <20240614061348.3814736-2-david@gibson.dropbear.id.au> <20240626002328.74900b0d@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="wPsK8q90dtmae5ir" Content-Disposition: inline In-Reply-To: <20240626002328.74900b0d@elisabeth> Message-ID-Hash: OAQPLC2KKNQSDRJMFS2KE5ALYPMGYA3G X-Message-ID-Hash: OAQPLC2KKNQSDRJMFS2KE5ALYPMGYA3G 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: --wPsK8q90dtmae5ir Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 26, 2024 at 12:23:28AM +0200, Stefano Brivio wrote: > On Fri, 14 Jun 2024 16:13:23 +1000 > David Gibson wrote: >=20 > > Handling of each protocol needs some degree of tracking of the > > addresses and ports at the end of each connection or flow. Sometimes > > that's explicit (as in the guest visible addresses for TCP > > connections), sometimes implicit (the bound and connected addresses of > > sockets). > >=20 > > To allow more consistent handling across protocols we want to > > uniformly track the address and port at each end of the connection. > > Furthermore, because we allow port remapping, and we sometimes need to > > apply NAT, the addresses and ports can be different as seen by the > > guest/namespace and as by the host. > >=20 > > Introduce 'struct flowside' to keep track of address and port > > information related to one side of a flow. Store two of these in the > > common fields of a flow to track that information for both sides. > >=20 > > For now we only populate the initiating side, requiring that > > information be completed when a flows enter INI. Later patches will > > populate the target side. > >=20 > > For now this leaves some information redundantly recorded in both gener= ic > > and type specific fields. We'll fix that in later patches. > >=20 > > Signed-off-by: David Gibson > > --- > > flow.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++--- > > flow.h | 16 +++++++++ > > flow_table.h | 8 ++++- > > icmp.c | 9 +++-- > > passt.h | 3 ++ > > tcp.c | 6 ++-- > > 6 files changed, 127 insertions(+), 11 deletions(-) > >=20 > > diff --git a/flow.c b/flow.c > > index d05aa495..1819111d 100644 > > --- a/flow.c > > +++ b/flow.c > > @@ -108,6 +108,31 @@ static const union flow *flow_new_entry; /* =3D NU= LL */ > > /* Last time the flow timers ran */ > > static struct timespec flow_timer_run; > > =20 > > +/** flowside_from_af() - Initialise flowside from addresses > > + * @fside: flowside to initialise > > + * @af: Address family (AF_INET or AF_INET6) > > + * @eaddr: Endpoint address (pointer to in_addr or in6_addr) > > + * @eport: Endpoint port > > + * @faddr: Forwarding address (pointer to in_addr or in6_addr) > > + * @fport: Forwarding port > > + */ > > +static void flowside_from_af(struct flowside *fside, sa_family_t af, > > + const void *eaddr, in_port_t eport, > > + const void *faddr, in_port_t fport) > > +{ > > + if (faddr) > > + inany_from_af(&fside->faddr, af, faddr); > > + else > > + fside->faddr =3D inany_any6; >=20 > I kind of wonder if, for clarity, we should perhaps: >=20 > #define inany_any inany_any6 >=20 > ? That might be a good idea. > And... I don't actually have in mind how IP versions are stored in the > flow table for unspecified addresses, but I wonder if we're losing some > information this way: if this is called with AF_INET as 'af', we're not > saying anywhere in the flowside information that this is going to be an > IPv4 flowside, right? So, I've gone back and forth on this a bit, and it's not entirely consistent in this version whether I always use :: for "blank", or whether I use 0.0.0.0 for IPv4 cases. Using :: everywhere does technically lose some information, although you can generally tell that a flowside is IPv4 from the other address. The argument for using :: everywhere is that in many of the cases this indicates "blank" or "missing" in a way that's not really dependent on the IP version, and sometimes it's simpler to do this. There are (or may be in future) also some cases where an otherwise IPv4 flowside might be routed through a dual-stack :: socket. The argument for using 0.0.0.0 where accurate is that it makes for more consistency across the flowside and makes matching up to addresses from elswhere easier in some cases. With the reworks for how I handle UDP, I'm going to need to revisit this anyway, so I'll see where I land. > > + fside->fport =3D fport; > > + > > + if (eaddr) > > + inany_from_af(&fside->eaddr, af, eaddr); > > + else > > + fside->eaddr =3D inany_any6; > > + fside->eport =3D eport; > > +} > > + > > /** flow_log_ - Log flow-related message > > * @f: flow the message is related to > > * @pri: Log priority > > @@ -140,6 +165,8 @@ void flow_log_(const struct flow_common *f, int pri= , const char *fmt, ...) > > */ > > static void flow_set_state(struct flow_common *f, enum flow_state stat= e) > > { > > + char estr[INANY_ADDRSTRLEN], fstr[INANY_ADDRSTRLEN]; > > + const struct flowside *ini =3D &f->side[INISIDE]; > > uint8_t oldstate =3D f->state; > > =20 > > ASSERT(state < FLOW_NUM_STATES); > > @@ -150,18 +177,28 @@ 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 =3D> %s", pif_name(f->pif[INISIDE]), > > - pif_name(f->pif[TGTSIDE])); > > + flow_log_(f, LOG_DEBUG, "%s [%s]:%hu -> [%s]:%hu =3D> %s", > > + pif_name(f->pif[INISIDE]), > > + inany_ntop(&ini->eaddr, estr, sizeof(estr)), > > + ini->eport, > > + inany_ntop(&ini->faddr, fstr, sizeof(fstr)), > > + ini->fport, > > + pif_name(f->pif[TGTSIDE])); > > else if (MAX(state, oldstate) >=3D FLOW_STATE_INI) > > - flow_log_(f, LOG_DEBUG, "%s =3D> ?", pif_name(f->pif[INISIDE])); > > + flow_log_(f, LOG_DEBUG, "%s [%s]:%hu -> [%s]:%hu =3D> ?", > > + pif_name(f->pif[INISIDE]), > > + inany_ntop(&ini->eaddr, estr, sizeof(estr)), > > + ini->eport, > > + inany_ntop(&ini->faddr, fstr, sizeof(fstr)), > > + ini->fport); > > } > > =20 > > /** > > - * flow_initiate() - Move flow to INI, setting INISIDE details > > + * flow_initiate_() - Move flow to INI, setting setting pif[INISIDE] >=20 > s/setting setting/setting/ Fixed. --=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 --wPsK8q90dtmae5ir Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZ7XoEACgkQzQJF27ox 2Gfqow/6A4qPZj3qh8g3YAcp4TWwbBIXbmEAtNZQ68klnTaS2RX1r42X2BBEZ0sJ 1rJgZMAwLCcEMM5LjoIz45+Yqjss+uhap6eO6UaVzWyo2OMF26dPTZh2Od0wJ5JK nwvUIqPROHFX/s5HI19tx7bQPVWLc/kC5GT0VI0qV/fgzrsOtV1Y5FE3B8SwXBTv /PssRuIOxALS9HG+3VGofNmI3ECfoyR/EXDRba2a6Nz1IiAPGzRMCATt3lM2/h4Z peOdyUXQQ2WHDimkm53tG2m6XcWcN9YRp6N+n4zHVB0u+oD/hjxrK1IniLO41Yae K0P+8X8cG4O4sR4tnonLc4c2lwOGavxPQBrlvb4eG2aGz6kZNrGJY5A6akFr/ODK Y37wWYEHY5OqzOOl9+be2RcRm4fe2xD6rfRhsLHziihneUDvSmZX5IWXth3LdzYN NmEMWJqfbHiQ3JfTLUGL4hVndvwooOjmjX/CHOEj4r7WLkT+020I4GMRm0vPXtkU yWbDViveVW4xAbWT8Wn+HwkGOZVDOb4YsexmZ616dI7Yh9JNRDnfs3PUfWcjxOz3 w4lxzcjPjFIVGHzI8p3cIaDt7Y/0Ewy4b5wuzurtmFFB4CVw10eYZjQ0C+ahmIso GSMdOa1Q21TrXMGhDseEcNO38AO5FTy+0l5cm2ai7f6nB5rUJtc= =Ee0B -----END PGP SIGNATURE----- --wPsK8q90dtmae5ir--