Re: [PATCH v5 06/19] flow: Populate address information for initiating side

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7230 bytes --]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]