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

["Robin Jarry" <rjarry@redhat.com>]

Stephen Hemminger, Mar 25, 2026 at 17:40:
> 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.

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.