Re: [PATCH v14 03/10] fwd: Add cache table for ARP/NDP contents

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

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

On Fri, Oct 17, 2025 at 02:49:33PM -0400, Jon Maloy wrote:
> 
> 
> On 2025-10-16 23:05, David Gibson wrote:
> > On Tue, Oct 14, 2025 at 10:55:14PM -0400, Jon Maloy wrote:
> > > We add a cache table to keep track of the contents of the kernel ARP
> > > and NDP tables. The table is fed from the just introduced netlink based
> > > neigbour subscription function.
> > > 
> > > Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > I do see one error here, but it's fairly harmless, so I think a follow
> > up makes more sense than a respin.
> > 
> > [snip]
> > > +	/* Blocker entries to stop events from hosts using these addresses */
> > > +	if (!inany_is_unspecified4(&mhl))
> > > +		fwd_neigh_table_update(c, &mhl, c->our_tap_mac, true);
> > > +
> > > +	if (!inany_is_unspecified4(&ggw) && !c->no_map_gw)
> > > +		fwd_neigh_table_update(c, &ggw, c->our_tap_mac, true);
> > > +
> > > +	if (!inany_is_unspecified4(&mga) && !inany_equals(&mhl, &mga)) {
> > 
> > That made me realise that we should throw an error during
> > configuration if map_host_loopback == map_guest_addr.  It doesn't make
> > sense for these to be the same - if they are, we have no way of
> > knowing if a packet should be mapped to 127.0.0.1 or to guest_addr.
> > *checks* looks like map_host_loopback will take precedence in this
> > case, because of the way nat_outbound() is ordered.
> 
> I also noticed it is possible to set guest_gw to some random address while
> at the same time setting no_map_gw. It seems to be harmless, since no_map_gw
> takes precedence, but it is an inconsistency we should fence
> against.

No, the existing behaviour is correct.  It gets confusing, because the
guest_gw is used for magic NATs, that's often what we're referring to
when we discuss the guest_gw address.

But the guest_gw is *also* exactly what it says on the tin: the
gateway address the guest uses.  Without that, the guest won't have
connectivity at all, so we need it.  --no-map-gw means we don't have
the magic NATs, but there's still a gateway, and -g still does and
should control its address.

> > In any case I think you can drop the inany_equals() test - the
> > permanent bit will stop the second update from clobbering the first,
> > even if we are misconfigured.
> > 
> > > +		uint8_t mac[ETH_ALEN];
> > > +		int rc;
> > > +
> > > +		rc = nl_link_get_mac(nl_sock, c->ifi4, mac);
> > > +		if (rc < 0) {
> > > +			debug("Couldn't get ip4 MAC addr: %s", strerror_(-rc));
> > > +			memcpy(mac, c->our_tap_mac, ETH_ALEN);
> > > +		}
> > 
> > Using the host's MAC for --map-guest-addr only makes sense if the
> > guest address is the same as the host address.  If -a is used to make
> > the guest address different, then it may shadow some other random
> > node, not the host.  We don't need special handling for that case -
> > the nat_inbound() you already have will do what we need.
> > 
> > IIUC, the host itself doesn't appear in the neighbour table, so we do
> > need special handling if we want to use the host MAC when
> > --map-guest-addr *does* refer to the host.  To handle that, I think
> > what we want is pseudo-codishly:
> > 
> >   fwd_neigh_table_update(c, nat_inbound(host_addr), host_mac, true);
> > 
> > The wrinkle is that while we do get the host address at some point,
> > I'm not sure we keep it around (it's typically irrelevant after init).
> > 
> > Strictly speaking 'permanent' isn't really correct here, but it's not
> > worth the hassle of setting up a whole other netlink monitor to watch
> > for changes in the host's MAC address.
> > 
> > In fact.. I'm not sure it's worth handling this case at all.  I think
> > it would be ok to just drop this clause.  That means we'll use
> > our_tap_mac by default for things NATted to the (non loopback) host,
> > which is probably fine.
> > 
> Fine with me, but I do think we need a blocker entry just in case somebody
> else comes up with the same address.

We don't need a blocker.  If someone else (let's call them X) comes up
with that address, that implies it's not the host's address.  If
that's so then --map-guest-gw will NAT to X, and that's intended.  In
which case it also makes sense to use X's MAC address.  We don't need
to do anything special to make that happen - X will appear in the host
neigh table, and the nat_inbound() call will put it into the slot of
the --map-guest-gw address.

> I'll post a complementary commit once the series has been applied.
> 
> ///jon
> 
> 

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