On Fri, 17 May 2024 14:27:46 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Thu, May 16, 2024 at 08:23:37PM +0200, Stefano Brivio wrote:Yes, "target" sounds pretty good to me as well. So we would have "endpoint" and "forwarding" addresses, but "initiating" and "target" side, right? An alternative could be "client" / "server", it should be always correct, but it would sound a bit strange for ICMP and UDP.On Tue, 14 May 2024 11:03:24 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote: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. 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". "receiving side" has all those problems and more.This requires the address and port information for the initiating side be populated when a flow enters INI 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. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- flow.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++--- flow_table.h | 8 ++++- icmp.c | 10 ++++-- tcp.c | 4 +-- 4 files changed, 103 insertions(+), 11 deletions(-) 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; /* = NULL */ /* Last time the flow timers ran */ static struct timespec flow_timer_run; +/** 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 portThis starts showing a mild inconsistency: - initiating / forwarding sides- endpoint / forwarding addresses The initiating side can correspond to the endpoint address ("ours") or to the forwarding address ("theirs").Not sure what you mean by that - both initiating side and forwarded side each have both an endpoint and a forwarding address.The forwarding side doesn't necessarily correspond to the forwarding address.Right :/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"?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.Local/remote might be misleading as well. Peer, passt and pasta all share the same initial."initiating side" / "target side" maybe? Or just "primary side" / "secondary side" or "A side" / "B side"? 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.[snip]Right, I realised only later that your patch only took care of socket addresses... anyway, reviewed, this hunk also looks correct to me.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.@@ -150,18 +177,28 @@ static void flow_set_state(struct flow_common *f, enum flow_state state) FLOW_STATE(f)); if (MAX(state, oldstate) >= FLOW_STATE_FWD) - flow_log_(f, LOG_DEBUG, "%s => %s", pif_name(f->pif[INISIDE]), - pif_name(f->pif[FWDSIDE])); + flow_log_(f, LOG_DEBUG, "%s [%s]:%hu -> [%s]:%hu => %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) >= FLOW_STATE_INI) - flow_log_(f, LOG_DEBUG, "%s => ?", pif_name(f->pif[INISIDE])); + flow_log_(f, LOG_DEBUG, "%s [%s]:%hu -> [%s]:%hu => ?", + pif_name(f->pif[INISIDE]), + inany_ntop(&ini->eaddr, estr, sizeof(estr)), + ini->eport, + inany_ntop(&ini->faddr, fstr, sizeof(fstr)), + ini->fport);If I review v2 of "util, tcp: Add helper to display socket addresses", can I skip reviewing this? :)Ah, I see now, okay. -- StefanoYeah, 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().} /** - * flow_initiate() - Move flow to INI state, setting INISIDE details + * 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)I don't feel like the underscore here is really necessary.