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. --- **net/tap: add software MAC address filtering** **Errors:** 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. ```c /* Current - leaks old mc_addrs on failure */ pmd->mc_addrs = rte_realloc(pmd->mc_addrs, nb_mc_addr * sizeof(*pmd->mc_addrs), 0); if (pmd->mc_addrs == NULL) { pmd->nb_mc_addrs = 0; return -ENOMEM; } /* Fix - save old pointer */ struct rte_ether_addr *new_addrs; new_addrs = rte_realloc(pmd->mc_addrs, nb_mc_addr * sizeof(*pmd->mc_addrs), 0); if (new_addrs == NULL) return -ENOMEM; pmd->mc_addrs = new_addrs; ``` Note the fix also avoids zeroing `nb_mc_addrs` on failure, which preserves the old list as a reasonable fallback. 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`. **Warnings:** 1. **Inconsistent error handling in test 7.** All other tests in `test_tap_mac_filter` use `TEST_ASSERT(ret == 0, ...)` for the inject call, but test 7 uses a manual `if (ret < 0) return TEST_FAILED;` check. This also skips `rte_eth_allmulticast_disable()` cleanup on failure. Should use `TEST_ASSERT` for consistency. 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`. 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: ```c if (rte_is_broadcast_ether_addr(dst)) return true; if (data->all_multicast) return true; for (i = 0; i < pmd->nb_mc_addrs; i++) { ... } ```
