Re: [PATCH v7 02/27] flow: Common address information for target side

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

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

On Wed, Jul 10, 2024 at 11:30:38PM +0200, Stefano Brivio wrote:
> Two minor details:
> 
> On Fri,  5 Jul 2024 12:06:59 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Require the address and port information for the target (non
> > initiating) side to be populated when a flow enters TGT 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.
> > 
> > For TCP we now use the information from the flow to construct the
> > destination socket address in both tcp_conn_from_tap() and
> > tcp_splice_connect().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  flow.c       | 38 ++++++++++++++++++------
> >  flow_table.h |  5 +++-
> >  icmp.c       |  3 +-
> >  inany.h      |  1 -
> >  pif.c        | 45 ++++++++++++++++++++++++++++
> >  pif.h        | 17 +++++++++++
> >  tcp.c        | 82 ++++++++++++++++++++++++++++------------------------
> >  tcp_splice.c | 45 +++++++++++-----------------
> >  8 files changed, 158 insertions(+), 78 deletions(-)
> > 
> > diff --git a/flow.c b/flow.c
> > index 44e7b3b8..f064fad1 100644
> > --- a/flow.c
> > +++ b/flow.c
> > @@ -165,8 +165,10 @@ void flow_log_(const struct flow_common *f, int pri, const char *fmt, ...)
> >   */
> >  static void flow_set_state(struct flow_common *f, enum flow_state state)
> >  {
> > -	char estr[INANY_ADDRSTRLEN], fstr[INANY_ADDRSTRLEN];
> > +	char estr0[INANY_ADDRSTRLEN], fstr0[INANY_ADDRSTRLEN];
> > +	char estr1[INANY_ADDRSTRLEN], fstr1[INANY_ADDRSTRLEN];
> >  	const struct flowside *ini = &f->side[INISIDE];
> > +	const struct flowside *tgt = &f->side[TGTSIDE];
> >  	uint8_t oldstate = f->state;
> >  
> >  	ASSERT(state < FLOW_NUM_STATES);
> > @@ -177,19 +179,24 @@ static void flow_set_state(struct flow_common *f, enum flow_state state)
> >  		  FLOW_STATE(f));
> >  
> >  	if (MAX(state, oldstate) >= FLOW_STATE_TGT)
> > -		flow_log_(f, LOG_DEBUG, "%s [%s]:%hu -> [%s]:%hu => %s",
> > +		flow_log_(f, LOG_DEBUG,
> > +			  "%s [%s]:%hu -> [%s]:%hu => %s [%s]:%hu -> [%s]:%hu",
> >  			  pif_name(f->pif[INISIDE]),
> > -			  inany_ntop(&ini->eaddr, estr, sizeof(estr)),
> > +			  inany_ntop(&ini->eaddr, estr0, sizeof(estr0)),
> >  			  ini->eport,
> > -			  inany_ntop(&ini->faddr, fstr, sizeof(fstr)),
> > +			  inany_ntop(&ini->faddr, fstr0, sizeof(fstr0)),
> >  			  ini->fport,
> > -			  pif_name(f->pif[TGTSIDE]));
> > +			  pif_name(f->pif[TGTSIDE]),
> > +			  inany_ntop(&tgt->faddr, fstr1, sizeof(fstr1)),
> > +			  tgt->fport,
> > +			  inany_ntop(&tgt->eaddr, estr1, sizeof(estr1)),
> > +			  tgt->eport);
> >  	else if (MAX(state, oldstate) >= FLOW_STATE_INI)
> >  		flow_log_(f, LOG_DEBUG, "%s [%s]:%hu -> [%s]:%hu => ?",
> >  			  pif_name(f->pif[INISIDE]),
> > -			  inany_ntop(&ini->eaddr, estr, sizeof(estr)),
> > +			  inany_ntop(&ini->eaddr, estr0, sizeof(estr0)),
> >  			  ini->eport,
> > -			  inany_ntop(&ini->faddr, fstr, sizeof(fstr)),
> > +			  inany_ntop(&ini->faddr, fstr0, sizeof(fstr0)),
> >  			  ini->fport);
> >  }
> >  
> > @@ -261,21 +268,34 @@ const struct flowside *flow_initiate_sa(union flow *flow, uint8_t pif,
> >  }
> >  
> >  /**
> > - * flow_target() - Move flow to TGT, setting TGTSIDE details
> > + * flow_target_af() - Move flow to TGT, setting TGTSIDE details
> >   * @flow:	Flow to change state
> >   * @pif:	pif of the target side
> > + * @af:		Address family for @eaddr and @faddr
> > + * @saddr:	Source address (pointer to in_addr or in6_addr)
> > + * @sport:	Endpoint port
> > + * @daddr:	Destination address (pointer to in_addr or in6_addr)
> > + * @dport:	Destination port
> > + *
> > + * Return: pointer to the target flowside information
> >   */
> > -void flow_target(union flow *flow, uint8_t pif)
> > +const struct flowside *flow_target_af(union flow *flow, uint8_t pif,
> > +				      sa_family_t af,
> > +				      const void *saddr, in_port_t sport,
> > +				      const void *daddr, in_port_t dport)
> >  {
> >  	struct flow_common *f = &flow->f;
> > +	struct flowside *tgt = &f->side[TGTSIDE];
> >  
> >  	ASSERT(pif != PIF_NONE);
> >  	ASSERT(flow_new_entry == flow && f->state == FLOW_STATE_INI);
> >  	ASSERT(f->type == FLOW_TYPE_NONE);
> >  	ASSERT(f->pif[INISIDE] != PIF_NONE && f->pif[TGTSIDE] == PIF_NONE);
> >  
> > +	flowside_from_af(tgt, af, daddr, dport, saddr, sport);
> >  	f->pif[TGTSIDE] = pif;
> >  	flow_set_state(f, FLOW_STATE_TGT);
> > +	return tgt;
> >  }
> >  
> >  /**
> > diff --git a/flow_table.h b/flow_table.h
> > index ad1bc787..00dca4b2 100644
> > --- a/flow_table.h
> > +++ b/flow_table.h
> > @@ -114,7 +114,10 @@ const struct flowside *flow_initiate_af(union flow *flow, uint8_t pif,
> >  const struct flowside *flow_initiate_sa(union flow *flow, uint8_t pif,
> >  					const union sockaddr_inany *ssa,
> >  					in_port_t dport);
> > -void flow_target(union flow *flow, uint8_t pif);
> > +const struct flowside *flow_target_af(union flow *flow, uint8_t pif,
> > +				      sa_family_t af,
> > +				      const void *saddr, in_port_t sport,
> > +				      const void *daddr, in_port_t dport);
> >  
> >  union flow *flow_set_type(union flow *flow, enum flow_type type);
> >  #define FLOW_SET_TYPE(flow_, t_, var_)	(&flow_set_type((flow_), (t_))->var_)
> > diff --git a/icmp.c b/icmp.c
> > index cf88ac1f..fd92c7da 100644
> > --- a/icmp.c
> > +++ b/icmp.c
> > @@ -167,7 +167,8 @@ static struct icmp_ping_flow *icmp_ping_new(const struct ctx *c,
> >  		return NULL;
> >  
> >  	flow_initiate_af(flow, PIF_TAP, af, saddr, id, daddr, id);
> > -	flow_target(flow, PIF_HOST);
> > +	/* FIXME: Record outbound source address when known */
> > +	flow_target_af(flow, PIF_HOST,	af, NULL, 0, daddr, 0);
> >  	pingf = FLOW_SET_TYPE(flow, flowtype, ping);
> >  
> >  	pingf->seq = -1;
> > diff --git a/inany.h b/inany.h
> > index 47b66fa9..8eaf5335 100644
> > --- a/inany.h
> > +++ b/inany.h
> > @@ -187,7 +187,6 @@ static inline bool inany_is_unspecified(const union inany_addr *a)
> >   *
> >   * Return: true if @a is in fe80::/10 (IPv6 link local unicast)
> >   */
> > -/* cppcheck-suppress unusedFunction */
> >  static inline bool inany_is_linklocal6(const union inany_addr *a)
> >  {
> >  	return IN6_IS_ADDR_LINKLOCAL(&a->a6);
> > diff --git a/pif.c b/pif.c
> > index ebf01cc8..9f2d39cc 100644
> > --- a/pif.c
> > +++ b/pif.c
> > @@ -7,9 +7,14 @@
> >  
> >  #include <stdint.h>
> >  #include <assert.h>
> > +#include <netinet/in.h>
> >  
> >  #include "util.h"
> >  #include "pif.h"
> > +#include "siphash.h"
> > +#include "ip.h"
> > +#include "inany.h"
> > +#include "passt.h"
> >  
> >  const char *pif_type_str[] = {
> >  	[PIF_NONE]		= "<none>",
> > @@ -19,3 +24,43 @@ const char *pif_type_str[] = {
> >  };
> >  static_assert(ARRAY_SIZE(pif_type_str) == PIF_NUM_TYPES,
> >  	      "pif_type_str[] doesn't match enum pif_type");
> > +
> > +
> > +/** pif_sockaddr() - Construct a socket address suitable for an interface
> > + * @c:		Execution context
> > + * @sa:		Pointer to sockaddr to fill in
> > + * @sl:		Updated to relevant of length of initialised @sa
> 
> to relevant length

Done.

> > + * @pif:	Interface to create the socket address
> > + * @addr:	IPv[46] address
> > + * @port:	Port (host byte order)
> > + *
> > + * Return: true if resulting socket address is non-trivial (specified address or
> > + *         non-zero port), false otherwise
> 
> This is not really intuitive in the only caller using this,
> tcp_bind_outbound(). I wonder if it would make more sense to perform
> this check directly there, and have this returning void instead.

Yeah, done.  When I implemented the return value I thought I was going
to want it in more places than turned out to be the case.

> > + */
> > +bool pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa, socklen_t *sl,
> > +		  uint8_t pif, const union inany_addr *addr, in_port_t port)
> > +{
> > +	const struct in_addr *v4 = inany_v4(addr);
> > +
> > +	ASSERT(pif_is_socket(pif));
> > +
> > +	if (v4) {
> > +		sa->sa_family = AF_INET;
> > +		sa->sa4.sin_addr = *v4;
> > +		sa->sa4.sin_port = htons(port);
> > +		memset(&sa->sa4.sin_zero, 0, sizeof(sa->sa4.sin_zero));
> > +		*sl = sizeof(sa->sa4);
> > +		return !IN4_IS_ADDR_UNSPECIFIED(v4) || port;
> > +	}
> > +
> > +	sa->sa_family = AF_INET6;
> > +	sa->sa6.sin6_addr = addr->a6;
> > +	sa->sa6.sin6_port = htons(port);
> > +	if (pif == PIF_HOST && IN6_IS_ADDR_LINKLOCAL(&addr->a6))
> > +		sa->sa6.sin6_scope_id = c->ifi6;
> > +	else
> > +		sa->sa6.sin6_scope_id = 0;
> > +	sa->sa6.sin6_flowinfo = 0;
> > +	*sl = sizeof(sa->sa6);
> > +	return !IN6_IS_ADDR_UNSPECIFIED(&addr->a6) || port;
> > +}
> 

-- 
David Gibson (he or they)	| 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 --]