Re: [PATCH v5 4/8] udp: refactor UDP header update functions

[Stefano Brivio <sbrivio@redhat.com>]

On Wed,  5 Jun 2024 17:21:25 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> This commit refactors the udp_update_hdr4() and udp_update_hdr6() functions
> to improve code portability by replacing the udp_meta_t parameter with
> more specific parameters for the IPv4 and IPv6 headers (iphdr/ipv6hdr)
> and the source socket address (sockaddr_in/sockaddr_in6).
> It also moves the tap_hdr_update() function call inside the udp_tap_send()
> function not to have to pass the TAP header to udp_update_hdr4() and
> udp_update_hdr6()
> 
> This refactor reduces complexity by making the functions more modular and
> ensuring that each function operates on more narrowly scoped data structures.
> This will facilitate future backend introduction like vhost-user.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  udp.c | 60 +++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 34 insertions(+), 26 deletions(-)
> 
> diff --git a/udp.c b/udp.c
> index 3abafc994537..4295d48046a6 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -556,7 +556,8 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start, unsigned n,
>  /**
>   * udp_update_hdr4() - Update headers for one IPv4 datagram
>   * @c:		Execution context
> - * @bm:		Pointer to udp_meta_t to update
> + * @ip4h:	Pre-filled IPv4 header (except for tot_len and saddr)
> + * @s_in:	Source socket address, filled in by recvmmsg()
>   * @bp:		Pointer to udp_payload_t to update
>   * @dstport:	Destination port number
>   * @dlen:	Length of UDP payload
> @@ -565,15 +566,16 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start, unsigned n,
>   * Return: size of IPv4 payload (UDP header + data)
>   */
>  static size_t udp_update_hdr4(const struct ctx *c,
> -			      struct udp_meta_t *bm, struct udp_payload_t *bp,
> +			      struct iphdr *ip4h, const struct sockaddr_in *s_in,
> +			      struct udp_payload_t *bp,
>  			      in_port_t dstport, size_t dlen,
>  			      const struct timespec *now)
>  {
> -	in_port_t srcport = ntohs(bm->s_in.sa4.sin_port);
> +	in_port_t srcport = ntohs(s_in->sin_port);
>  	const struct in_addr dst = c->ip4.addr_seen;
> -	struct in_addr src = bm->s_in.sa4.sin_addr;
> +	struct in_addr src = s_in->sin_addr;
>  	size_t l4len = dlen + sizeof(bp->uh);
> -	size_t l3len = l4len + sizeof(bm->ip4h);
> +	size_t l3len = l4len + sizeof(*ip4h);

Nit: while at it, it would be nice to reorder declarations from longest
to shortest, as well.

>  
>  	if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match) &&
>  	    IN4_ARE_ADDR_EQUAL(&src, &c->ip4.dns_host) && srcport == 53 &&
> @@ -594,24 +596,24 @@ static size_t udp_update_hdr4(const struct ctx *c,
>  		src = c->ip4.gw;
>  	}
>  
> -	bm->ip4h.tot_len = htons(l3len);
> -	bm->ip4h.daddr = dst.s_addr;
> -	bm->ip4h.saddr = src.s_addr;
> -	bm->ip4h.check = csum_ip4_header(l3len, IPPROTO_UDP, src, dst);
> +	ip4h->tot_len = htons(l3len);
> +	ip4h->daddr = dst.s_addr;
> +	ip4h->saddr = src.s_addr;
> +	ip4h->check = csum_ip4_header(l3len, IPPROTO_UDP, src, dst);
>  
> -	bp->uh.source = bm->s_in.sa4.sin_port;
> +	bp->uh.source = s_in->sin_port;
>  	bp->uh.dest = htons(dstport);
>  	bp->uh.len = htons(l4len);
>  	csum_udp4(&bp->uh, src, dst, bp->data, dlen);
>  
> -	tap_hdr_update(&bm->taph, l3len + sizeof(udp4_eth_hdr));
>  	return l4len;
>  }
>  
>  /**
>   * udp_update_hdr6() - Update headers for one IPv6 datagram
>   * @c:		Execution context
> - * @bm:		Pointer to udp_meta_t to update
> + * @ip6h:	Pre-filled IPv6 header (except for payload_len and addresses)
> + * @s_in:	Source socket address, filled in by recvmmsg()
>   * @bp:		Pointer to udp_payload_t to update
>   * @dstport:	Destination port number
>   * @dlen:	Length of UDP payload
> @@ -620,13 +622,14 @@ static size_t udp_update_hdr4(const struct ctx *c,
>   * Return: size of IPv6 payload (UDP header + data)
>   */
>  static size_t udp_update_hdr6(const struct ctx *c,
> -			      struct udp_meta_t *bm, struct udp_payload_t *bp,
> +			      struct ipv6hdr *ip6h, struct sockaddr_in6 *s_in6,
> +			      struct udp_payload_t *bp,
>  			      in_port_t dstport, size_t dlen,
>  			      const struct timespec *now)
>  {
> -	const struct in6_addr *src = &bm->s_in.sa6.sin6_addr;
> +	const struct in6_addr *src = &s_in6->sin6_addr;
>  	const struct in6_addr *dst = &c->ip6.addr_seen;
> -	in_port_t srcport = ntohs(bm->s_in.sa6.sin6_port);
> +	in_port_t srcport = ntohs(s_in6->sin6_port);

Same here.

>  	uint16_t l4len = dlen + sizeof(bp->uh);
>  
>  	if (IN6_IS_ADDR_LINKLOCAL(src)) {
> @@ -663,19 +666,18 @@ static size_t udp_update_hdr6(const struct ctx *c,
>  
>  	}
>  
> -	bm->ip6h.payload_len = htons(l4len);
> -	bm->ip6h.daddr = *dst;
> -	bm->ip6h.saddr = *src;
> -	bm->ip6h.version = 6;
> -	bm->ip6h.nexthdr = IPPROTO_UDP;
> -	bm->ip6h.hop_limit = 255;
> +	ip6h->payload_len = htons(l4len);
> +	ip6h->daddr = *dst;
> +	ip6h->saddr = *src;
> +	ip6h->version = 6;
> +	ip6h->nexthdr = IPPROTO_UDP;
> +	ip6h->hop_limit = 255;
>  
> -	bp->uh.source = bm->s_in.sa6.sin6_port;
> +	bp->uh.source = s_in6->sin6_port;
>  	bp->uh.dest = htons(dstport);
> -	bp->uh.len = bm->ip6h.payload_len;
> +	bp->uh.len = ip6h->payload_len;
>  	csum_udp6(&bp->uh, src, dst, bp->data, dlen);
>  
> -	tap_hdr_update(&bm->taph, l4len + sizeof(bm->ip6h) + sizeof(udp6_eth_hdr));
>  	return l4len;
>  }
>  
> @@ -708,11 +710,17 @@ static void udp_tap_send(const struct ctx *c,
>  		size_t l4len;
>  
>  		if (v6) {
> -			l4len = udp_update_hdr6(c, bm, bp, dstport,
> +			l4len = udp_update_hdr6(c, &bm->ip6h,
> +						&bm->s_in.sa6, bp, dstport,
>  						udp6_l2_mh_sock[i].msg_len, now);
> +			tap_hdr_update(&bm->taph, l4len + sizeof(bm->ip6h) +
> +					     sizeof(udp6_eth_hdr));

You're summing sizeof(udp6_eth_hdr) to l4len + sizeof(bm->ip6h), so
this should be aligned as follows:

			tap_hdr_update(&bm->taph, l4len + sizeof(bm->ip6h) +
						  sizeof(udp6_eth_hdr));

>  		} else {
> -			l4len = udp_update_hdr4(c, bm, bp, dstport,
> +			l4len = udp_update_hdr4(c, &bm->ip4h,
> +						&bm->s_in.sa4, bp, dstport,
>  						udp4_l2_mh_sock[i].msg_len, now);
> +			tap_hdr_update(&bm->taph, l4len + sizeof(bm->ip4h) +
> +					     sizeof(udp4_eth_hdr));

Same here:
			tap_hdr_update(&bm->taph, l4len + sizeof(bm->ip4h) +
						  sizeof(udp4_eth_hdr));

>  		}
>  		tap_iov[i][UDP_IOV_PAYLOAD].iov_len = l4len;
>  	}

-- 
Stefano