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 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 > > --- > > 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 > > #include > > +#include > > > > #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] = "", > > @@ -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