Re: [PATCH v11 8/9] tap: change signature of function tap_push_l2h()

[Stefano Brivio <sbrivio@redhat.com>]

On Sat, 27 Sep 2025 15:25:21 -0400
Jon Maloy <jmaloy@redhat.com> wrote:

> In the next commit 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 any logical changes.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> ---
> v3: -Improved comment for src_mac argument, as suggested by Stefano.
> v4: -Re-added comment in tap.c and remove redundant argument check
>      in tap_push_l2()
> v5: -Ensured that MAC argument to tap_push_l2h() never is NULL.
> v6: -Clarification of comment about ARP lookup
> ---
>  tap.c | 16 +++++++++-------
>  tap.h |  3 ++-
>  tcp.c |  5 +++--
>  3 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index 250a0f6..ec7e525 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -171,17 +171,19 @@ 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.

Trailing . which we never use.

>   * @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 */
> +	/* TODO: ARP lookup on tap side */

Why? I mean, what would we do with guest-side MAC addresses, as we
don't control Layer-2 host-side?

>  	memcpy(eh->h_dest, c->guest_mac, ETH_ALEN);
> -	memcpy(eh->h_source, c->our_tap_mac, ETH_ALEN);
> +	memcpy(eh->h_source, src_mac, ETH_ALEN);
>  	eh->h_proto = ntohs(proto);
>  	return eh + 1;
>  }
> @@ -261,7 +263,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, c->our_tap_mac, 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 +283,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, c->our_tap_mac, ETH_P_IP);
>  	struct icmphdr *icmp4h = tap_push_ip4h(ip4h, src, dst,
>  					       l4len, IPPROTO_ICMP);
>  
> @@ -367,7 +369,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, c->our_tap_mac, 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 +391,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, c->our_tap_mac, 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 21db4d2..02f7761 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 af05d35..b874317 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1978,7 +1978,8 @@ 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, c->our_tap_mac,
> +						  ETH_P_IP);
>  		const struct in_addr *rst_src = daddr;
>  		const struct in_addr *rst_dst = saddr;
>  
> @@ -1988,7 +1989,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, c->our_tap_mac, ETH_P_IPV6);

This can and should be wrapped to 80 columns.

>  		const struct in6_addr *rst_src = daddr;
>  		const struct in6_addr *rst_dst = saddr;
>  

-- 
Stefano