Re: [PATCH v2 2/8] arp: respond with true mac address of LAN local remote hosts

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 12 Jun 2025 00:21:46 -0400
Jon Maloy <jmaloy@redhat.com> wrote:

> When we receive an ARP or NTP request over the tap interface for a

Nit: NDP.

> host on the local network, 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 |  2 +-
>  fwd.h |  3 ++-
>  ndp.c | 11 +++++++++++
>  4 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/arp.c b/arp.c
> index fc482bb..fbd8219 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,8 @@
>   */
>  int arp(const struct ctx *c, const struct pool *p)
>  {
> +	union inany_addr translated;
> +	union inany_addr addr;

Those aren't very descriptive, especially 'translated'. What about
'tgt_ipaddr' and 'tgt_ipaddr_nat'? I'm not fond of those names either
but I can't come up with much better right now.

Nit: that could be a single line.

>  	unsigned char swap[4];
>  	struct ethhdr *eh;
>  	struct arphdr *ah;
> @@ -72,6 +75,12 @@ 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));
>  
> +	/* If remote host on local network - respond with its mac address */

This is partially a full sentence ("If remote host [...] respond ..."),
partially a schematic form of a correspondence ("Remote host [...]:
respond with ..."). In general we stick to either one, not a mix.

> +	inany_from_af(&addr, AF_INET, am->tip);
> +	nat_outbound(c, &addr, &translated);
> +	if (!memcmp(&addr, &translated, sizeof(addr)))

You can use inany_equals() here as well, for consistency, even though
it's always an IPv4 address.

By the way, this way of checking if the target IP address is a
non-local host on the local segment is a bit convoluted, and perhaps
would deserve its own helper outside of this function.

I also wonder if it's correct in all cases: what happens if this is a
local (non-loopback) address of a host interface? Then we'll call
nl_mac_get() which should leave am->sha as it is (because the MAC
address is not found), but the comment above doesn't match anymore.

> +		nl_mac_get(nl_sock, &addr, am->sha);
> +
>  	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/ndp.c b/ndp.c
> index 3e15494..d702f37 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,9 @@ 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 translated;
> +	union inany_addr addr_any;
> +
>  	struct ndp_na na = {
>  		.ih = {
>  			.icmp6_type		= NA,
> @@ -215,6 +219,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);
>  
> +	/* If remote host on local network - respond with its mac address */

In IPv6 terms this is typically called link-layer address.

> +	inany_from_af(&addr_any, AF_INET6, addr);
> +	nat_outbound(c, &addr_any, &translated);
> +
> +	if (!memcmp(&addr_any, &translated, sizeof(addr_any)))
> +		nl_mac_get(nl_sock, &addr_any, na.target_l2_addr.mac);
> +

Same as for arp().

>  	ndp_send(c, dst, &na, sizeof(na));
>  }
>  

-- 
Stefano