Stephen Hemminger, Mar 25, 2026 at 17:40: > On Tue, 24 Mar 2026 20:09:16 +0100 > Robin Jarry <[email protected]> wrote: > >> Linux TAP devices deliver all packets to userspace regardless of the >> PROMISC/ALLMULTI flags on the interface. When promiscuous mode is >> disabled, drop received packets whose destination MAC does not match >> any configured unicast or multicast address. >> >> The receive path checks the destination MAC against the device's >> unicast address table (managed by the ethdev layer), the multicast >> address list (stored by the driver since the ethdev layer does not keep >> a copy), and accepts broadcast unconditionally. Promiscuous and >> all-multicast modes bypass the respective checks. >> >> To support multiple unicast addresses via rte_eth_dev_mac_addr_add(), >> allocate mac_addrs with rte_zmalloc (TAP_MAX_MAC_ADDRS=16) instead of >> pointing into dev_private, and advertise the new limit in dev_infos_get. >> >> Add a test to ensure it works as expected. >> >> Signed-off-by: Robin Jarry <[email protected]> >> --- > > Looks good, willing to merge this version. > > AI review found some other small things; but these could be addressed later. > Warnings 2 and 3 look like just nuisance stuff.
Hey Stephen, thanks for reviewing. > 1. **Resource leak in `tap_set_mc_addr_list()` on `rte_realloc()` failure.** > `rte_realloc()` returns NULL on failure but does NOT free the original > allocation. The code assigns the result directly back to `pmd->mc_addrs`, so > when it returns NULL the pointer to the old allocation is lost and leaked. I can send a v6 with this fix. > 2. **fd leak in `tap_inject_packet()` on error after `socket()`.** > If `bind()` or `send()` fails, `TEST_ASSERT` causes an immediate return > without closing `fd`. This is test code so the impact is minor, but the fd > leaks on every failed assertion after the socket is opened. > > Suggest using a local `goto cleanup` pattern or closing `fd` before each > `TEST_ASSERT`. This seems not important for test code. > 2. **`rte_malloc` used for `mc_addrs` in `tap_set_mc_addr_list()`.** The > multicast address list is a control-path data structure not accessed by DMA > and not shared between processes (it's in `pmd_internals`, which is > `dev_private`). Standard `malloc`/`realloc`/`free` would be more appropriate > per DPDK guidelines, and would avoid consuming hugepage memory. If > `rte_malloc` is kept, the leak fix above needs to use `rte_free` for cleanup, > which it already would since the old pointer came from `rte_realloc`. I don't mind, but it seems pedantic. > 3. **Broadcast check could be moved before the `mc_addrs` loop.** In > `tap_mac_filter_match()`, broadcast frames (which are a subset of multicast) > traverse the entire `mc_addrs` loop before hitting the > `rte_is_broadcast_ether_addr()` check at the end. Moving the broadcast check > before the loop would avoid unnecessary iterations for a common packet type: I can change this. Let me know if you want a respin.
