Re: [PATCH v12 4/9] arp/ndp: respond with true MAC address of LAN local remote hosts

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

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

On Thu, Oct 02, 2025 at 08:34:07PM -0400, Jon Maloy wrote:
> When we receive an ARP request or NDP neigbour solicitation over
> the tap interface for a host on the local network segment attached
> to the template interface, we respond with that host's real MAC
> address, if available.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> ---
> v3: - Added helper function to find out if a remote ip address is subject
>       to NAT. This filters out local host addresses which should be
>       presented with the passt/pasta local MAC address 9a:55:9a:55:9a:55 even
>       though it is on the local segment.
>     - Adapted to the change in nl_mac_get() function, so that we now consider
>       only the template interface when checking the ARP/NDP table.
> v4: - Moved NAT check into the function nat_outbound() to obtain more
>       precise criteria for when NAT is used. We may in theory
>       have NAT even if original and translated addresses are equal, and
>       we want to catch this case.
>     - I chose to keep the wrapper funtion inany_nat(), but moved it to
>       fwd.h/fwd.c and renamed it to fwd_inany_nat().
> v5: - Simplified criteria for when we do ARP/NDP lookup. Now, we
>       just try with the potentially translated address after an
>       attempted NAT check.
>     - Using the new ARP/NDP cache table instead of using netlink
>       directly.
> v6: - Fixes after feedback from David:
>     - Renamed nat_outbound() to fwd_nat_outbound()
>     - Eliminated unnecessary temporary variable in arp.c::arp()
> v12: - Some minor changes after feedback from Stefano
> ---
>  arp.c   | 9 +++++++--
>  fwd.c   | 8 ++++----
>  fwd.h   | 2 ++
>  inany.c | 1 +
>  ndp.c   | 8 ++++++++
>  5 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/arp.c b/arp.c
> index b08780f..a95c377 100644
> --- a/arp.c
> +++ b/arp.c
> @@ -69,6 +69,7 @@ static bool ignore_arp(const struct ctx *c,
>   */
>  int arp(const struct ctx *c, struct iov_tail *data)
>  {
> +	union inany_addr tgt, tgt_nat;
>  	struct {
>  		struct ethhdr eh;
>  		struct arphdr ah;
> @@ -102,8 +103,12 @@ int arp(const struct ctx *c, struct iov_tail *data)
>  	resp.ah.ar_hln = ah->ar_hln;
>  	resp.ah.ar_pln = ah->ar_pln;
>  
> -	/* ARP message */
> -	memcpy(resp.am.sha,		c->our_tap_mac,	sizeof(resp.am.sha));
> +	/* MAC address to return in ARP message */
> +	inany_from_af(&tgt, AF_INET, am->tip);
> +	fwd_nat_outbound(c, &tgt, &tgt_nat);
> +	fwd_neigh_mac_get(c, &tgt_nat, resp.am.sha);
> +	/* Rest of ARP message */
>  	memcpy(resp.am.sip,		am->tip,	sizeof(resp.am.sip));
>  	memcpy(resp.am.tha,		am->sha,	sizeof(resp.am.tha));
>  	memcpy(resp.am.tip,		am->sip,	sizeof(resp.am.tip));
> diff --git a/fwd.c b/fwd.c
> index ade97c8..a69d84d 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -522,7 +522,7 @@ static bool fwd_guest_accessible(const struct ctx *c,
>  }
>  
>  /**
> - * nat_outbound() - Apply address translation for outbound (TAP to HOST)
> + * fwd_nat_outbound() - Apply address translation for outbound (TAP to HOST)
>   * @c:		Execution context
>   * @addr:	Input address (as seen on TAP interface)
>   * @translated:	Output address (as seen on HOST interface)
> @@ -530,8 +530,8 @@ static bool fwd_guest_accessible(const struct ctx *c,
>   * Only handles translations that depend *only* on the address.  Anything
>   * related to specific ports or flows is handled elsewhere.
>   */
> -static void nat_outbound(const struct ctx *c, const union inany_addr *addr,
> -			 union inany_addr *translated)
> +void fwd_nat_outbound(const struct ctx *c, const union inany_addr *addr,
> +		      union inany_addr *translated)
>  {
>  	if (inany_equals4(addr, &c->ip4.map_host_loopback))
>  		*translated = inany_loopback4;
> @@ -565,7 +565,7 @@ uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
>  		   inany_equals6(&ini->oaddr, &c->ip6.dns_match))
>  		tgt->eaddr.a6 = c->ip6.dns_host;
>  	else
> -		nat_outbound(c, &ini->oaddr, &tgt->eaddr);
> +		fwd_nat_outbound(c, &ini->oaddr, &tgt->eaddr);
>  
>  	tgt->eport = ini->oport;
>  
> diff --git a/fwd.h b/fwd.h
> index 6ca743c..f91fa55 100644
> --- a/fwd.h
> +++ b/fwd.h
> @@ -50,6 +50,8 @@ void fwd_scan_ports_init(struct ctx *c);
>  
>  bool nat_inbound(const struct ctx *c, const union inany_addr *addr,
>  		 union inany_addr *translated);
> +void fwd_nat_outbound(const struct ctx *c, const union inany_addr *addr,
> +		      union inany_addr *translated);
>  uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
>  			 const struct flowside *ini, struct flowside *tgt);
>  uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto,
> diff --git a/inany.c b/inany.c
> index 65a39f9..7680439 100644
> --- a/inany.c
> +++ b/inany.c
> @@ -16,6 +16,7 @@
>  #include "ip.h"
>  #include "siphash.h"
>  #include "inany.h"
> +#include "fwd.h"
>  
>  const union inany_addr inany_loopback4 = INANY_INIT4(IN4ADDR_LOOPBACK_INIT);
>  const union inany_addr inany_any4 = INANY_INIT4(IN4ADDR_ANY_INIT);
> diff --git a/ndp.c b/ndp.c
> index d7f64a3..6bba0a8 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -196,6 +196,7 @@ static void ndp_send(const struct ctx *c, const struct in6_addr *dst,
>  static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
>  	    const struct in6_addr *addr)
>  {
> +	union inany_addr tgt, tgt_nat;
>  	struct ndp_na na = {
>  		.ih = {
>  			.icmp6_type		= NA,
> @@ -215,6 +216,13 @@ static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
>  
>  	memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
>  
> +	/* Respond with recorded MAC address if remote host's address
> +	 * or NAT translated address can be found in NDP table.
> +	 */
> +	inany_from_af(&tgt, AF_INET6, addr);
> +	fwd_nat_outbound(c, &tgt, &tgt_nat);
> +	fwd_neigh_mac_get(c, &tgt_nat, na.target_l2_addr.mac);

You do indeed need nat_outbound() here, and as noted on the previous
patch you need nat_inbound() for unsolicited NAs triggered by host
neighbour updates.  But you want to avoid doing both an inbound and
outbound NAT for the same operation, which might require some
restructuring here.

Or, I guess, you could convert the whole neighbour table to be in
terms of guest side addresses, in which case you'd remove the NAT
here, and instead need a nat_inbound() before you insert things into
the table.

>  	ndp_send(c, dst, &na, sizeof(na));
>  }
>  
> -- 
> 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 --]