On Fri, May 17, 2024 at 10:11:23PM +0200, Stefano Brivio wrote:
On Fri, 17 May 2024 16:58:38 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:
On Thu, May 16, 2024 at 10:53:50PM +0200, Stefano
Brivio wrote:
On Tue, 14 May 2024 11:03:33 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:
icmp_sock_handler() obtains the guest address
from it's most recently
observed IP, and the ICMP id from the epoll reference. Both of these
can be obtained readily from the flow.
icmp_tap_handler() builds its socket address for sendto() directly
from the destination address supplied by the incoming tap packet.
This can instead be generated from the flow.
struct icmp_ping_flow contains a field for the ICMP id of the ping, but
this is now redundant, since the id is also stored as the "port" in the
common flowsides.
Using the flowsides as the common source of truth here prepares us for
allowing more flexible NAT and forwarding by properly initialising
that flowside information.
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
icmp.c | 37 ++++++++++++++++++++++---------------
icmp_flow.h | 1 -
tap.c | 11 -----------
tap.h | 1 -
4 files changed, 22 insertions(+), 28 deletions(-)
diff --git a/icmp.c b/icmp.c
index 37a3586..1e9a05e 100644
--- a/icmp.c
+++ b/icmp.c
@@ -58,6 +58,7 @@ static struct icmp_ping_flow *icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS];
void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
{
struct icmp_ping_flow *pingf = PINGF(ref.flowside.flow);
+ const struct flowside *ini = &pingf->f.side[INISIDE];
union sockaddr_inany sr;
socklen_t sl = sizeof(sr);
char buf[USHRT_MAX];
@@ -83,7 +84,7 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
goto unexpected;
/* Adjust packet back to guest-side ID */
- ih4->un.echo.id = htons(pingf->id);
+ ih4->un.echo.id = htons(ini->eport);
seq = ntohs(ih4->un.echo.sequence);
} else if (pingf->f.type == FLOW_PING6) {
struct icmp6hdr *ih6 = (struct icmp6hdr *)buf;
@@ -93,7 +94,7 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
goto unexpected;
/* Adjust packet back to guest-side ID */
- ih6->icmp6_identifier = htons(pingf->id);
+ ih6->icmp6_identifier = htons(ini->eport);
seq = ntohs(ih6->icmp6_sequence);
} else {
ASSERT(0);
@@ -108,13 +109,20 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
}
flow_dbg(pingf, "echo reply to tap, ID: %"PRIu16", seq: %"PRIu16,
- pingf->id, seq);
+ ini->eport, seq);
- if (pingf->f.type == FLOW_PING4)
- tap_icmp4_send(c, sr.sa4.sin_addr, tap_ip4_daddr(c), buf, n);
- else if (pingf->f.type == FLOW_PING6)
- tap_icmp6_send(c, &sr.sa6.sin6_addr,
- tap_ip6_daddr(c, &sr.sa6.sin6_addr), buf, n);
+ if (pingf->f.type == FLOW_PING4) {
+ const struct in_addr *saddr = inany_v4(&ini->faddr);
+ const struct in_addr *daddr = inany_v4(&ini->eaddr);
+
+ ASSERT(saddr && daddr); /* Must have IPv4 addresses */
...are those somehow special compared to IPv6 ones?
Well, we're about to call a function that's specific to IPv4 ICMP and
will require IPv4 addresses for both ends.
Yes, I understand that part, but I was wondering why "Must have IPv4
addresses" and not IPv6 ones below. On the other hand, due to how the
inany thing works, we'll always have IPv6 addresses "set" there.
[snip]
diff
--git a/icmp_flow.h b/icmp_flow.h
index 5a2eed9..f053211 100644
--- a/icmp_flow.h
+++ b/icmp_flow.h
@@ -22,7 +22,6 @@ struct icmp_ping_flow {
int seq;
int sock;
time_t ts;
- uint16_t id;
Nit: drop 'id' from struct comment.
Fixed, thanks.
--
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