Re: [PATCH v9 9/9] arp/ndp: send gratuitous ARP / unsolicitated NA when MAC cache entry added

[Jon Maloy <jmaloy@redhat.com>]

On 2025-09-25 20:47, David Gibson wrote:
> On Thu, Sep 25, 2025 at 08:48:51AM -0400, Jon Maloy wrote:
>>
>>
>> On 2025-09-25 02:38, David Gibson wrote:
>>> On Wed, Sep 24, 2025 at 07:32:43PM -0400, Jon Maloy wrote:
>>>>
>>>>
>>>> On 2025-09-24 18:18, Jon Maloy wrote:
>>>>>
>>>>>
>>>>> On 2025-09-23 23:22, David Gibson wrote:
>>>>>> On Tue, Sep 23, 2025 at 09:13:30PM -0400, Jon Maloy wrote:
>>>>
>>>> [...]
>>>>
>>>>>>> --- a/fwd.c
>>>>>>> +++ b/fwd.c
>>>>>>> @@ -26,6 +26,8 @@
>>>>>>>     #include "passt.h"
>>>>>>>     #include "lineread.h"
>>>>>>>     #include "flow_table.h"
>>>>>>> +#include "arp.h"
>>>>>>> +#include "ndp.h"
>>>>>>>     /* Empheral port range: values from RFC 6335 */
>>>>>>>     static in_port_t fwd_ephemeral_min = (1 << 15) + (1 << 14);
>>>>>>> @@ -129,6 +131,15 @@ void fwd_neigh_mac_cache_alloc(const struct ctx *c,
>>>>>>>         memcpy(&e->addr, addr, sizeof(*addr));
>>>>>>>         memcpy(e->mac, mac, ETH_ALEN);
>>>>>>> +
>>>>>>> +    /* Send gratuitous ARP / unsolicited NA for the new mapping */
>>>>>>
>>>>>> AFAICT this doesn't actually implement what the commit message
>>>>>> describes - it seems to always send an ARP/NA when the neighbour table
>>>>>> is updated.
>>>>
>>>> No. Check the code again.
>>>
>>> Still not seeing it, I'm going to need a more specific pointer to what
>>> I've missed.
>>>
>> Look in fwd_neigh_mac_cache_alloc().
>> If the entry already exists, the function updates the mac address just to be
>> on the safe side, but then returns without sending any ARP/NA.
> 
> Uh... no, no it doesn't.  I applied all the patches on a branch and
> here's what I see in fwd_neigh_mac_cache_alloc():
> 
> 	memcpy(&e->addr, addr, sizeof(*addr));
> 	memcpy(e->mac, mac, ETH_ALEN);
> 
> 	/* Send gratuitous ARP / unsolicited NA for the new mapping */
> 	if (inany_v4(addr)) {
> 		struct in_addr ip4 = *inany_v4(addr);
> 
> 		arp_send_gratuitous(c, ip4, e->mac);
> 	} else {
> 		ndp_send_unsolicited_na(c, &addr->a6);
> 	}
> 
> If it updates, it ARPs.  Do you have something in your tree that
> didn't make it into the published patches?
> 

This was introduced already in patch #1, not #9.
The final function looks like this:

void fwd_neigh_mac_cache_alloc(const struct ctx *c,
                                const union inany_addr *addr,
                                uint8_t *mac)

{
         struct mac_cache_table *t = &mac_cache;
         struct mac_cache_entry *e;
         ssize_t idx;

         /* MAC address might change sometimes */
         e = fwd_mac_cache_find(c, addr);
         if (e) {
                 memcpy(e->mac, mac, ETH_ALEN);
====>           return;
         }

         e = t->free;
         if (!e)
                 return;
         t->free = e->next;

         idx = fwd_mac_cache_slot_idx(c, addr);
         e->next = t->slots[idx];
         t->slots[idx] = e;

         memcpy(&e->addr, addr, sizeof(*addr));
         memcpy(e->mac, mac, ETH_ALEN);

         /* Send gratuitous ARP / unsolicited NA for the new mapping */
         if (inany_v4(addr)) {
                 struct in_addr ip4 = *inany_v4(addr);

                 arp_send_gratuitous(c, ip4, e->mac);
         } else {
                 ndp_send_unsolicited_na(c, &addr->a6);
         }
}