From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 278EF5A0272 for ; Thu, 7 Sep 2023 06:14:17 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1694060051; bh=Zr41MmVZF5Zdwf3hHdV/G0YWMeMVNHps+E3EnNAqWKU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=i6BrLiy6e/c3APBFBlBmDFU5bWDt+kJ67l9PY0Qyt+n10Tp6NIcuIw4Uaa5NOHSrD XDpiZ80ZaUb9AabZDrHhQuKiscU5C++AHhHWs5HngKIysV0aqSnmIpoBOebc/XvvXP ikJ7lc8Bvf9qN6gv7zDkageqKSjoFWbP4J80YmY0= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Rh5Tv3Ntdz4xFC; Thu, 7 Sep 2023 14:14:11 +1000 (AEST) Date: Thu, 7 Sep 2023 14:05:37 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v2 05/10] flow: Introduce struct flowside, space for uniform tracking of addresses Message-ID: References: <20230828054146.48673-1-david@gibson.dropbear.id.au> <20230828054146.48673-6-david@gibson.dropbear.id.au> <20230907030140.5dd18377@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="dQ0nEVRcNMXgv7gD" Content-Disposition: inline In-Reply-To: <20230907030140.5dd18377@elisabeth> Message-ID-Hash: 24NRWIHPT6DNOHYXCKLIZHCTVGCAOTGQ X-Message-ID-Hash: 24NRWIHPT6DNOHYXCKLIZHCTVGCAOTGQ 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 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: --dQ0nEVRcNMXgv7gD Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Sep 07, 2023 at 03:01:40AM +0200, Stefano Brivio wrote: > On Mon, 28 Aug 2023 15:41:41 +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 expl= icit > > (as in the guest visible addresses for TCP connections), sometimes impl= icit > > (the bound and connected addresses of sockets). > >=20 > > To allow more general abd robust handling, and more consistency 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 the address and ports of a > > flow from a single "side" (guest or host). Store two of these in the > > common fields of a flow to track that information for both sides. > >=20 > > For now we just introduce the structure and fields themselves, along wi= th > > some simple helpers. Later patches will actually use these to store us= eful > > information. > >=20 > > Signed-off-by: David Gibson > > --- > > flow.c | 22 ++++++++++++++++++++++ > > flow.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 70 insertions(+) > >=20 > > diff --git a/flow.c b/flow.c > > index 12ca8db..a93cf8c 100644 > > --- a/flow.c > > +++ b/flow.c > > @@ -7,6 +7,7 @@ > > =20 > > #include > > #include > > +#include > > =20 > > #include "util.h" > > #include "passt.h" > > @@ -24,6 +25,27 @@ const char *flow_type_str[] =3D { > > /* Global Flow Table */ > > union flow flowtab[FLOW_MAX]; > > =20 > > +/** flowside_fmt - Format a flowside as a string > > + * @fs: flowside to format > > + * @buf: Buffer into which to store the formatted version > > + * @size: Size of @buf > > + * > > + * Return: pointer to formatted string describing @fs, or NULL on error > > + */ > > +/* cppcheck-suppress unusedFunction */ > > +const char *flowside_fmt(const struct flowside *fs, char *buf, size_t = size) > > +{ > > + char ebuf[INET6_ADDRSTRLEN], fbuf[INET6_ADDRSTRLEN]; > > + > > + if (!inet_ntop(AF_INET6, &fs->eaddr, ebuf, sizeof(ebuf)) > > + || !inet_ntop(AF_INET6, &fs->faddr, fbuf, sizeof(fbuf))) >=20 > For consistency (also with flowside_complete() below): >=20 > if (!inet_ntop(AF_INET6, &fs->eaddr, ebuf, sizeof(ebuf)) || > !inet_ntop(AF_INET6, &fs->faddr, fbuf, sizeof(fbuf))) Done. > > + return NULL; > > + > > + snprintf(buf, size, "[%s]:%hu <-> [%s]:%hu", fbuf, fs->fport, > > + ebuf, fs->eport); > > + return (const char *)buf; > > +} > > + > > /** > > * flow_table_compact() - Perform compaction on flow table > > * @c: Execution context > > diff --git a/flow.h b/flow.h > > index e212796..9891fcb 100644 > > --- a/flow.h > > +++ b/flow.h > > @@ -18,11 +18,59 @@ extern const char *flow_type_str[]; > > #define FLOW_TYPE(f) \ > > ((f)->type <=3D FLOW_MAX ? flow_type_str[(f)->type] : "?") > > =20 > > +/** > > + * struct flowside - Describes a logical packet flow as seen from one = "side" > > + * @eaddr: Endpoint address (remote address from passt's PoV) > > + * @faddr: Forwarding address (local address from passt's PoV) > > + * @eport: Endpoint port > > + * @fport: Forwarding port > > + */ > > +struct flowside { > > + union inany_addr faddr; > > + union inany_addr eaddr; > > + in_port_t fport, eport; >=20 > I guess always valid, but uncommon (compared to in_port_t x; in_port_t > y;)? Huh.. I didn't even think about that (and the fact that I did this for the ports, but not for the addresses). Changed it to the more conventional style. > > +}; > > + > > +/** flowside_from_af - Initialize a flowside from addresses > > + * @fs: flowside to initialize > > + * @af: Address family (AF_INET or AF_INET6) > > + * @faddr: Forwarding address (pointer to in_addr or in6_addr) > > + * @fport: Forwarding port > > + * @eaddr: Endpoint address (pointer to in_addr or in6_addr) > > + * @eport: Endpoint port > > + */ > > +static inline void flowside_from_af(struct flowside *fs, int af, > > + const void *faddr, in_port_t fport, > > + const void *eaddr, in_port_t eport) > > +{ > > + inany_from_af(&fs->faddr, af, faddr); > > + inany_from_af(&fs->eaddr, af, eaddr); > > + fs->fport =3D fport; > > + fs->eport =3D eport; > > +} > > + > > +/** flowside_complete - Check if flowside is fully initialized > > + * @fs: flowside to check > > + */ > > +static inline bool flowside_complete(const struct flowside *fs) > > +{ > > + return !IN6_IS_ADDR_UNSPECIFIED(&fs->faddr) && > > + !IN6_IS_ADDR_UNSPECIFIED(&fs->eaddr) && > > + fs->fport !=3D 0 && fs->eport !=3D 0; >=20 > Zero is reserved for TCP (which could be problematic anyway if we try > to match things), The more practical consideration here is that a 0 port is used in the sockaddr passed to bind() to represent "pick a port for me". The point of this is to check that we have a "fully specified" flowside, that provides sufficient information to both bind() and connect() a socket with no ambiguity on the endpoints. > but for UDP a "zero" port can actually be used (in > the probably desuete sense of "no port"). [Aside: I've never encountered the word desuete before] By "no port" here are you meaning for UDP traffic that expects no response? If that's so we probably neither need or want to create a flow for it anyway. In any case, even if port 0 can be used at the protocol level, I don't think it can be used at the socket level: I'm pretty sure the bind() behaviour of treating 0 as "pick for me" is true for UDP as well as TCP - it's functionality that's basically necessary, and I can't see any other way to specify it. > Maybe we should use -1 > instead? That doesn't really help - port 65535 is itself valid, so unless we widen these fields - which I don't really want to do - it's still ambiguous (in fact, worse, because AFAICT port 65535 could actually be used in practice). Even if we did widen, it's still a problem, because if we got the port =66rom, for example, getsockname() on a not-yet-connected socket, that will give us 0 and the point of this function is to tell us that's not a fully specified endpoint. >=20 > > +} > > + > > +#define FLOWSIDE_STRLEN (2*(INET6_ADDRSTRLEN+8) + 6) >=20 > For consistency: "(2 * INET6_ADDRSTRLEN + 8) + 6)". Done. > Limited to the usage I've seen in 6/10 (maybe I'm ignoring something): > is it worth it to have flowside_fmt() as a function forming a string, > rather than something calling debug() with what we want...? At the > moment we have tap_packet_debug(), admittedly not very elegant but > perhaps more terse than this. There's at least one more use coming in the remainder of the series, and I'd expect to see more as other protocols are added to the flow mechanics. I also think it could be a very useful helper when adding ad-hoc debugging. So, yes, I think it's worth it. --=20 David Gibson | 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 --dQ0nEVRcNMXgv7gD Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmT5TAoACgkQzQJF27ox 2GeGRg/+OVNp/MnDjnd2B+syA8O/kEyNtPk5qrWjotlwxG4CnD62xN9tTxIcAfVL WfJGlmEAf28XQgWLySSSfl18Md+ioC6+2o1GSu1YGpD+U3Ya9gJVeZSPKztdA6m1 KMJvof0W4NAhIVZ1qz6fxUNNyxl6tv5E8LqnTzgsWhQalq+4SiAmog/w9tD7Pz5N sY7nR2jrU6H5IiWnAGpeztsUOqeO2FK9cWyWniUOlDTGyDE15jSU9iKDPJ0+Ut0e 9XqzfLs2+WWBYJ+tPeOPoCY71jsvTwM99+IVwG/WSozgQJk7Iqih+9iY3zJT0rrc 6v2Nl22isUSehEf6vzDcI83LCeDVcepRKd4h5f4wRag1G6DmJEv4ebv7OKvRojZH un76vI8gOIM0xp5KeXXkexlxkOMXGNNrOr5VsUmlzOvHaM17VVZON7x3akikUlhX PNLaDHi5yKoiXF+93l9JnkxtmKN70sLRXBm5TP+YHAR9mJ2OG0Pq5kBXw3cx4CRQ +rlAHvnFrU0HQ8wKPi8jJsJVN5hOOANS5bhGNbtX5mhnQrhGzQSDowqlo+k5sk/0 Mvho1xSNLURerfzQLMjc/t0c8QW5rOdnV9FdOPybjLC14Jc85USgGJeR80rn+Pop uaFkg/jnh20LkV1Usjh7T984j2eo2P4e1HJdMMo6E9H+J8ITpLU= =dFai -----END PGP SIGNATURE----- --dQ0nEVRcNMXgv7gD--