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: 6830 bytes --]

On Tue, Aug 05, 2025 at 11:39:35PM +0200, Stefano Brivio wrote:
> On Tue, 5 Aug 2025 16:00:53 -0400
> Jon Maloy <jmaloy@redhat.com> wrote:
> 
> > On 2025-07-21 21:55, David Gibson wrote:
> > > 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.  
> > 
> > I notice this recurring concern from you, but I see no suggestion for
> > how to handle it. Could I try to trigger a host ARP call, e.g., by 
> > sending a preceding ping when certain conditions are fulfilled, or are
> > you saying the series is meaningless?
> 
> We discussed this concern with David in a call a while ago, and I guess
> I owe you a summary (sorry for not following up). Some tentative
> (somewhat debatable but probably reasonable) outcomes were:
> 
> 1. the most important use case for the feature you're implementing is
>    for inbound connections (remember the PiHole thing...?), and in that
>    case, the MAC address of the peer just appeared in the ARP table,
>    supposedly, so that should already work

Right.

> 2. for outbound connections, that's not the case, and the MAC address
>    might actually be missing. In that case, we pick our_tap_mac when
>    the flow is established, and perhaps we should just keep using that
>    for the whole duration of the flow?
> 
> 3. the guest seeing a MAC address change for a given IP address
>    shouldn't be a problem (normative references don't say anything
>    about it, as far as I can tell, and it's something that happens in
>    common cases), but it still feels saner to stick to a given MAC
>    address for a given flow (even if at some point we have more updated
>    information about a peer)

Well.. maybe.  The nasty case is if you have two concurrent flows to
the same peer.  The first one gets assigned our_tap_mac (because passt
doesn't know any better), the second one gets the real MAC, because
the first flow's traffic forced the host to ARP.  This is now likely
to cause the guest's idea of the peer's MAC to ping pong between the
two addresses as it receive traffic from the two flows.

I think that will probably still work for the cases we have, but it is
pretty weird.

> 4. adding probes sounds worse than the original problem because:
> 
>    a. we introduce spurious, potentially confusing, spurious traffic,
>       and that's something we always avoided, in the name of
>       "transparency"
> 
>    b. ICMP won't necessarily work
> 
>    c. even if it does, you don't know how long it takes, and delaying
>       flows for whatever amount of time might be problematic
> 
>    d. again, this mostly (almost entirely?) matters for inbound
>       connections only. We don't have to implement crazy stuff for
>       outbound connections where it practically shouldn't matter

Right.  When I made the initial comment I was thinking of triggering
a ping (or something) on the host to force the arp, but based on those
reasons I no longer think that's a good idea.

> ...long story short, sticking to our_tap_mac for the whole duration of
> the flow _if we don't have a MAC address_ (when the flow is established)
> sounded like the most reasonable approach. And, if we have a MAC
> address, what your series does should be the right thing anyway. What
> do you think?

Actually, I think I had one idea that might be better.  Can we defer
assigning the flow's MAC until we get the first inbound traffic.  So
for flows initiated from outside, we set it immediately just like in
the draft.  For flows initiated from the guest, don't set the MAC
until we get the first reply packet from outside.  I don't think we
should need that MAC before that point, because it's only when we have
traffic to send back to the guest that we actually need it.

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