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

[Stefano Brivio <sbrivio@redhat.com>]

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

> + * @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.

> + */
> +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;
> +}

-- 
Stefano