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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5069 bytes --]

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

Well, there are different strength of requirements here.  We simply
can't proceed without an IPv4 address here: if we tried we'd either
NULL pointer dereference from inany_v4() or we'd take a garbage chunk
out of an IPv6 address.

For the IPv6 case, if the address are v4 it means we'll send ICMPv6
packets with IPv4 mapped addresses in them.  That's kinda weird, but
not necessarily wrong.

> 
> > 
> > [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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]