On Sat, May 18, 2024 at 12:13:45AM +0200, Stefano Brivio wrote:On Tue, 14 May 2024 11:03:36 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Good idea, changed. [snip]Currently the code to translate host side addresses and ports to guest side addresses and ports, and vice versa, is scattered across the TCP code. This includes both port redirection as controlled by the -t and -T options, and our special case NAT controlled by the --no-map-gw option. Gather this logic into fwd_from_*() functions for each input interface in fwd.c which take protocol and address information for the initiating side and generates the pif and address information for the forwarded side. This performs any NAT or port forwarding needed. We create a flow_forward() helper which applies those forwarding functions as needed to automatically move a flow from INI to FWD state. For now we leave the older flow_forward_af() function taking explicit addresses as a transitional tool. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- flow.c | 53 +++++++++++++++++++++++++ flow_table.h | 2 + fwd.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++ fwd.h | 12 ++++++ tcp.c | 102 +++++++++++++++-------------------------------- tcp_splice.c | 63 ++--------------------------- tcp_splice.h | 5 +-- 7 files changed, 213 insertions(+), 134 deletions(-) diff --git a/flow.c b/flow.c index 4942075..a6afe39 100644 --- a/flow.c +++ b/flow.c @@ -304,6 +304,59 @@ const struct flowside *flow_forward_af(union flow *flow, uint8_t pif, return fwd; } + +/** + * flow_forward() - Determine where flow should forward to, and move to FWD + * @c: Execution context + * @flow: Flow to forward + * @proto: Protocol + * + * Return: pointer to the forwarded flowside information + */ +const struct flowside *flow_forward(const struct ctx *c, union flow *flow, + uint8_t proto) +{ + char estr[INANY_ADDRSTRLEN], fstr[INANY_ADDRSTRLEN]; + struct flow_common *f = &flow->f; + const struct flowside *ini = &f->side[INISIDE]; + struct flowside *fwd = &f->side[FWDSIDE]; + uint8_t pif1 = PIF_NONE;This could now be 'pif_fwd' / 'pif_tgt', right?Ah, yes. I meant to go back and add these, but obviously forgot. Fixed now.diff --git a/fwd.c b/fwd.c index b3d5a37..5fe2361 100644 --- a/fwd.c +++ b/fwd.c @@ -25,6 +25,7 @@ #include "fwd.h" #include "passt.h" #include "lineread.h" +#include "flow_table.h" /* See enum in kernel's include/net/tcp_states.h */ #define UDP_LISTEN 0x07 @@ -154,3 +155,112 @@ void fwd_scan_ports_init(struct ctx *c) &c->tcp.fwd_out, &c->tcp.fwd_in); } } + +uint8_t fwd_from_tap(const struct ctx *c, uint8_t proto, + const struct flowside *a, struct flowside *b)A function comment would be nice to have, albeit a bit redundant.Now 'a' and 'b' could also be called 'ini' and 'tgt' I guess?Also a good idea, done.Ah.. right. So notionally we should set tgt->faddr here. However, because in this case we're forwarding to PIF_HOST we don't actually know tgt->faddr (or tgt->fport) without a getsockname() call, so we're leaving them blank. They will, in fact, be blank because we zero the entire entry in flow_alloc(). That's pretty non-obvious though, I'll change this to explicitly set faddr and fport with a comment.+{ + (void)proto; + + b->eaddr = a->faddr; + b->eport = a->fport; + + if (!c->no_map_gw) { + struct in_addr *v4 = inany_v4(&b->eaddr); + + if (v4 && IN4_ARE_ADDR_EQUAL(v4, &c->ip4.gw)) + *v4 = in4addr_loopback; + if (IN6_ARE_ADDR_EQUAL(&b->eaddr, &c->ip6.gw)) + b->eaddr.a6 = in6addr_loopback;I haven't tested this, but I'm a bit lost: I thought that in this case we would also set b->faddr here. Where does that happen?Only in the sense that if we accept()ed a connection from a loopback address on a socket not bound to a loopback address (or ANY), then the kernel has done something wrong. This kind of has the inverse of the issue above: we don't necessarily know the forwarding address here - we only know that with either a getsockname(), or by looking at the bound address of the listening socket (which might be unspecified).+ } + + return PIF_HOST; +} + +uint8_t fwd_from_splice(const struct ctx *c, uint8_t proto, + const struct flowside *a, struct flowside *b) +{ + const struct in_addr *ae4 = inany_v4(&a->eaddr); + + if (!inany_is_loopback(&a->eaddr) || + (!inany_is_loopback(&a->faddr) && !inany_is_unspecified(&a->faddr))) { + char estr[INANY_ADDRSTRLEN], fstr[INANY_ADDRSTRLEN]; + + debug("Non loopback address on %s: [%s]:%hu -> [%s]:%hu", + pif_name(PIF_SPLICE), + inany_ntop(&a->eaddr, estr, sizeof(estr)), a->eport, + inany_ntop(&a->faddr, fstr, sizeof(fstr)), a->fport); + return PIF_NONE; + } + + if (ae4) + inany_from_af(&b->eaddr, AF_INET, &in4addr_loopback); + else + inany_from_af(&b->eaddr, AF_INET6, &in6addr_loopback); + + b->eport = a->fport; + + if (proto == IPPROTO_TCP) + b->eport += c->tcp.fwd_out.delta[b->eport]; + + return PIF_HOST; +} + +uint8_t fwd_from_host(const struct ctx *c, uint8_t proto, + const struct flowside *a, struct flowside *b) +{ + struct in_addr *bf4; + + if (c->mode == MODE_PASTA && inany_is_loopback(&a->eaddr) && + proto == IPPROTO_TCP) { + /* spliceable */Before we conclude this, does f->pif[INISIDE] == PIF_HOST in the caller guarantee that inany_is_loopback(&a->faddr), too?If not, we shouldn't splice unless that's true as well.So I'm pretty confident what we do here is equivalent to what we did before. That might not be correct, but fixing that is for a different patch. Making problems like that more obvious is one of the advantages I expect for gathering all this forwarding logic into one place.Fair point. This was just because I thought my way through the two cases separately. I've made this stanza common. [snip]+ b->faddr = a->eaddr; + + if (inany_v4(&a->eaddr)) + inany_from_af(&b->eaddr, AF_INET, &in4addr_loopback); + else + inany_from_af(&b->eaddr, AF_INET6, &in6addr_loopback); + b->eport = a->fport; + if (proto == IPPROTO_TCP) + b->eport += c->tcp.fwd_in.delta[b->eport]; + + return PIF_SPLICE; + } + + b->faddr = a->eaddr; + b->fport = a->eport; + + bf4 = inany_v4(&b->faddr); + + if (bf4) { + if (IN4_IS_ADDR_LOOPBACK(bf4) || + IN4_IS_ADDR_UNSPECIFIED(bf4) || + IN4_ARE_ADDR_EQUAL(bf4, &c->ip4.addr_seen)) + *bf4 = c->ip4.gw; + } else { + struct in6_addr *bf6 = &b->faddr.a6; + + if (IN6_IS_ADDR_LOOPBACK(bf6) || + IN6_ARE_ADDR_EQUAL(bf6, &c->ip6.addr_seen) || + IN6_ARE_ADDR_EQUAL(bf6, &c->ip6.addr)) { + if (IN6_IS_ADDR_LINKLOCAL(&c->ip6.gw)) + *bf6 = c->ip6.gw; + else + *bf6 = c->ip6.addr_ll; + } + } + + if (bf4) { + inany_from_af(&b->eaddr, AF_INET, &c->ip4.addr_seen); + } else { + if (IN6_IS_ADDR_LINKLOCAL(&b->faddr.a6)) + b->eaddr.a6 = c->ip6.addr_ll_seen; + else + b->eaddr.a6 = c->ip6.addr_seen; + } + + b->eport = a->fport; + if (proto == IPPROTO_TCP) + b->eport += c->tcp.fwd_in.delta[b->eport];As we do this in any case, spliced or not spliced, I would find it less confusing to have these assignments in common, earlier (I just spent half an hour trying to figure out why you wouldn't set b->eport for the non-spliced case...).Looks like I already fixed that. [snip]+static void tcp_tap_conn_from_sock(struct ctx *c, union flow *flow, int s, const struct timespec *now) { - union inany_addr saddr, daddr; /* FIXME: avoid bulky temporaries */ - struct tcp_tap_conn *conn; - in_port_t srcport; + struct tcp_tap_conn *conn = FLOW_SET_TYPE(flow, FLOW_TCP, tcp); uint64_t hash; - inany_from_sockaddr(&saddr, &srcport, sa); - tcp_snat_inbound(c, &saddr); - - if (inany_v4(&saddr)) { - inany_from_af(&daddr, AF_INET, &c->ip4.addr_seen); - } else { - if (IN6_IS_ADDR_LINKLOCAL(&saddr)) - daddr.a6 = c->ip6.addr_ll_seen; - else - daddr.a6 = c->ip6.addr_seen; - } - dstport += c->tcp.fwd_in.delta[dstport]; - - flow_forward_af(flow, PIF_TAP, AF_INET6, - &saddr, srcport, &daddr, dstport); - conn = FLOW_SET_TYPE(flow, FLOW_TCP, tcp); - +Excess newline and tab.That's included in the new descriptions of the flow states. There might be a way to make it more obvious, but I'm not immediately sure of it. In any case the answer is: you can't cancel once you FLOW_ACTIVATE(). [snip] -- 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--- a/tcp_splice.c +++ b/tcp_splice.c @@ -395,71 +395,18 @@ static int tcp_conn_sock_ns(const struct ctx *c, sa_family_t af) /** * tcp_splice_conn_from_sock() - Attempt to init state for a spliced connection * @c: Execution context - * @pif0: pif id of side 0 - * @dstport: Side 0 destination port of connection * @flow: flow to initialise * @s0: Accepted (side 0) socket * @sa: Peer address of connection * - * Return: true if able to create a spliced connection, false otherwiseNot related to this patch, but I think we should probably describe in the theory of operation for flows what's the threshold between calling flow_alloc_cancel() on a flow (which would imply returning something here, in case tcp_splice_connect() fails), and deferring that instead to a CLOSING state.