Re: [PATCH v3 6/8] tap: change signature of function tap_push_l2h()

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

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

On Sun, Jun 29, 2025 at 01:13:45PM -0400, Jon Maloy wrote:
> In the following commits it must be possible for the callers of
> function tap_push_l2h() to specify which source MAC address should
> be added to the ethernet header sent over the tap interface. As
> a preparation, we now add a new argument to that function, still
> without actually using it.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> 
> ---
> v3: Improved comment for src_mac argument, as suggested by Stefano.
> ---
>  tap.c | 18 +++++++++++-------
>  tap.h |  3 ++-
>  tcp.c |  4 ++--
>  3 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index 6db5d88..ceb9d9f 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -171,17 +171,21 @@ const struct in6_addr *tap_ip6_daddr(const struct ctx *c,
>   * tap_push_l2h() - Build an L2 header for an inbound packet
>   * @c:		Execution context
>   * @buf:	Buffer address at which to generate header
> + * @src_mac:	MAC address to be used as source for message. NULL means default
>   * @proto:	Ethernet protocol number for L3
>   *
>   * Return: pointer at which to write the packet's payload
>   */
> -void *tap_push_l2h(const struct ctx *c, void *buf, uint16_t proto)
> +void *tap_push_l2h(const struct ctx *c, void *buf,
> +		   const void *src_mac, uint16_t proto)
>  {
>  	struct ethhdr *eh = (struct ethhdr *)buf;
>  
> -	/* TODO: ARP table lookup */

I think the comment should stay.  To my understanding this was never
talking about this ARP lookup of the peer, but about sending ARPs to
the guest to get its MAC, rather than just knowing it from other
channels.

>  	memcpy(eh->h_dest, c->guest_mac, ETH_ALEN);
> -	memcpy(eh->h_source, c->our_tap_mac, ETH_ALEN);
> +	if (src_mac)
> +		memcpy(eh->h_source, src_mac, ETH_ALEN);

I'm not sure this NULL handling adds much.  The callers can pass
&c->our_tap_mac explicitly if that's what they want, with not much
loss of brevity.

> +	else
> +		memcpy(eh->h_source, c->our_tap_mac, ETH_ALEN);
>  	eh->h_proto = ntohs(proto);
>  	return eh + 1;
>  }
> @@ -261,7 +265,7 @@ void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport,
>  {
>  	size_t l4len = dlen + sizeof(struct udphdr);
>  	char buf[USHRT_MAX];
> -	struct iphdr *ip4h = tap_push_l2h(c, buf, ETH_P_IP);
> +	struct iphdr *ip4h = tap_push_l2h(c, buf, NULL, ETH_P_IP);
>  	struct udphdr *uh = tap_push_ip4h(ip4h, src, dst, l4len, IPPROTO_UDP);
>  	char *data = tap_push_uh4(uh, src, sport, dst, dport, in, dlen);
>  
> @@ -281,7 +285,7 @@ void tap_icmp4_send(const struct ctx *c, struct in_addr src, struct in_addr dst,
>  		    const void *in, size_t l4len)
>  {
>  	char buf[USHRT_MAX];
> -	struct iphdr *ip4h = tap_push_l2h(c, buf, ETH_P_IP);
> +	struct iphdr *ip4h = tap_push_l2h(c, buf, NULL, ETH_P_IP);
>  	struct icmphdr *icmp4h = tap_push_ip4h(ip4h, src, dst,
>  					       l4len, IPPROTO_ICMP);
>  
> @@ -367,7 +371,7 @@ void tap_udp6_send(const struct ctx *c,
>  {
>  	size_t l4len = dlen + sizeof(struct udphdr);
>  	char buf[USHRT_MAX];
> -	struct ipv6hdr *ip6h = tap_push_l2h(c, buf, ETH_P_IPV6);
> +	struct ipv6hdr *ip6h = tap_push_l2h(c, buf, NULL, ETH_P_IPV6);
>  	struct udphdr *uh = tap_push_ip6h(ip6h, src, dst,
>  					  l4len, IPPROTO_UDP, flow);
>  	char *data = tap_push_uh6(uh, src, sport, dst, dport, in, dlen);
> @@ -389,7 +393,7 @@ void tap_icmp6_send(const struct ctx *c,
>  		    const void *in, size_t l4len)
>  {
>  	char buf[USHRT_MAX];
> -	struct ipv6hdr *ip6h = tap_push_l2h(c, buf, ETH_P_IPV6);
> +	struct ipv6hdr *ip6h = tap_push_l2h(c, buf, NULL, ETH_P_IPV6);
>  	struct icmp6hdr *icmp6h = tap_push_ip6h(ip6h, src, dst, l4len,
>  						IPPROTO_ICMPV6, 0);
>  
> diff --git a/tap.h b/tap.h
> index 6fe3d15..e640d95 100644
> --- a/tap.h
> +++ b/tap.h
> @@ -70,7 +70,8 @@ static inline void tap_hdr_update(struct tap_hdr *thdr, size_t l2len)
>  }
>  
>  unsigned long tap_l2_max_len(const struct ctx *c);
> -void *tap_push_l2h(const struct ctx *c, void *buf, uint16_t proto);
> +void *tap_push_l2h(const struct ctx *c, void *buf,
> +		   const void *src_mac, uint16_t proto);
>  void *tap_push_ip4h(struct iphdr *ip4h, struct in_addr src,
>  		     struct in_addr dst, size_t l4len, uint8_t proto);
>  void *tap_push_uh4(struct udphdr *uh, struct in_addr src, in_port_t sport,
> diff --git a/tcp.c b/tcp.c
> index 057ee93..3ecf9e8 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1898,7 +1898,7 @@ static void tcp_rst_no_conn(const struct ctx *c, int af,
>  		return;
>  
>  	if (af == AF_INET) {
> -		struct iphdr *ip4h = tap_push_l2h(c, buf, ETH_P_IP);
> +		struct iphdr *ip4h = tap_push_l2h(c, buf, NULL, ETH_P_IP);
>  		const struct in_addr *rst_src = daddr;
>  		const struct in_addr *rst_dst = saddr;
>  
> @@ -1908,7 +1908,7 @@ static void tcp_rst_no_conn(const struct ctx *c, int af,
>  					      *rst_src, *rst_dst);
>  
>  	} else {
> -		struct ipv6hdr *ip6h = tap_push_l2h(c, buf, ETH_P_IPV6);
> +		struct ipv6hdr *ip6h = tap_push_l2h(c, buf, NULL, ETH_P_IPV6);
>  		const struct in6_addr *rst_src = daddr;
>  		const struct in6_addr *rst_dst = saddr;
>  

-- 
David Gibson (he or they)	| 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 --]