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

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.

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.

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