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.
> [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.
> > > }
> > >
> > > /**
> > > - * 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.
--
Stefano