Re: [PATCH v4 8/9] icmp: let icmp use mac address from flowside structure

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

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

On Tue, Aug 19, 2025 at 11:10:04PM -0400, Jon Maloy wrote:
> Even ICMP needs to be updated to use the external MAC address instead
> of just the own tap address when applicable. We do that here.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>  icmp.c |  4 ++--
>  ndp.c  |  2 +-
>  tap.c  | 10 ++++++----
>  tap.h  |  4 ++--
>  udp.c  | 12 ++++++++----
>  5 files changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/icmp.c b/icmp.c
> index 95f38c1..aa42cc3 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -129,12 +129,12 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
>  		const struct in_addr *daddr = inany_v4(&ini->eaddr);
>  
>  		ASSERT(saddr && daddr); /* Must have IPv4 addresses */
> -		tap_icmp4_send(c, *saddr, *daddr, buf, n);
> +		tap_icmp4_send(c, *saddr, *daddr, buf, pingf->f.tap_omac, n);
>  	} else if (pingf->f.type == FLOW_PING6) {
>  		const struct in6_addr *saddr = &ini->oaddr.a6;
>  		const struct in6_addr *daddr = &ini->eaddr.a6;
>  
> -		tap_icmp6_send(c, saddr, daddr, buf, n);
> +		tap_icmp6_send(c, saddr, daddr, buf, pingf->f.tap_omac, n);
>  	}
>  	return;
>  
> diff --git a/ndp.c b/ndp.c
> index 9912f80..19b9b28 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -185,7 +185,7 @@ static void ndp_send(const struct ctx *c, const struct in6_addr *dst,
>  {
>  	const struct in6_addr *src = &c->ip6.our_tap_ll;
>  
> -	tap_icmp6_send(c, src, dst, buf, l4len);
> +	tap_icmp6_send(c, src, dst, buf, c->our_tap_mac, l4len);
>  }
>  
>  /**
> diff --git a/tap.c b/tap.c
> index 0349835..443928a 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -278,13 +278,14 @@ void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport,
>   * @src:	IPv4 source address
>   * @dst:	IPv4 destination address
>   * @in:		ICMP packet, including ICMP header
> + * @src_mac:	MAC address to be used as source for message
>   * @l4len:	ICMP packet length, including ICMP header
>   */
>  void tap_icmp4_send(const struct ctx *c, struct in_addr src, struct in_addr dst,
> -		    const void *in, size_t l4len)
> +		    const void *in, const void *src_mac, size_t l4len)
>  {
>  	char buf[USHRT_MAX];
> -	struct iphdr *ip4h = tap_push_l2h(c, buf, NULL, ETH_P_IP);
> +	struct iphdr *ip4h = tap_push_l2h(c, buf, src_mac, ETH_P_IP);
>  	struct icmphdr *icmp4h = tap_push_ip4h(ip4h, src, dst,
>  					       l4len, IPPROTO_ICMP);
>  
> @@ -385,14 +386,15 @@ void tap_udp6_send(const struct ctx *c,
>   * @src:	IPv6 source address
>   * @dst:	IPv6 destination address
>   * @in:		ICMP packet, including ICMP header
> + * @src_mac:	MAC address to be used as source for message
>   * @l4len:	ICMP packet length, including ICMP header
>   */
>  void tap_icmp6_send(const struct ctx *c,
>  		    const struct in6_addr *src, const struct in6_addr *dst,
> -		    const void *in, size_t l4len)
> +		    const void *in, const void *src_mac, size_t l4len)
>  {
>  	char buf[USHRT_MAX];
> -	struct ipv6hdr *ip6h = tap_push_l2h(c, buf, NULL, ETH_P_IPV6);
> +	struct ipv6hdr *ip6h = tap_push_l2h(c, buf, src_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 6ec171d..ce0b9a6 100644
> --- a/tap.h
> +++ b/tap.h
> @@ -91,7 +91,7 @@ void tap_udp4_send(const struct ctx *c, struct in_addr src, in_port_t sport,
>  		   struct in_addr dst, in_port_t dport,
>  		   const void *in, size_t dlen);
>  void tap_icmp4_send(const struct ctx *c, struct in_addr src, struct in_addr dst,
> -		    const void *in, size_t l4len);
> +		    const void *in, const void *src_mac, size_t l4len);
>  const struct in6_addr *tap_ip6_daddr(const struct ctx *c,
>  				     const struct in6_addr *src);
>  void *tap_push_ip6h(struct ipv6hdr *ip6h,
> @@ -103,7 +103,7 @@ void tap_udp6_send(const struct ctx *c,
>  		   uint32_t flow, void *in, size_t dlen);
>  void tap_icmp6_send(const struct ctx *c,
>  		    const struct in6_addr *src, const struct in6_addr *dst,
> -		    const void *in, size_t l4len);
> +		    const void *in, const void *src_mac, size_t l4len);
>  void tap_send_single(const struct ctx *c, const void *data, size_t l2len);
>  size_t tap_send_frames(const struct ctx *c, const struct iovec *iov,
>  		       size_t bufs_per_frame, size_t nframes);
> diff --git a/udp.c b/udp.c
> index 30937dd..8d43646 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -385,6 +385,7 @@ static void udp_tap_prepare(const struct mmsghdr *mmh,
>   * udp_send_tap_icmp4() - Construct and send ICMPv4 to local peer
>   * @c:		Execution context
>   * @ee:	Extended error descriptor
> + * @uflow:	UDP flow
>   * @toside:	Destination side of flow
>   * @saddr:	Address of ICMP generating node
>   * @in:	First bytes (max 8) of original UDP message body
> @@ -392,6 +393,7 @@ static void udp_tap_prepare(const struct mmsghdr *mmh,
>   */
>  static void udp_send_tap_icmp4(const struct ctx *c,
>  			       const struct sock_extended_err *ee,
> +			       const struct udp_flow *uflow,
>  			       const struct flowside *toside,
>  			       struct in_addr saddr,
>  			       const void *in, size_t dlen)
> @@ -421,7 +423,7 @@ static void udp_send_tap_icmp4(const struct ctx *c,
>  	tap_push_uh4(&msg.uh, eaddr, eport, oaddr, oport, in, dlen);
>  	memcpy(&msg.data, in, dlen);
>  
> -	tap_icmp4_send(c, saddr, eaddr, &msg, msglen);
> +	tap_icmp4_send(c, saddr, eaddr, &msg, uflow->f.tap_omac, msglen);

This is only correct when saddr == toside->oaddr; i.e. when the ICMP
is coming from the flow peer.  An ICMP can come from an intermediate
node, which we'd need a separate MAC lookup for.

>  }
>  
>  
> @@ -429,6 +431,7 @@ static void udp_send_tap_icmp4(const struct ctx *c,
>   * udp_send_tap_icmp6() - Construct and send ICMPv6 to local peer
>   * @c:		Execution context
>   * @ee:	Extended error descriptor
> + * @uflow:	UDP flow
>   * @toside:	Destination side of flow
>   * @saddr:	Address of ICMP generating node
>   * @in:	First bytes (max 1232) of original UDP message body
> @@ -437,6 +440,7 @@ static void udp_send_tap_icmp4(const struct ctx *c,
>   */
>  static void udp_send_tap_icmp6(const struct ctx *c,
>  			       const struct sock_extended_err *ee,
> +			       const struct udp_flow *uflow,
>  			       const struct flowside *toside,
>  			       const struct in6_addr *saddr,
>  			       void *in, size_t dlen, uint32_t flow)
> @@ -466,7 +470,7 @@ static void udp_send_tap_icmp6(const struct ctx *c,
>  	tap_push_uh6(&msg.uh, eaddr, eport, oaddr, oport, in, dlen);
>  	memcpy(&msg.data, in, dlen);
>  
> -	tap_icmp6_send(c, saddr, eaddr, &msg, msglen);
> +	tap_icmp6_send(c, saddr, eaddr, &msg, uflow->f.tap_omac, msglen);

Ditto.

>  }
>  
>  /**
> @@ -626,12 +630,12 @@ static int udp_sock_recverr(const struct ctx *c, int s, flow_sidx_t sidx,
>  	if (hdr->cmsg_level == IPPROTO_IP &&
>  	    (o4 = inany_v4(&otap)) && inany_v4(&toside->eaddr)) {
>  		dlen = MIN(dlen, ICMP4_MAX_DLEN);
> -		udp_send_tap_icmp4(c, ee, toside, *o4, data, dlen);
> +		udp_send_tap_icmp4(c, ee, uflow, toside, *o4, data, dlen);
>  		return 1;
>  	}
>  
>  	if (hdr->cmsg_level == IPPROTO_IPV6 && !inany_v4(&toside->eaddr)) {
> -		udp_send_tap_icmp6(c, ee, toside, &otap.a6, data, dlen,
> +		udp_send_tap_icmp6(c, ee, uflow, toside, &otap.a6, data, dlen,
>  				   FLOW_IDX(uflow));
>  		return 1;
>  	}

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