port: prevent disabling forced promiscuous and allmulti modes #381
Conversation
📝 WalkthroughWalkthroughAdds two interface state flags (PROMISC_FIXED, ALLMULTI_FIXED) and updates CLI formatting to indicate "(fixed)". Port control helpers (port_promisc_set, port_allmulti_set, port_mac_add, port_mac_del) were made public and declared in a new port_priv.h. MAC add/remove now handle purge/reinstall of address lists and update fixed-state flags. Unit tests and build/coverage rule adjustments were added. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rjarry
force-pushed
the
port-mac-filter
branch
from
cba45a0 to
1d7b385
Compare
|
What about ospf in FRR ? These ones use a socket to listen multicast groupe. The cp port will not be enough in this case. |
rjarry
force-pushed
the
port-mac-filter
branch
from
1d7b385 to
61eed17
Compare
I don't understand what you mean. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
GNUmakefile(4 hunks)meson.build(1 hunks)modules/infra/api/gr_infra.h(1 hunks)modules/infra/cli/iface.c(1 hunks)modules/infra/control/meson.build(1 hunks)modules/infra/control/port.c(6 hunks)modules/infra/control/port_priv.h(1 hunks)modules/infra/control/port_test.c(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/port_test.cmodules/infra/api/gr_infra.hmodules/infra/control/port_priv.hmodules/infra/cli/iface.cmodules/infra/control/port.c
🧠 Learnings (2)
📚 Learning: 2025-11-05T13:55:26.169Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.169Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/control/port_test.cmodules/infra/control/port_priv.h
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
meson.build
🧬 Code graph analysis (3)
modules/infra/control/port_test.c (3)
modules/infra/control/worker.c (4)
port_unplug(145-182)port_plug(184-223)worker_count(125-133)worker_queue_distribute(302-424)modules/infra/control/gr_iface.h (1)
iface(16-22)modules/infra/control/port.c (4)
port_mac_add(506-602)port_promisc_set(198-225)port_mac_del(604-709)port_allmulti_set(227-254)
modules/infra/control/port_priv.h (1)
modules/infra/control/port.c (4)
port_mac_add(506-602)port_mac_del(604-709)port_promisc_set(198-225)port_allmulti_set(227-254)
modules/infra/control/port.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: rpm
- GitHub Check: commits
- GitHub Check: deb
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: lint
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
rjarry
force-pushed
the
port-mac-filter
branch
from
61eed17 to
cfb58cc
Compare
|
approving it ; as discussed, Instead of complex tracking, as IPv6 relies heavily on multicast, we should think about removing this code to enable |
Enable code coverage using the standard b_coverage meson option. Remove manual -coverage and -lgcov args. Fix gcovr options. Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Christophe Fontaine <[email protected]>
When the hardware MAC address filter reaches its capacity, the port switches to allmulti or promisc mode to receive all traffic. However, some addresses may still remain programmed in the hardware filter, creating an inconsistent state where only a subset of the software MAC list is actually filtered by hardware. When adding a new address triggers the MAC_FILTER_F_ALL flag (indicating the hardware limit was reached), purge all explicit addresses from the hardware filter. This ensures we don't lose track of which addresses are handled by hardware filtering versus allmulti/promisc mode. All addresses continue to be tracked in software. When removing addresses brings the count back under the hardware limit, reinstall all remaining addresses to the hardware filter after disabling allmulti/promisc mode. This restores proper hardware filtering. Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Christophe Fontaine <[email protected]>
When a port MAC filter table reaches its hardware limit or if the driver does not support individual address filtering, the code falls back to enabling promisc or allmulti mode to ensure all required traffic is still received. However, other code paths that manage these modes were unaware of this fallback mechanism and could inadvertently disable them, breaking MAC address filtering. Introduce two new interface state flags to track when promisc or allmulti modes have been force-enabled due to MAC filter overflow/unsupported. The port_promisc_set() and port_allmulti_set() functions now check these forced states and refuse to modify the hardware state when the mode is required for filtering. Update the CLI to distinguish between user-requested and filter-forced modes by displaying "promisc(fixed)" or "allmulti(fixed)" when applicable. Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Christophe Fontaine <[email protected]>
The MAC address filtering logic handles several edge cases that are difficult to test with real hardware: hardware filter capacity limits, transitions between normal and promisc/allmulti modes, and reference counting for duplicate addresses. Add a comprehensive test suite that mocks DPDK functions to verify the correct behavior when adding and removing both unicast and multicast addresses. The tests ensure that when hardware limits are reached, the port correctly switches to promisc/allmulti mode and tracks the limit, and that when addresses are removed bringing the count back under the limit, it properly restores hardware filtering. Export previously static functions via a private header to allow the test code to exercise them directly without going through the full interface API. Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Christophe Fontaine <[email protected]>
grout-bot
force-pushed
the
port-mac-filter
branch
from
cfb58cc to
b2556ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
modules/infra/control/port.c (1)
576-581: Set iface->flags when forcing promisc/allmulti.When
rte_eth_{promiscuous,allmulticast}_enable()succeeds, the code sets the state bits (GR_IFACE_S_ALLMULTI_FIXEDorGR_IFACE_S_PROMISC_FIXED) but doesn't set the correspondingiface->flags(GR_IFACE_F_ALLMULTIorGR_IFACE_F_PROMISC). This means the interface flags won't reflect the actual hardware mode, breaking tests and consumers that rely on those flags.Apply the diff from the previous review:
- if (ret == 0) { - filter->flags |= MAC_FILTER_F_ALL; - iface->state |= multicast ? - GR_IFACE_S_ALLMULTI_FIXED : - GR_IFACE_S_PROMISC_FIXED; - } + if (ret == 0) { + filter->flags |= MAC_FILTER_F_ALL; + if (multicast) { + iface->state |= GR_IFACE_S_ALLMULTI_FIXED; + iface->flags |= GR_IFACE_F_ALLMULTI; + } else { + iface->state |= GR_IFACE_S_PROMISC_FIXED; + iface->flags |= GR_IFACE_F_PROMISC; + } + }
🧹 Nitpick comments (1)
modules/infra/control/port.c (1)
529-537: Refcount increment timing in log message.Line 536 uses post-increment
filter->refcnt[i]++, so the LOG statement displays the value before incrementing. Consider using pre-increment++filter->refcnt[i]to show the updated refcount, or move the increment after the LOG.if (rte_is_same_ether_addr(&filter->mac[i], mac)) { LOG(DEBUG, "%s: %s mac " ETH_F " already filtered (refs=%u)", iface->name, mac_type, mac, - filter->refcnt[i]++); + ++filter->refcnt[i]); return 0; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
GNUmakefile(4 hunks)meson.build(1 hunks)modules/infra/api/gr_infra.h(1 hunks)modules/infra/cli/iface.c(1 hunks)modules/infra/control/meson.build(1 hunks)modules/infra/control/port.c(6 hunks)modules/infra/control/port_priv.h(1 hunks)modules/infra/control/port_test.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- modules/infra/cli/iface.c
- modules/infra/api/gr_infra.h
- modules/infra/control/port_test.c
- GNUmakefile
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/port_priv.hmodules/infra/control/port.c
🧠 Learnings (2)
📚 Learning: 2025-09-05T07:06:51.554Z
Learnt from: rjarry
Repo: DPDK/grout PR: 294
File: modules/policy/cli/meson.build:4-8
Timestamp: 2025-09-05T07:06:51.554Z
Learning: The grout project uses a modular CLI build system where individual modules contribute their CLI sources to a global cli_src variable via `cli_src += files(...)`, and the top-level meson.build file builds the final grcli executable using all collected CLI sources. The grcli executable is defined in the top-level meson.build at lines 122-127.
Applied to files:
meson.build
📚 Learning: 2025-11-05T13:55:26.169Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.169Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
modules/infra/control/port_priv.h
🧬 Code graph analysis (2)
modules/infra/control/port_priv.h (1)
modules/infra/control/port.c (4)
port_mac_add(506-602)port_mac_del(604-709)port_promisc_set(198-225)port_allmulti_set(227-254)
modules/infra/control/port.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-22)
🔇 Additional comments (8)
meson.build (1)
140-141: LGTM - test build configuration simplified.The removal of conditional coverage instrumentation aligns with the broader test/coverage adjustments across the PR.
modules/infra/control/meson.build (1)
44-58: LGTM - test suite properly configured.The new port_test.c entry correctly wraps all necessary DPDK Ethernet functions for testing MAC filtering flows.
modules/infra/control/port_priv.h (1)
1-13: LGTM - clean header design.The header properly exposes the four port control functions with appropriate includes and header guards.
modules/infra/control/port.c (5)
4-4: LGTM - header include.
198-225: LGTM - forced promisc protection.The early return when
GR_IFACE_S_PROMISC_FIXEDis set correctly prevents disabling promiscuous mode when it's required for MAC filtering.
227-254: LGTM - forced allmulti protection.The early return when
GR_IFACE_S_ALLMULTI_FIXEDis set correctly prevents disabling all-multicast mode when it's required for MAC filtering.
589-599: LGTM - purge explicit addresses.After enabling forced promisc/allmulti, purging explicit addresses from the device is correct—they'll be reinstalled when the forced mode is later disabled.
694-706: LGTM - normal address removal path.The non-forced path correctly removes individual MAC addresses from hardware.
| else | ||
| iface->state &= multicast ? | ||
| ~GR_IFACE_S_ALLMULTI_FIXED : | ||
| ~GR_IFACE_S_PROMISC_FIXED; | ||
|
|
||
| // Reinstall all addresses after disabling allmulti/promisc. | ||
| if (multicast) { | ||
| ret = rte_eth_dev_set_mc_addr_list( | ||
| port->port_id, filter->mac, filter->count | ||
| ); | ||
| } else { | ||
| for (i = 0; i < filter->count; i++) { | ||
| int r = rte_eth_dev_mac_addr_add(port->port_id, &filter->mac[i], 0); | ||
| if (r < 0 && ret == 0) | ||
| ret = r; | ||
| } | ||
| } | ||
| if (ret < 0) | ||
| LOG(WARNING, "%s: %s: %s", iface->name, mac_type, rte_strerror(-ret)); |
There was a problem hiding this comment.
Update iface->flags when clearing forced mode.
After successfully disabling allmulti/promisc (lines 674-677), the code clears the state flags but doesn't update iface->flags to clear GR_IFACE_F_ALLMULTI/GR_IFACE_F_PROMISC. Since the hardware mode is being disabled, the flags should reflect that.
Apply this diff to clear the flags:
else
- iface->state &= multicast ?
- ~GR_IFACE_S_ALLMULTI_FIXED :
- ~GR_IFACE_S_PROMISC_FIXED;
+ if (multicast) {
+ iface->state &= ~GR_IFACE_S_ALLMULTI_FIXED;
+ iface->flags &= ~GR_IFACE_F_ALLMULTI;
+ } else {
+ iface->state &= ~GR_IFACE_S_PROMISC_FIXED;
+ iface->flags &= ~GR_IFACE_F_PROMISC;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else | |
| iface->state &= multicast ? | |
| ~GR_IFACE_S_ALLMULTI_FIXED : | |
| ~GR_IFACE_S_PROMISC_FIXED; | |
| // Reinstall all addresses after disabling allmulti/promisc. | |
| if (multicast) { | |
| ret = rte_eth_dev_set_mc_addr_list( | |
| port->port_id, filter->mac, filter->count | |
| ); | |
| } else { | |
| for (i = 0; i < filter->count; i++) { | |
| int r = rte_eth_dev_mac_addr_add(port->port_id, &filter->mac[i], 0); | |
| if (r < 0 && ret == 0) | |
| ret = r; | |
| } | |
| } | |
| if (ret < 0) | |
| LOG(WARNING, "%s: %s: %s", iface->name, mac_type, rte_strerror(-ret)); | |
| else | |
| if (multicast) { | |
| iface->state &= ~GR_IFACE_S_ALLMULTI_FIXED; | |
| iface->flags &= ~GR_IFACE_F_ALLMULTI; | |
| } else { | |
| iface->state &= ~GR_IFACE_S_PROMISC_FIXED; | |
| iface->flags &= ~GR_IFACE_F_PROMISC; | |
| } | |
| // Reinstall all addresses after disabling allmulti/promisc. | |
| if (multicast) { | |
| ret = rte_eth_dev_set_mc_addr_list( | |
| port->port_id, filter->mac, filter->count | |
| ); | |
| } else { | |
| for (i = 0; i < filter->count; i++) { | |
| int r = rte_eth_dev_mac_addr_add(port->port_id, &filter->mac[i], 0); | |
| if (r < 0 && ret == 0) | |
| ret = r; | |
| } | |
| } | |
| if (ret < 0) | |
| LOG(WARNING, "%s: %s: %s", iface->name, mac_type, rte_strerror(-ret)); |
🤖 Prompt for AI Agents
In modules/infra/control/port.c around lines 674-692, after clearing the forced
state bits (iface->state &= multicast ? ~GR_IFACE_S_ALLMULTI_FIXED :
~GR_IFACE_S_PROMISC_FIXED;), also clear the corresponding iface->flags bit so
the software flags reflect that hardware allmulti/promisc was disabled: if
multicast clear GR_IFACE_F_ALLMULTI from iface->flags, otherwise clear
GR_IFACE_F_PROMISC. Place this update immediately after the state change and
before reinstalling addresses.
3 participants
When a port MAC filter table reaches its hardware limit or if the driver does not support individual address filtering, the code falls back to enabling promisc or allmulti mode to ensure all required traffic is still received. However, other code paths that manage these modes were unaware of this fallback mechanism and could inadvertently disable them, breaking MAC address filtering.
Introduce two new interface state flags to track when promisc or allmulti modes have been force-enabled due to MAC filter overflow/unsupported. The port_promisc_set() and port_allmulti_set() functions now check these forced states and refuse to modify the hardware state when the mode is required for filtering.
Update the CLI to distinguish between user-requested and filter-forced modes by displaying "promisc(forced)" or "allmulti(forced)" when applicable.
Summary by CodeRabbit
New Features
Behavioral Fixes
Chores
Tests