Re: [PATCH v5 15/19] icmp: Use flowsides as the source of truth wherever possible

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.
  
-- 
Stefano