On Sat, May 18, 2024 at 12:14:08AM +0200, Stefano Brivio wrote:On Tue, 14 May 2024 11:03:37 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:It would, and I've thought about it, but haven't seen a great way to go about it. * The flow type is not set at this point, so we can't use that. We can't trivially move setting the type earlier, because for TCP at least we need the information from flow_froward() to determine if we're spliced and set the type based on that. * Including both flow type and protocol in flow_common is annoyingly redundant, as well as adding a full 32-bits to the structure because of padding. * We could possibly eliminate flow type and make it implicit based on the protocol+pifs: regular TCP flow is (TCP, HOST, TAP) or (TCP, TAP, HOST), whereas TCP spliced is (TCP, HOST, SPLICE) or (TCP, SPLICE, HOST). Type is the selector field for which of the union variants is valid, and I don't love that being something with a kind of complicated calculation behind it. * We could make the type field hold the protocol until FLOW_SET_TYPE(), but I don't love semantics of a field changing like that. * We could just pass either a protocol number or a string to flow_forward() etc., but that seems a bit awkward. Hrm... actually thinking on that last one. It might make sense to add a descriptive string to flow_initiate(), not just the protocol but something like "TCP SYN" versus "TCP accept()" or the like. That wouldn't directly help flow_forward(), but the info line from flow_initiate() is likely to be in close proximity, so it would help.Current ICMP hard codes its forwarding rules, and never applies any translations. Change it to use the flow_forward() function, so that it's translated the same as TCP (excluding TCP specific port redirection). This means that gw mapping now applies to ICMP so "ping <gw address>" will now ping the host's loopback instead of the actual gw machine. This removes the surprising behaviour that the target you ping might not be the same as you connect to with TCP. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- flow.c | 1 + icmp.c | 14 ++++++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/flow.c b/flow.c index a6afe39..b43a079 100644 --- a/flow.c +++ b/flow.c @@ -285,6 +285,7 @@ const struct flowside *flow_initiate_sa(union flow *flow, uint8_t pif, * * Return: pointer to the forwarded flowside information */ +/* cppcheck-suppress unusedFunction */ const struct flowside *flow_forward_af(union flow *flow, uint8_t pif, sa_family_t af, const void *saddr, in_port_t sport, diff --git a/icmp.c b/icmp.c index 0112fd9..6310178 100644 --- a/icmp.c +++ b/icmp.c @@ -153,6 +153,7 @@ static struct icmp_ping_flow *icmp_ping_new(const struct ctx *c, sa_family_t af, uint16_t id, const void *saddr, const void *daddr) { + uint8_t proto = af == AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6; uint8_t flowtype = af == AF_INET ? FLOW_PING4 : FLOW_PING6; union epoll_ref ref = { .type = EPOLL_TYPE_PING }; union flow *flow = flow_alloc(); @@ -163,9 +164,18 @@ static struct icmp_ping_flow *icmp_ping_new(const struct ctx *c, if (!flow) return NULL; - flow_initiate_af(flow, PIF_TAP, af, saddr, id, daddr, id); - flow_forward_af(flow, PIF_HOST, af, NULL, 0, daddr, 0); + if (!flow_forward(c, flow, proto)) + goto cancel; + + if (flow->f.pif[FWDSIDE] != PIF_HOST) { + flow_err(flow, "No support for forwarding %s from %s to %s", + proto == IPPROTO_ICMP ? "ICMP" : "ICMPv6",Which brings me to two remarks: - having the protocol name also in the flow_err() message printed in flow_forward() could be helpful- then, perhaps, we should re-introduce ip_proto_str[] which was dropped with 340164445341 ("epoll: Generalize epoll_ref to cover things other than sockets")Maybe. I guess the standard way to do that is with getprotobyname(3), but that probably won't work in our isolated namespace, I guess. -- 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