Re: [PATCH v5 10/10] icmp: let icmp use mac address from flowside structure

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

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

On Fri, Sep 05, 2025 at 10:11:54PM -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>
> 
> ---
> v3: - Adapted to the move of external MAC address from struct flowside
>       to struct flow_common
> v4: - Adapted to name changes in previous commits in this series
> v5: - Added conditional lookup in ARP/NDP if the flow's tap_omac is
>       undefined
> ---
>  icmp.c |  8 ++++++--
>  ndp.c  |  2 +-
>  tap.c  | 10 ++++++----
>  tap.h  |  4 ++--
>  udp.c  | 12 ++++++++----
>  5 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/icmp.c b/icmp.c
> index 6dffafb..2d5bf0c 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -125,17 +125,21 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
>  	flow_dbg(pingf, "echo reply to tap, ID: %"PRIu16", seq: %"PRIu16,
>  		 ini->eport, seq);
>  
> +	/* Try to find true MAC address in ARP/NDP table if needed */
> +	if (mac_undefined(pingf->f.tap_omac))
> +		fwd_neigh_mac_get(c, &ini->oaddr, pingf->f.tap_omac);
> +
>  	if (pingf->f.type == FLOW_PING4) {
>  		const struct in_addr *saddr = inany_v4(&ini->oaddr);
>  		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 820c556..e0e8938 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -184,7 +184,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 05429aa..6443812 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, c->our_tap_mac, 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, c->our_tap_mac, 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 02f7761..1864173 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 7d98845..26f8c22 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);

As I noted on an earlier version, this is only correct if the ICMP is
coming from the peer, not an intermediate host.  You'd need to look up
the MAC based on saddr, which could be different from the peer
address.

Or... I suspect it's probably good enough to just use our_tap_mac in
the case that (saddr != peer address).

>  }
>  
>  
> @@ -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);

Same thing here.

>  }
>  
>  /**
> @@ -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;
>  	}
> -- 
> 2.50.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 --]