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

[Jon Maloy <jmaloy@redhat.com>]

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.

  >
> 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.
I'll post a complementary commit once the series has been applied.

///jon