Re: [PATCH v2 10/14] tap: Split tap_ip6_send() into UDP and ICMP variants

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 19 Oct 2022 11:43:53 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> tap_ip6_send() has special case logic to compute the checksums for UDP
> and ICMP packets, which is a mild layering violation.  By using a suitable
> helper we can split it into tap_udp6_send() and tap_icmp6_send() functions
> without greatly increasing the code size, this removing that layering
> violation.
> 
> We make some small changes to the interface while there.  In both cases
> we make the destination IPv6 address a parameter, which will be useful
> later.  For the UDP variant we make it take just the UDP payload, and it
> will generate the UDP header.  For the ICMP variant we pass in the ICMP
> header as before.  The inconsistency is because that's what seems to be
> the more natural way to invoke the function in the callers in each case.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  dhcpv6.c | 21 +++------------
>  icmp.c   |  3 ++-
>  tap.c    | 82 ++++++++++++++++++++++++++++++++++++++++++--------------
>  tap.h    |  9 +++++--
>  4 files changed, 75 insertions(+), 40 deletions(-)
> 
> diff --git a/dhcpv6.c b/dhcpv6.c
> index 7829968..e763aed 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -208,15 +208,8 @@ struct msg_hdr {
>  	uint32_t xid:24;
>  } __attribute__((__packed__));
>  
> -#if __BYTE_ORDER == __BIG_ENDIAN
> -#define UH_RESP {{{ 547, 546, 0, 0, }}}
> -#else
> -#define UH_RESP {{{ __bswap_constant_16(547), __bswap_constant_16(546), 0, 0 }}}
> -#endif
> -
>  /**
>   * struct resp_t - Normal advertise and reply message
> - * @uh:			UDP header
>   * @hdr:		DHCP message header
>   * @server_id:		Server Identifier option
>   * @ia_na:		Non-temporary Address option
> @@ -226,7 +219,6 @@ struct msg_hdr {
>   * @dns_search:		Domain Search List, here just for storage size
>   */
>  static struct resp_t {
> -	struct udphdr  uh;
>  	struct msg_hdr hdr;
>  
>  	struct opt_server_id server_id;
> @@ -236,7 +228,6 @@ static struct resp_t {
>  	struct opt_dns_servers dns_servers;
>  	struct opt_dns_search dns_search;
>  } __attribute__((__packed__)) resp = {
> -	UH_RESP,
>  	{ 0 },
>  	SERVER_ID,
>  
> @@ -270,13 +261,11 @@ static const struct opt_status_code sc_not_on_link = {
>  
>  /**
>   * struct resp_not_on_link_t - NotOnLink error (mandated by RFC 8415, 18.3.2.)
> - * @uh:		UDP header
>   * @hdr:	DHCP message header
>   * @server_id:	Server Identifier option
>   * @var:	Payload: IA_NA from client, status code, client ID
>   */
>  static struct resp_not_on_link_t {
> -	struct udphdr  uh;
>  	struct msg_hdr hdr;
>  
>  	struct opt_server_id server_id;
> @@ -284,7 +273,6 @@ static struct resp_not_on_link_t {
>  	uint8_t var[sizeof(struct opt_ia_na) + sizeof(struct opt_status_code) +
>  		    sizeof(struct opt_client_id)];
>  } __attribute__((__packed__)) resp_not_on_link = {
> -	UH_RESP,
>  	{ TYPE_REPLY, 0 },
>  	SERVER_ID,
>  	{ 0, },
> @@ -527,12 +515,11 @@ int dhcpv6(struct ctx *c, const struct pool *p,
>  			n += sizeof(struct opt_hdr) + ntohs(client_id->l);
>  
>  			n = offsetof(struct resp_not_on_link_t, var) + n;
> -			resp_not_on_link.uh.len = htons(n);
>  
>  			resp_not_on_link.hdr.xid = mh->xid;
>  
> -			tap_ip6_send(c, src, IPPROTO_UDP,
> -				     (char *)&resp_not_on_link, n, mh->xid);
> +			tap_udp6_send(c, src, 547, tap_ip6_daddr(c, src), 546,
> +				      mh->xid, &resp_not_on_link, n);
>  
>  			return 1;
>  		}
> @@ -576,11 +563,11 @@ int dhcpv6(struct ctx *c, const struct pool *p,
>  	n = offsetof(struct resp_t, client_id) +
>  	    sizeof(struct opt_hdr) + ntohs(client_id->l);
>  	n = dhcpv6_dns_fill(c, (char *)&resp, n);
> -	resp.uh.len = htons(n);
>  
>  	resp.hdr.xid = mh->xid;
>  
> -	tap_ip6_send(c, src, IPPROTO_UDP, (char *)&resp, n, mh->xid);
> +	tap_udp6_send(c, src, 547, tap_ip6_daddr(c, src), 546,
> +		      mh->xid, &resp, n);
>  	c->ip6.addr_seen = c->ip6.addr;
>  
>  	return 1;
> diff --git a/icmp.c b/icmp.c
> index 61c2d90..6493ea9 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -105,7 +105,8 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref,
>  			icmp_id_map[V6][id].seq = seq;
>  		}
>  
> -		tap_ip6_send(c, &sr6->sin6_addr, IPPROTO_ICMPV6, buf, n, 0);
> +		tap_icmp6_send(c, &sr6->sin6_addr,
> +			       tap_ip6_daddr(c, &sr6->sin6_addr), buf, n);
>  	} else {
>  		struct sockaddr_in *sr4 = (struct sockaddr_in *)&sr;
>  		struct icmphdr *ih = (struct icmphdr *)buf;
> diff --git a/tap.c b/tap.c
> index 0e8c99b..135d799 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -175,21 +175,22 @@ void tap_ip4_send(const struct ctx *c, in_addr_t src, uint8_t proto,
>  }
>  
>  /**
> - * tap_ip6_send() - Send IPv6 packet, with L2 headers, calculating L3/L4 checksums
> + * tap_push_ip6h() - Build IPv6 header for inbound packet
>   * @c:		Execution context
>   * @src:	IPv6 source address
> - * @proto:	L4 protocol number
> - * @in:		Payload
> + * @dst:	IPv6 destination address
>   * @len:	L4 payload length
> - * @flow:	Flow label
> + * @proto:	L4 protocol number
> + * @flow:	IPv6 flow identifier
> + *
> + * Return: pointer at which to write the packet's payload
>   */
> -void tap_ip6_send(const struct ctx *c, const struct in6_addr *src,
> -		  uint8_t proto, const char *in, size_t len, uint32_t flow)
> +static void *tap_push_ip6h(char *buf,

Sorry, I missed this during review, but this function doesn't take @c
anymore, and the comment change didn't cover all the argument changes.
It takes a buffer pointer which is not entirely obvious.

You changed this later in 1ebe787fe460 ("tap: Simplify some casts in
the tap "slow path" functions"), but that commit doesn't update the
comment either.

Same for tap_push_ip4h(). I just realised as I accidentally passed an
"input" header to it.

> +			   const struct in6_addr *src,
> +			   const struct in6_addr *dst,
> +			   size_t len, uint8_t proto, uint32_t flow)
>  {
> -	char buf[USHRT_MAX];
> -	struct ipv6hdr *ip6h =
> -		(struct ipv6hdr *)tap_push_l2h(c, buf, ETH_P_IPV6);
> -	char *data = (char *)(ip6h + 1);
> +	struct ipv6hdr *ip6h = (struct ipv6hdr *)buf;
>  
>  	ip6h->payload_len = htons(len);
>  	ip6h->priority = 0;
> @@ -197,24 +198,65 @@ void tap_ip6_send(const struct ctx *c, const struct in6_addr *src,
>  	ip6h->nexthdr = proto;
>  	ip6h->hop_limit = 255;
>  	ip6h->saddr = *src;
> -	ip6h->daddr = *tap_ip6_daddr(c, src);
> +	ip6h->daddr = *dst;
>  	ip6h->flow_lbl[0] = (flow >> 16) & 0xf;
>  	ip6h->flow_lbl[1] = (flow >> 8) & 0xff;
>  	ip6h->flow_lbl[2] = (flow >> 0) & 0xff;
> +	return ip6h + 1;
> +}
>  
> +/**
> + * tap_udp6_send() - Send UDP over IPv6 packet
> + * @c:		Execution context
> + * @src:	IPv6 source address
> + * @sport:	UDP source port
> + * @dst:	IPv6 destination address
> + * @dport:	UDP destination port
> + * @flow:	Flow label
> + * @in:		UDP payload contents (not including UDP header)
> + * @len:	UDP payload length (not including UDP header)
> + */
> +void tap_udp6_send(const struct ctx *c,
> +		   const struct in6_addr *src, in_port_t sport,
> +		   const struct in6_addr *dst, in_port_t dport,
> +		   uint32_t flow, const void *in, size_t len)
> +{
> +	size_t udplen = len + sizeof(struct udphdr);
> +	char buf[USHRT_MAX];
> +	void *ip6h = tap_push_l2h(c, buf, ETH_P_IPV6);
> +	void *uhp = tap_push_ip6h(ip6h, src, dst, udplen, IPPROTO_UDP, flow);
> +	struct udphdr *uh = (struct udphdr *)uhp;
> +	char *data = (char *)(uh + 1);
> +
> +	uh->source = htons(sport);
> +	uh->dest = htons(dport);
> +	uh->len = htons(udplen);
> +	csum_udp6(uh, src, dst, in, len);
>  	memcpy(data, in, len);
>  
> -	if (proto == IPPROTO_UDP) {
> -		struct udphdr *uh = (struct udphdr *)(ip6h + 1);
> +	if (tap_send(c, buf, len + (data - buf)) < 1)
> +		debug("tap: failed to send %lu bytes (IPv6)", len);
> +}
>  
> -		csum_udp6(uh, &ip6h->saddr, &ip6h->daddr,
> -			  uh + 1, len - sizeof(*uh));
> -	} else if (proto == IPPROTO_ICMPV6) {
> -		struct icmp6hdr *ih = (struct icmp6hdr *)(ip6h + 1);
> +/**
> + * tap_icmp6_send() - Send ICMPv6 packet
> + * @c:		Execution context
> + * @src:	IPv6 source address
> + * @dst:	IPv6 destination address
> + * @in:		ICMP packet, including ICMP header
> + * @len:	ICMP packet length, including ICMP header
> + */
> +void tap_icmp6_send(const struct ctx *c,
> +		    const struct in6_addr *src, const struct in6_addr *dst,
> +		    void *in, size_t len)
> +{
> +	char buf[USHRT_MAX];
> +	void *ip6h = tap_push_l2h(c, buf, ETH_P_IPV6);
> +	char *data = tap_push_ip6h(ip6h, src, dst, len, IPPROTO_ICMPV6, 0);
> +	struct icmp6hdr *icmp6h = (struct icmp6hdr *)data;
>  
> -		csum_icmp6(ih, &ip6h->saddr, &ip6h->daddr,
> -			   ih + 1, len - sizeof(*ih));
> -	}
> +	memcpy(data, in, len);
> +	csum_icmp6(icmp6h, src, dst, icmp6h + 1, len - sizeof(*icmp6h));
>  
>  	if (tap_send(c, buf, len + (data - buf)) < 1)
>  		debug("tap: failed to send %lu bytes (IPv6)", len);
> diff --git a/tap.h b/tap.h
> index 011ba8e..d43c7a0 100644
> --- a/tap.h
> +++ b/tap.h
> @@ -11,8 +11,13 @@ const struct in6_addr *tap_ip6_daddr(const struct ctx *c,
>  				     const struct in6_addr *src);
>  void tap_ip4_send(const struct ctx *c, in_addr_t src, uint8_t proto,
>  		  const char *in, size_t len);
> -void tap_ip6_send(const struct ctx *c, const struct in6_addr *src,
> -		  uint8_t proto, const char *in, size_t len, uint32_t flow);
> +void tap_udp6_send(const struct ctx *c,
> +		   const struct in6_addr *src, in_port_t sport,
> +		   const struct in6_addr *dst, in_port_t dport,
> +		   uint32_t flow, const void *in, size_t len);
> +void tap_icmp6_send(const struct ctx *c,
> +		    const struct in6_addr *src, const struct in6_addr *dst,
> +		    void *in, size_t len);
>  int tap_send(const struct ctx *c, const void *data, size_t len);
>  void tap_handler(struct ctx *c, int fd, uint32_t events,
>  		 const struct timespec *now);

-- 
Stefano