Re: [PATCH v3 2/8] 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: 7246 bytes --]

On Sun, Jun 29, 2025 at 01:13:41PM -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>
> 
> ---
> 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.
> ---
>  arp.c   |  9 +++++++++
>  fwd.c   |  2 +-
>  fwd.h   |  3 ++-
>  inany.c | 15 +++++++++++++++
>  inany.h |  1 +
>  ndp.c   |  9 +++++++++
>  6 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/arp.c b/arp.c
> index fc482bb..1952a63 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
> @@ -40,6 +41,7 @@
>  int arp(const struct ctx *c, const struct pool *p)
>  {
>  	unsigned char swap[4];
> +	union inany_addr tgt;
>  	struct ethhdr *eh;
>  	struct arphdr *ah;
>  	struct arpmsg *am;
> @@ -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 (!inany_nat(c, &tgt))
> +		nl_mac_get(nl_sock, &tgt, c->ifi4, am->sha);
> +

Hmm.  Here's one concern about the overall concept here.  If neither
the guest nor the host has contacted this neighbour before, it
probably won't be in the host's arp/neighbour table so this lookup
will fail, and we'll use our_tap_mac.  The guest then contacts it, so
the host ARPs it.  When the guest's ARP times out, it re-ARPs and this
time gets the actual MAC address.  i.e. it seems to me this approach
may substantially increase the odds of the guest seeing a peer change
MAC.  Not sure if that's a problem.



>  	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..02ebc9d 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -332,7 +332,7 @@ 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,
> +void nat_outbound(const struct ctx *c, const union inany_addr *addr,
>  			 union inany_addr *translated)
>  {
>  	if (inany_equals4(addr, &c->ip4.map_host_loopback))
> diff --git a/fwd.h b/fwd.h
> index 0458a3c..61632f2 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);
> -
> +void nat_outbound(const struct ctx *c, const union inany_addr *addr,
> +		  union inany_addr *translated);
>  #endif /* FWD_H */
> diff --git a/inany.c b/inany.c
> index f5483bf..0ecf10a 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);
> @@ -56,3 +57,17 @@ int inany_pton(const char *src, union inany_addr *dst)
>  
>  	return 0;
>  }
> +
> +/** 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 inany_nat(const struct ctx *c, const union inany_addr *addr)

This certainly doesn't belong in inany.h, it should be in fwd.[ch].
inany.h is about the mechanics of handling inany addresses.  This
function is about NAT, a much higher level concept.

... and indeed it might be better just to extend nat_outbound() with
this functionality rather than writing a new wrapper.

> +{
> +	union inany_addr addr_nat;
> +
> +	nat_outbound(c, addr, &addr_nat);
> +	return !inany_equals(addr, &addr_nat);

Hrm... I'm having trouble convincing myself is this is the correct
logic.  Having an address NATted doesn't necessarily preclude sensibly
maintaining it's MAC, as long as the translation is consistent and 1
to 1.  Though you could certainly argue that if you're translating the
L3 address, maintaining the L2 address is kind of meaningless.

In the other direction, I'm not sure if we should be preserving MACs
for things that are "logically" NATted.  That is, they do hit one of
the translation cases in nat_outbound(), but it happens that the
translated address is the same as the original address.

> +}
> diff --git a/inany.h b/inany.h
> index 7ca5cbd..b2a66b0 100644
> --- a/inany.h
> +++ b/inany.h
> @@ -278,5 +278,6 @@ static inline void inany_siphash_feed(struct siphash_state *state,
>  
>  const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size);
>  int inany_pton(const char *src, union inany_addr *dst);
> +bool inany_nat(const struct ctx *c, const union inany_addr *addr);
>  
>  #endif /* INANY_H */
> diff --git a/ndp.c b/ndp.c
> index 3e15494..2e7f0bc 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 (!inany_nat(c, &tgt))
> +		nl_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 --]