On Fri, May 17, 2024 at 09:58:45PM +0200, Stefano Brivio wrote:
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:
On Tue, 14 May 2024 11:03:24 +1000
David Gibson <david(a)gibson.dropbear.id.au> 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 <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 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?
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