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

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

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

On Sun, Jun 29, 2025 at 01:13:47PM -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
> ---
>  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 7e2b342..6a3b636 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.omac, n);

The usual concern about first contact.  But more so here, because ping
flows are *always* outbound initiated.

>  	} 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.omac, n);
>  	}
>  	return;
>  
> diff --git a/ndp.c b/ndp.c
> index 2e7f0bc..09a9e0a 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 ceb9d9f..e62c9a6 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -279,13 +279,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);
>  
> @@ -386,14 +387,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 e640d95..7025eaa 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 5942088..453bb51 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -386,6 +386,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
> @@ -393,6 +394,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)
> @@ -422,7 +424,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.omac, msglen);

I don't think this is correct.  We'd want the MAC corresponding to the
source address of the ICMP, which is not necessarily the same as the
peer of the flow (it could be an intermediate host).  Actually trying
to get that right for intermediate hosts is probably pointless and
complex, so I think it would make sense to just use our_tap_mac if
saddr != eaddr.

>  }
>  
>  
> @@ -430,6 +432,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
> @@ -438,6 +441,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)
> @@ -467,7 +471,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.omac, msglen);

Same here.

>  }
>  
>  /**
> @@ -627,12 +631,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 --]