From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from imap.gmail.com [173.194.76.109] by localhost with POP3 (fetchmail-6.3.26) for (single-drop); Sun, 19 May 2024 09:04:40 +0200 (CEST) Received: by 2002:a05:6a10:271d:b0:547:81a2:46ee with SMTP id ib29csp6373746pxb; Sun, 19 May 2024 00:04:12 -0700 (PDT) X-Forwarded-Encrypted: i=2; AJvYcCX00Qw6SaHiyd3BYX8mcaeO5c+UI/ZzqvyjC2ZH6pNQ3A+XkH71SO6HYWqC/9Clin36RZyY78iOXjSHR29lJj0Opsmcupd0jSQ= X-Google-Smtp-Source: AGHT+IGDxfwIANr8CuxLjxIiRkQlCDlxAar0xPEL9wrV8FHmDkzx7IjiavPXFyI8no3vIGwygRBH X-Received: by 2002:a05:620a:2a0d:b0:792:9da4:9edc with SMTP id af79cd13be357-792c75abf48mr2989639385a.34.1716102252070; Sun, 19 May 2024 00:04:12 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1716102252; cv=none; d=google.com; s=arc-20160816; b=A486l/1IU6jbn+RDYjfNtACmms4fB/mcsHqxhOxvEfEeKA7iZGz8liOTSH+dRI7kDH /8Zi/lcOvGzkFOycKEVyonXUoSvDuBM2JiRB+2SrTqedDanKrFPKBja2F+q3+EofWaIh JjClz/wS7lTo+rnMiCYIpgkNSVimywglnaL2bFtGllCy41MKWYXXdneI7nN51tpPSNAm LL1xVyZioiURrRYlvlcswjhTQJX2Z2pHBYtOQ39RJUNT1oNsuQ2c4k5djw0FoE+Cgj4k PALhrVEzCCjJVGF4cu/qJfSVTXxm/FBqdbzHwbeBo6DejDizr4im+sO2/qnsK9vBn5rX Fltw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:dkim-signature:delivered-to; bh=vwCUzKojBo0LuZIjwiiHxrjg6ummyonFEnJ690CpipQ=; fh=htis3z6nDPD3FFPmKX7SGQyhLv1B9tDlnJYWrC8d10o=; b=yn/dNIulwZAkdNI57JOGnHrVU30rP8QvFXhDQcahOoNdmKoqJy6PPe9qLr17W/O3dZ eD9Kk47yNpgvEUr2sgLXMBJaSmFGTbxVIQr1i6mPg+bndb0LApwOzbSDIQkyp3TabtKV XI0F1aZI5EDEs0kVlccf6vOhWusq/FGBA5levhBKuWqvos7nsFnJOdzf9Vtq7pv6nuQt R9WNYlBbX0OJ7ebtEQeWhVglgtxRyhjZ3Q9I6vXGTBzpZq76qhY2GGvS316xYzMfNtOG aQnS/RgCM5ofk2UQEXgDOsaFWNAKpN9CfAbdNd3Leb8hZsgjYrAHSl4V5dnqui76y91G QDLg==; dara=google.com ARC-Authentication-Results: i=1; mx.google.com; dkim=temperror (no key for signature) header.i=@gibson.dropbear.id.au header.s=202312 header.b=pLWm6RID; spf=pass (google.com: domain of dgibson@gandalf.ozlabs.org designates 150.107.74.76 as permitted sender) smtp.mailfrom=dgibson@gandalf.ozlabs.org Return-Path: Received: from us-smtp-inbound-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com. [205.139.110.120]) by mx.google.com with ESMTPS id af79cd13be357-792bf33b4f4si2461913085a.487.2024.05.19.00.04.11 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 19 May 2024 00:04:11 -0700 (PDT) Received-SPF: pass (google.com: domain of dgibson@gandalf.ozlabs.org designates 150.107.74.76 as permitted sender) client-ip=150.107.74.76; Authentication-Results: mx.google.com; dkim=temperror (no key for signature) header.i=@gibson.dropbear.id.au header.s=202312 header.b=pLWm6RID; spf=pass (google.com: domain of dgibson@gandalf.ozlabs.org designates 150.107.74.76 as permitted sender) smtp.mailfrom=dgibson@gandalf.ozlabs.org Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-120-aimxRWzDNDGMGik8ZGeVxA-1; Sun, 19 May 2024 03:04:09 -0400 X-MC-Unique: aimxRWzDNDGMGik8ZGeVxA-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 2A91F800994 for ; Sun, 19 May 2024 07:04:09 +0000 (UTC) Received: by smtp.corp.redhat.com (Postfix) id 27504740F; Sun, 19 May 2024 07:04:09 +0000 (UTC) Received: from mimecast-mx02.redhat.com (mimecast04.extmail.prod.ext.rdu2.redhat.com [10.11.55.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E2EEA51BF for ; Sun, 19 May 2024 07:04:08 +0000 (UTC) Received: from us-smtp-inbound-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [170.10.128.131]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id C8E5D101A525 for ; Sun, 19 May 2024 07:04:08 +0000 (UTC) Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-651-KCegCC2kNTmWMaBw_i-nBA-1; Sun, 19 May 2024 03:04:05 -0400 X-MC-Unique: KCegCC2kNTmWMaBw_i-nBA-1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1716102240; bh=vwCUzKojBo0LuZIjwiiHxrjg6ummyonFEnJ690CpipQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=pLWm6RIDX/r+41WH3Bx77K8f1xoapcrkamVq/VzwVMlGnbqoBpocL/BttfM4OOVvA kCoFT05nU1PHdIhkWogdJ2cOpSL70vjup6Ee6F11WkV5rXgIloljOC0Nyym4b21d1P CG9ajsxB0O5GZKBO86f+3U+7pcKiaqGLkz3SQKJImZuzE8BopHRGl+tHtPcVn9ld7y MC7y1DPNxkyryl0fgRm0z8JAK66k9SXoF489o/jcMcXJd1n2B9IaUqqdqmy7VzSo2T +C4q9/BE69tUi2YM4vnARMAmyOe+MEtaF/uDKdWjKzfSV29oFTxf8sHpM80/w4iqNq anZ7MdIhlSBwQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4VhsB83VZNz4wbh; Sun, 19 May 2024 17:04:00 +1000 (AEST) Date: Sat, 18 May 2024 17:00:30 +1000 From: David Gibson To: Stefano Brivio Cc: passt-dev@passt.top Subject: Re: [PATCH v5 06/19] flow: Populate address information for initiating side Message-ID: References: <20240514010337.1104606-1-david@gibson.dropbear.id.au> <20240514010337.1104606-7-david@gibson.dropbear.id.au> <20240516202337.1b90e5f2@elisabeth> <20240517215845.4d09eaae@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="FnDbN8JPG4JaTpnV" Content-Disposition: inline In-Reply-To: <20240517215845.4d09eaae@elisabeth> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.5 List-Id: --FnDbN8JPG4JaTpnV Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, May 17, 2024 at 09:58:45PM +0200, Stefano Brivio wrote: > On Fri, 17 May 2024 14:27:46 +1000 > David Gibson wrote: >=20 > > On Thu, May 16, 2024 at 08:23:37PM +0200, Stefano Brivio wrote: > > > On Tue, 14 May 2024 11:03:24 +1000 > > > David Gibson wrote: > > > =20 > > > > This requires the address and port information for the initiating s= ide be > > > > populated when a flow enters INI state. Implement that for TCP and= ICMP. > > > >=20 > > > > For now this leaves some information redundantly recorded in both g= eneric > > > > and type specific fields. We'll fix that in later patches. > > > >=20 > > > > Signed-off-by: David Gibson > > > > --- > > > > flow.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++= +--- > > > > flow_table.h | 8 ++++- > > > > icmp.c | 10 ++++-- > > > > tcp.c | 4 +-- > > > > 4 files changed, 103 insertions(+), 11 deletions(-) > > > >=20 > > > > diff --git a/flow.c b/flow.c > > > > index aee2736..3d5b3a5 100644 > > > > --- a/flow.c > > > > +++ b/flow.c > > > > @@ -108,6 +108,31 @@ static const union flow *flow_new_entry; /* = =3D NULL */ > > > > /* 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 =20 > > >=20 > > > This starts showing a mild inconsistency: > > >=20 > > > - initiating / forwarding sides =20 > >=20 > > So I was trying to go with "initiating" and "forwarded" sides here, > > distinct from "forwarding" used for addresses. But the potential for > > confusion between "forwarded" and "forwarding" is way too high, I > > agree. > >=20 > > I considered "accepting side", but I don't like that because a) in the > > case of UDP "accepting" isn't really a thing and b) there's no > > guarantee that the peer on that side will actually "accept" the > > "connection". > >=20 > > "receiving side" has all those problems and more. > >=20 > > > - endpoint / forwarding addresses > > >=20 > > > The initiating side can correspond to the endpoint address ("ours") or > > > to the forwarding address ("theirs"). =20 > >=20 > > Not sure what you mean by that - both initiating side and forwarded > > side each have both an endpoint and a forwarding address. > >=20 > > > The forwarding side doesn't necessarily correspond to the forwarding > > > address. =20 > >=20 > > Right :/ > >=20 > > > But I have no better ideas than "initiating" and "forwarding" for > > > sides. Should we consider something different for addresses and ports, > > > such as "ours" and "theirs"? =20 > >=20 > > I dislike those because I think it's not always clear if "us" is just > > pasta, or includes the guest. Plus the distinction here is between > > the side that initiates the flow and the.. other one.. that doesn't > > correspond to "ours" or "theirs" in any consistent way I can see. > >=20 > > > Local/remote might be misleading as well. > > > Peer, passt and pasta all share the same initial. =20 > >=20 > > "initiating side" / "target side" maybe? Or just "primary side" / > > "secondary side" or "A side" / "B side"? > >=20 > > Actually, I think I like "target" - I feel like "flow_target()" > > conveys what "flow_forward()" does pretty well. I'll go with that > > pending any further suggestions you have. >=20 > Yes, "target" sounds pretty good to me as well. So we would have > "endpoint" and "forwarding" addresses, but "initiating" and "target" > side, right? Yes, and all four combinations are valid. We have an initiating endpoint address, an initiating forwarding address, a target forwarding address and a target endpoint address. > An alternative could be "client" / "server", it should be always > correct, but it would sound a bit strange for ICMP and UDP. I tend to agree. > > [snip] > > > > @@ -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_FWD) > > > > - flow_log_(f, LOG_DEBUG, "%s =3D> %s", pif_name(f->pif[INISIDE]), > > > > - pif_name(f->pif[FWDSIDE])); > > > > + 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[FWDSIDE])); > > > > 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 > > >=20 > > > If I review v2 of "util, tcp: Add helper to display socket addresses", > > > can I skip reviewing this? :) =20 > >=20 > > Alas, no - this isn't based on a socket address. However, I have been > > looking at introducing a flowside_ntop() function which would help in > > a similar way here. >=20 > Right, I realised only later that your patch only took care of socket > addresses... anyway, reviewed, this hunk also looks correct to me. It turns out to be harder to write / use flowside_ntop() than I thought. In the current log messages, the two flowsides are actually displayed in reverse order: endpoint then forwarding for the initiating side, forwarding then endpoint for the target side. That gives the overall order that makes the most sense to me, because it's the order the payload of the initiating packet goes through. But that means we'd need an order parameter for flowside_ntop() or something, and it all becomes a bit of a mess. > > > > } > > > > =20 > > > > /** > > > > - * flow_initiate() - Move flow to INI state, setting INISIDE detai= ls > > > > + * flow_initiate_() - Move flow to INI state, setting pif[INISIDE] > > > > * @flow: Flow to change state > > > > * @pif: pif of the initiating side > > > > */ > > > > -void flow_initiate(union flow *flow, uint8_t pif) > > > > +static void flow_initiate_(union flow *flow, uint8_t pif) =20 > > >=20 > > > I don't feel like the underscore here is really necessary. =20 > >=20 > > Yeah, I'm trying to convey that it's not safe to call this directly - > > it won't perform a correct state transition without the extra logic in > > flow_initiate_af() and flow_initiate_sa(). >=20 > Ah, I see now, okay. >=20 --=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 --FnDbN8JPG4JaTpnV Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZIUg0ACgkQzQJF27ox 2GdVBw//WDX8NwQDQVvs1+WsCoxvyFptHvXvRBkEsGzYNJO/LLNSDdxsRt2YoYwF iv8bla/4n133IVIEgBORM/EWbbPTRvKNQ//59Kmwxg/nwVnDsFT+4zs3P8z7DJI8 Oda7KsvXlZdE9XMaEjfdrc6daHrHxmf5PL44c6+/KzMuURfmBwvyrcxVGQM5Pk/t bORzHeWrnNcC6EbT3/oURf1l9l5eXu8Qj8ebi2wjXp3oXLn43RutdFKsPbriQHDG u1v8ImQ9Tbkx7NwfOvomuPRXnXxAyyEQR75eQiuZ54AQlb8w95a349D+XMBMYYwB fmjjO6BZFiNCB4rHaDGDde/sXIJdEQ69QK5ZrodAkuhubgLduXpyqNkK08vEfmFK T3lHIOsQ5B/ZQLBly5oD/nMQCdn85PAkHweEtmHivv6fcsG0vxaMn9TzNeerqyst I5/daYlsxZbKe4ADPiYPRtit3xMHOC9IpsXjrfBSRwwMuF6F0KlZM74ah8rx/yVZ q6IeXorTch3xct6+UgrfVcB3qMSK4m/zpPd2MM1tOeNw4DdvHpWl72iw2usPMRMG tdBILeyqKu8J6/RBAR8J1gpx2Lmy8If9JI2CrDEeFqmqyEjfviw3S4aKJcddBszA 140JiXN961QjLo+adjDmbEfh6ZZvPIOw96j6p59JQNuilFNNUw8= =RIgz -----END PGP SIGNATURE----- --FnDbN8JPG4JaTpnV--