Re: [PATCH v4 2/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: 6627 bytes --]

On Tue, Aug 19, 2025 at 11:09:58PM -0400, Jon Maloy wrote:
> When we receive an ARP request or NDP neigbor 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.
> 
> The local host, which is acting as a proxy for the default gateway,
> is still exempted from this rule.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>  arp.c   |  9 +++++++++
>  fwd.c   | 38 ++++++++++++++++++++++++++++++--------
>  fwd.h   |  1 +
>  inany.c |  1 +
>  ndp.c   |  9 +++++++++
>  5 files changed, 50 insertions(+), 8 deletions(-)
> 
> diff --git a/arp.c b/arp.c
> index fc482bb..c37867a 100644
> --- a/arp.c
> +++ b/arp.c
> @@ -29,6 +29,7 @@
>  #include "dhcp.h"
>  #include "passt.h"
>  #include "tap.h"
> +#include "netlink.h"
>  
>  /**
>   * arp() - Check if this is a supported ARP message, reply as needed
> @@ -39,6 +40,7 @@
>   */
>  int arp(const struct ctx *c, const struct pool *p)
>  {
> +	union inany_addr tgt;
>  	unsigned char swap[4];
>  	struct ethhdr *eh;
>  	struct arphdr *ah;
> @@ -72,6 +74,13 @@ int arp(const struct ctx *c, const struct pool *p)
>  	memcpy(am->tha,		am->sha,	sizeof(am->tha));
>  	memcpy(am->sha,		c->our_tap_mac,	sizeof(am->sha));
>  
> +	/* Respond with true MAC address if remote host is on
> +	 * the template interface's network segment
> +	 */
> +	inany_from_af(&tgt, AF_INET, am->tip);
> +	if (!fwd_inany_nat(c, &tgt))
> +		nl_neigh_mac_get(nl_sock, &tgt, c->ifi4, am->sha);

You're not checking for errors from nl_neigh_mac_get().

>  	memcpy(swap,		am->tip,	sizeof(am->tip));
>  	memcpy(am->tip,		am->sip,	sizeof(am->tip));
>  	memcpy(am->sip,		swap,		sizeof(am->sip));
> diff --git a/fwd.c b/fwd.c
> index 250cf56..55bf5f2 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -331,20 +331,29 @@ 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.
> + *
> + * Return: true if there was a translation, otherwise false
>   */
> -static void nat_outbound(const struct ctx *c, const union inany_addr *addr,
> -			 union inany_addr *translated)
> +static bool nat_outbound(const struct ctx *c, const union inany_addr *addr,
> +				union inany_addr *translated)
>  {

I'm having trouble convincing myself that explicitly excluding MAC
preservation in NAT cases is strictly correct, as opposed to doing a
MAC lookup on the translated address.  I'm reasonably confident it
will be good enough for most purposes.

> -	if (inany_equals4(addr, &c->ip4.map_host_loopback))
> +	if (inany_equals4(addr, &c->ip4.map_host_loopback)) {

For this case, we'll certainly want our_tap_mac.  We should get that
indirectly anyway, though, since 127.0.0.1 will never be in the host
interface's neighbour table.

>  		*translated = inany_loopback4;
> -	else if (inany_equals6(addr, &c->ip6.map_host_loopback))
> +		return true;
> +	} else if (inany_equals6(addr, &c->ip6.map_host_loopback)) {
>  		*translated = inany_loopback6;

Ditto, but for ::.

> -	else if (inany_equals4(addr, &c->ip4.map_guest_addr))
> +		return true;
> +	} else if (inany_equals4(addr, &c->ip4.map_guest_addr)) {
>  		*translated = inany_from_v4(c->ip4.addr);
> -	else if (inany_equals6(addr, &c->ip6.map_guest_addr))
> +		return true;
> +	} else if (inany_equals6(addr, &c->ip6.map_guest_addr)) {
>  		translated->a6 = c->ip6.addr;

Arguably for these cases, using the MAC of the translated address
is what we want.  There is a real peer on the host side (usually the
host itself, but maybe something else), which might have a presence on
the template interface.  We have the NAT in place because it conflicts
with the guest's IP, but the MAC should still be unique.

> -	else
> -		*translated = *addr;
> +		return true;
> +	}
> +
> +	*translated = *addr;
> +	return false;
> +
>  }
>  
>  /**
> @@ -554,3 +563,16 @@ uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto,
>  
>  	return PIF_TAP;
>  }
> +
> +/** fwd_inany_nat - Find if a remote IPv[46] address is subject to NAT
> + * @c:          Execution context
> + * @addr:       IPv[46] address
> + *
> + * Return: true if translated, false otherwise
> + */
> +bool fwd_inany_nat(const struct ctx *c, const union inany_addr *addr)
> +{
> +	union inany_addr addr_nat;
> +
> +	return nat_outbound(c, addr, &addr_nat);
> +}
> diff --git a/fwd.h b/fwd.h
> index 65c7c96..c8d485d 100644
> --- a/fwd.h
> +++ b/fwd.h
> @@ -56,5 +56,6 @@ uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto,
>  			    const struct flowside *ini, struct flowside *tgt);
>  uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto,
>  			  const struct flowside *ini, struct flowside *tgt);
> +bool fwd_inany_nat(const struct ctx *c, const union inany_addr *addr);
>  
>  #endif /* FWD_H */
> 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 3e15494..9912f80 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -32,6 +32,7 @@
>  #include "passt.h"
>  #include "tap.h"
>  #include "log.h"
> +#include "netlink.h"
>  
>  #define	RT_LIFETIME	65535
>  
> @@ -196,6 +197,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;
>  	struct ndp_na na = {
>  		.ih = {
>  			.icmp6_type		= NA,
> @@ -215,6 +217,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 true link-layer address if remote host is on
> +	 * the template interface's network segment
> +	 */
> +	inany_from_af(&tgt, AF_INET6, addr);
> +	if (!fwd_inany_nat(c, &tgt))
> +		nl_neigh_mac_get(nl_sock, &tgt, c->ifi6, na.target_l2_addr.mac);
> +
>  	ndp_send(c, dst, &na, sizeof(na));
>  }
>  

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