Re: [PATCH dpdk v5] net/tap: add software MAC address filtering

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 24 Mar 2026 20:09:16 +0100
Robin Jarry <rjarry@redhat.com> 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 <rjarry@redhat.com>
> ---

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++) { ... }
   ```