Re: [PATCH v2 08/12] icmp: Share more between IPv4 and IPv6 paths in icmp_tap_handler()

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

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

On Sat, Jan 06, 2024 at 05:00:21PM +0100, Stefano Brivio wrote:
> On Thu, 21 Dec 2023 17:53:23 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Currently icmp_tap_handler() consists of two almost disjoint paths for the
> > IPv4 and IPv6 cases.  The only thing they share is an error message.
> > We can use some intermediate variables to refactor this to share some more
> > code between those paths.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  icmp.c | 140 ++++++++++++++++++++++++++++-----------------------------
> >  1 file changed, 69 insertions(+), 71 deletions(-)
> > 
> > diff --git a/icmp.c b/icmp.c
> > index 02739b9..f6c744a 100644
> > --- a/icmp.c
> > +++ b/icmp.c
> > @@ -154,102 +154,100 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
> >  		     const void *saddr, const void *daddr,
> >  		     const struct pool *p, const struct timespec *now)
> >  {
> > +	uint8_t proto = af == AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6;
> > +	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
> > +	union {
> > +		struct sockaddr sa;
> > +		struct sockaddr_in sa4;
> > +		struct sockaddr_in6 sa6;
> > +	} sa = { .sa.sa_family = af };
> > +	const socklen_t sl = af == AF_INET ? sizeof(sa.sa4) : sizeof(sa.sa6);
> > +	struct icmp_id_sock *id_map;
> > +	uint16_t id, seq;
> >  	size_t plen;
> > +	void *pkt;
> > +	int s;
> >  
> >  	(void)saddr;
> >  	(void)pif;
> >  
> > +	pkt = packet_get(p, 0, 0, 0, &plen);
> > +	if (!pkt)
> > +		return 1;
> > +
> >  	if (af == AF_INET) {
> > -		struct sockaddr_in sa = {
> > -			.sin_family = AF_INET,
> > -		};
> > -		union icmp_epoll_ref iref;
> > -		struct icmphdr *ih;
> > -		int id, s;
> > -
> > -		ih = packet_get(p, 0, 0, sizeof(*ih), &plen);
> > -		if (!ih)
> > +		struct icmphdr *ih = (struct icmphdr *)pkt;
> > +
> > +		if (plen < sizeof(*ih))
> >  			return 1;
> 
> This is obviously the same as the existing check. On the other hand,
> I've been trying to use packet_get() to validate any packet read length
> we need.
> 
> Here, the first call sets plen, but the only purpose is to get the
> length of the message we need to relay. Given that we need at least
> sizeof(*ih) here, I would prefer keep the explicit packet_get()
> validation.
> 
> Same for the IPv6 path below.

Ok, fair enough.  Updated.

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