Re: [PATCH v11 2/9] fwd: Add cache table for ARP/NDP contents

On Tue, Sep 30, 2025 at 01:58:42AM +0200, Stefano Brivio wrote:

Almost entirely nitpicks here:

On Sat, 27 Sep 2025 15:25:15 -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. The new table eliminates the need for explicit netlink calls to find a host's MAC address.

Signed-off-by: Jon Maloy

[snip]

...

+ const union inany_addr *key) +{ + struct siphash_state st = SIPHASH_INIT(c->hash_secret); + uint32_t i; + + inany_siphash_feed(&st, key); + i = siphash_final(&st, sizeof(*key), 0); + + return ((size_t)i) & (NEIGH_TABLE_SIZE - 1); +} + +/** + * fwd_neigh_table_find() - Find a MAC table entry

Strictly speaking, it's a MAC address table -- MAC refers to the access control itself.

I mean, it's both. The entries are our neighbours and the contents of each entry is the MAC address. [snip]

...

+void fwd_neigh_table_free(const struct ctx *c, const union inany_addr *addr) +{ + ssize_t slot = neigh_table_slot(c, addr); + struct neigh_table *t = &neigh_table; + struct neigh_table_entry *e, **prev; + + prev = &t->slots[slot]; + e = t->slots[slot]; + while (e && !inany_equals(&e->addr, addr)) { + prev = &e->next; + e = e->next; + } + if (!e) + return; + + *prev = e->next; + e->next = t->free; + t->free = e; + memset(&e->addr, 0, sizeof(*addr)); + memset(e->mac, 0, ETH_ALEN);

Do we care about zeroing them? If we do because we might find those entries, note that both all-zero MAC address and IP addresses are valid.

In the case of IP addresses, while all-zero is valid in certain contexts, I'd argue it's never a valid address for a neighbour ("this host on this network" is frustratingly vague, but pretty clearly not a neighbour). 255.255.255.255 would also make a reasonable placeholder if we must have one. For MAC addresses, ff:ff:ff:ff:ff:ff would probably make a better placeholder, if we must have one. -- 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