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:

...

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:

...

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 --- 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 port

This starts showing a mild inconsistency:

- initiating / forwarding sides

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.

...

- 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.

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));

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? :)

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.

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.

...

...

...

}

/** - * 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.

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().

Ah, I see now, okay.

-- 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