From 436d76cad765363621bce1ceaa9210979a3042d2 Mon Sep 17 00:00:00 2001 From: David Marchand Date: Thu, 26 Mar 2026 18:03:48 +0100 Subject: [PATCH 1/9] port: cleanup unit test symbols After refactoring multicast filtering, there is no need for wrapping some symbols. Signed-off-by: David Marchand --- modules/infra/control/meson.build | 3 --- modules/infra/control/port_test.c | 3 --- 2 files changed, 6 deletions(-) diff --git a/modules/infra/control/meson.build b/modules/infra/control/meson.build index c9008981c..cad6b435a 100644 --- a/modules/infra/control/meson.build +++ b/modules/infra/control/meson.build @@ -50,12 +50,9 @@ tests += [ { 'sources': files('port_test.c', 'port.c'), 'link_args': [ - '-Wl,--wrap=rte_eth_allmulticast_disable', '-Wl,--wrap=rte_eth_allmulticast_enable', - '-Wl,--wrap=rte_eth_allmulticast_get', '-Wl,--wrap=rte_eth_dev_mac_addr_add', '-Wl,--wrap=rte_eth_dev_mac_addr_remove', - '-Wl,--wrap=rte_eth_dev_set_mc_addr_list', '-Wl,--wrap=rte_eth_promiscuous_disable', '-Wl,--wrap=rte_eth_promiscuous_enable', '-Wl,--wrap=rte_eth_promiscuous_get', diff --git a/modules/infra/control/port_test.c b/modules/infra/control/port_test.c index b843df5ca..cc019105e 100644 --- a/modules/infra/control/port_test.c +++ b/modules/infra/control/port_test.c @@ -50,12 +50,9 @@ mock_func(int, port_unplug(struct iface_info_port *)); mock_func(int, port_plug(struct iface_info_port *)); mock_func(unsigned, worker_count(void)); mock_func(int, worker_queue_distribute(const cpu_set_t *, vec struct iface_info_port **)); -mock_func(int, __wrap_rte_eth_allmulticast_disable(uint16_t)); mock_func(int, __wrap_rte_eth_allmulticast_enable(uint16_t)); -mock_func(int, __wrap_rte_eth_allmulticast_get(uint16_t)); mock_func(int, __wrap_rte_eth_dev_mac_addr_add(uint16_t, struct rte_ether_addr *, uint32_t)); mock_func(int, __wrap_rte_eth_dev_mac_addr_remove(uint16_t, struct rte_ether_addr *)); -mock_func(int, __wrap_rte_eth_dev_set_mc_addr_list(uint16_t, struct rte_ether_addr *, uint32_t)); mock_func(int, __wrap_rte_eth_promiscuous_disable(uint16_t)); mock_func(int, __wrap_rte_eth_promiscuous_enable(uint16_t)); mock_func(int, __wrap_rte_eth_promiscuous_get(uint16_t)); From 1f9e7d910444db9a9529b3a5215384362bce6647 Mon Sep 17 00:00:00 2001 From: David Marchand Date: Wed, 15 Apr 2026 18:00:18 +0200 Subject: [PATCH 2/9] port: check for overflow on refcount refcount integer overflowing would be a nightmare to debug. Add explicit checks. Signed-off-by: David Marchand --- modules/infra/control/port.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/modules/infra/control/port.c b/modules/infra/control/port.c index 7ff89d82d..0373aaa6e 100644 --- a/modules/infra/control/port.c +++ b/modules/infra/control/port.c @@ -669,6 +669,9 @@ int port_mac_add(struct iface *iface, const struct rte_ether_addr *mac) { for (unsigned i = 0; i < port->filter.count; i++) { m = &port->filter.macs[i]; if (rte_is_same_ether_addr(&m->mac, mac)) { + if (m->refcnt == UINT_NUM_VALUES(m->refcnt) - 1) + return errno_set(EOVERFLOW); + LOG(DEBUG, "%s: mac " ETH_F " already filtered (refs=%u)", iface->name, @@ -749,6 +752,9 @@ int port_mac_del(struct iface *iface, const struct rte_ether_addr *mac) { return errno_set(ENOENT); found: + if (m->refcnt == 0) + return errno_set(EOVERFLOW); + if (--m->refcnt > 0) { LOG(DEBUG, "%s: mac " ETH_F " still filtered (refs=%u)", From 147fe566ff7feb7c84744493fea4c78d309cecd7 Mon Sep 17 00:00:00 2001 From: David Marchand Date: Thu, 26 Mar 2026 18:41:37 +0100 Subject: [PATCH 3/9] port: simplify secondary mac management The current code around secondary mac addresses is buggy if a hw limit is reached once, and mac addresses are removed under this limit: on the first removal, hw_limit is reset to 0 but never updated when re-adding the mac addresses. On the other hand, trying to evaluate the hw limit at runtime is tricky, as the hardware may reply differently depending on global resources availability. For example, when using VFs on Intel nics, all VFs shared a maximum number of 32k switching rules. For those reasons, prefer tracking if mac addresses got into the hardware or not. On deletion, try to add more mac, and disable promisc when all of them are in hardware. Signed-off-by: David Marchand --- modules/infra/control/port.c | 104 +++++++++++++++--------------- modules/infra/control/port.h | 10 +-- modules/infra/control/port_test.c | 31 ++++++--- 3 files changed, 76 insertions(+), 69 deletions(-) diff --git a/modules/infra/control/port.c b/modules/infra/control/port.c index 0373aaa6e..b9caa25de 100644 --- a/modules/infra/control/port.c +++ b/modules/infra/control/port.c @@ -686,30 +686,30 @@ int port_mac_add(struct iface *iface, const struct rte_ether_addr *mac) { m = &port->filter.macs[port->filter.count++]; m->refcnt = 1; + m->hardware = false; m->mac = *mac; LOG(INFO, "%s: enabling " ETH_F " mac filter", iface->name, mac); - if (iface->state & GR_IFACE_S_PROMISC_FIXED) - return 0; - ret = rte_eth_dev_mac_addr_add(port->port_id, (struct rte_ether_addr *)mac, 0); - if (ret == -ENOSPC || ret == -EOPNOTSUPP) { - if (ret == -ENOSPC) { - port->filter.flags |= MAC_FILTER_F_NOSPC; - port->filter.hw_limit = port->filter.count - 1; - LOG(INFO, "%s: %s", iface->name, rte_strerror(-ret)); - } else { - port->filter.flags |= MAC_FILTER_F_UNSUPP; + if (ret == 0) { + m->hardware = true; + } else if (ret == -ENOSPC || ret == -EOPNOTSUPP) { + if (ret == -ENOSPC) LOG(INFO, "%s: %s", iface->name, rte_strerror(-ret)); + else + LOG(DEBUG, "%s: %s", iface->name, rte_strerror(-ret)); + + ret = 0; + if ((iface->state & GR_IFACE_S_PROMISC_FIXED) == 0) { + ret = rte_eth_promiscuous_enable(port->port_id); + if (ret == 0) { + LOG(INFO, "%s: enabled promisc", iface->name); + iface->state |= GR_IFACE_S_PROMISC_FIXED; + } else { + LOG(INFO, "%s: %s", iface->name, rte_strerror(-ret)); + } } - - LOG(INFO, "%s: enabling promisc", iface->name); - - // promisc enable is a noop if already enabled - ret = rte_eth_promiscuous_enable(port->port_id); - if (ret == 0) - iface->state |= GR_IFACE_S_PROMISC_FIXED; } if (ret < 0) { @@ -717,16 +717,6 @@ int port_mac_add(struct iface *iface, const struct rte_ether_addr *mac) { return errno_set(-ret); } - if (iface->state & GR_IFACE_S_PROMISC_FIXED) { - // Purge the device from all explicit addresses. - // We will add them back when removing the forced allmulti/promisc. - // Ignore the return values. They don't matter. - for (unsigned i = 0; i < port->filter.count - 1; i++) { - m = &port->filter.macs[i]; - rte_eth_dev_mac_addr_remove(port->port_id, &m->mac); - } - } - return 0; } @@ -766,39 +756,49 @@ int port_mac_del(struct iface *iface, const struct rte_ether_addr *mac) { LOG(INFO, "%s: removing " ETH_F " mac filter", iface->name, mac); - if (port->filter.count > 1) { - // Swap the removed address with the last one - port->filter.macs[i] = port->filter.macs[port->filter.count - 1]; + if (m->hardware) { + ret = rte_eth_dev_mac_addr_remove(port->port_id, (struct rte_ether_addr *)mac); + if (ret < 0 && ret != -ENOTSUP) + LOG(WARNING, "%s: %s", iface->name, rte_strerror(-ret)); + } + + if (port->filter.count > 1 && i < port->filter.count - 1) { + memmove(&port->filter.macs[i], + &port->filter.macs[i + 1], + (port->filter.count - 1 - i) * sizeof(port->filter.macs[0])); } port->filter.count--; if (iface->state & GR_IFACE_S_PROMISC_FIXED) { - if (port->filter.count > 0 && port->filter.flags & MAC_FILTER_F_UNSUPP) - return 0; - if (port->filter.count > port->filter.hw_limit - && port->filter.flags & MAC_FILTER_F_NOSPC) - return 0; - port->filter.flags = 0; - port->filter.hw_limit = 0; - ret = rte_eth_promiscuous_disable(port->port_id); - if (ret < 0 && ret != -ENOTSUP) - LOG(NOTICE, "%s: promisc disable: %s", iface->name, rte_strerror(-ret)); - else - iface->state &= ~GR_IFACE_S_PROMISC_FIXED; + bool disable_promisc = true; - // Reinstall all addresses after disabling promisc. - for (i = 0; i < port->filter.count; i++) { + LOG(INFO, "i=%d, port->filter.count=%d", i, port->filter.count); + for (; i < port->filter.count; i++) { m = &port->filter.macs[i]; - int r = rte_eth_dev_mac_addr_add(port->port_id, &m->mac, 0); - if (r < 0 && ret == 0) - ret = r; + if (m->hardware) + continue; + ret = rte_eth_dev_mac_addr_add(port->port_id, &m->mac, 0); + if (ret < 0) { + LOG(INFO, "%s: %s", iface->name, rte_strerror(-ret)); + disable_promisc = false; + break; + } + m->hardware = true; } - } else { - ret = rte_eth_dev_mac_addr_remove(port->port_id, (struct rte_ether_addr *)mac); - } - if (ret < 0 && ret != -ENOTSUP) - LOG(WARNING, "%s: %s", iface->name, rte_strerror(-ret)); + if (disable_promisc) { + ret = rte_eth_promiscuous_disable(port->port_id); + if (ret < 0 && ret != -ENOTSUP) { + LOG(NOTICE, + "%s: promisc disable: %s", + iface->name, + rte_strerror(-ret)); + } else { + LOG(INFO, "%s: disabled promisc", iface->name); + iface->state &= ~GR_IFACE_S_PROMISC_FIXED; + } + } + } return 0; } diff --git a/modules/infra/control/port.h b/modules/infra/control/port.h index adbd2e3b0..514169fcc 100644 --- a/modules/infra/control/port.h +++ b/modules/infra/control/port.h @@ -14,13 +14,9 @@ #include -typedef enum { - MAC_FILTER_F_UNSUPP = GR_BIT8(0), - MAC_FILTER_F_NOSPC = GR_BIT8(1), -} mac_filter_flags_t; - struct port_mac { - uint16_t refcnt; + uint8_t refcnt; + bool hardware; struct rte_ether_addr mac; }; @@ -38,8 +34,6 @@ GR_IFACE_INFO(GR_IFACE_TYPE_PORT, iface_info_port, { uint64_t rx_offloads; rte_spinlock_t txq_locks[RTE_MAX_QUEUES_PER_PORT]; struct { - mac_filter_flags_t flags; - unsigned hw_limit; unsigned count; struct port_mac macs[RTE_ETH_NUM_RECEIVE_MAC_ADDR]; } filter; diff --git a/modules/infra/control/port_test.c b/modules/infra/control/port_test.c index cc019105e..8c9f5373e 100644 --- a/modules/infra/control/port_test.c +++ b/modules/infra/control/port_test.c @@ -102,27 +102,34 @@ static void mac_add_unicast(void **) { will_return(__wrap_rte_eth_dev_mac_addr_add, 0); assert_return_code(port_mac_add(iface, &ucast1), errno); + assert_false(iface->state & GR_IFACE_S_PROMISC_FIXED); assert_int_equal(port->filter.count, 1); + assert_memory_equal(&port->filter.macs[0].mac, &ucast1, sizeof(ucast1)); assert_int_equal(port->filter.macs[0].refcnt, 1); + assert_true(port->filter.macs[0].hardware); will_return(__wrap_rte_eth_dev_mac_addr_add, 0); assert_return_code(port_mac_add(iface, &ucast2), errno); + assert_false(iface->state & GR_IFACE_S_PROMISC_FIXED); assert_int_equal(port->filter.count, 2); + assert_memory_equal(&port->filter.macs[1].mac, &ucast2, sizeof(ucast2)); assert_int_equal(port->filter.macs[1].refcnt, 1); + assert_true(port->filter.macs[1].hardware); assert_return_code(port_mac_add(iface, &ucast1), errno); assert_int_equal(port->filter.count, 2); + assert_memory_equal(&port->filter.macs[0].mac, &ucast1, sizeof(ucast1)); assert_int_equal(port->filter.macs[0].refcnt, 2); + assert_true(port->filter.macs[0].hardware); will_return(__wrap_rte_eth_dev_mac_addr_add, -ENOSPC); will_return(__wrap_rte_eth_promiscuous_enable, 0); - will_return(__wrap_rte_eth_dev_mac_addr_remove, 0); - will_return(__wrap_rte_eth_dev_mac_addr_remove, 0); assert_return_code(port_mac_add(iface, &ucast3), errno); - assert_true(port->filter.flags & MAC_FILTER_F_NOSPC); assert_true(iface->state & GR_IFACE_S_PROMISC_FIXED); assert_int_equal(port->filter.count, 3); - assert_int_equal(port->filter.hw_limit, 2); + assert_memory_equal(&port->filter.macs[2].mac, &ucast3, sizeof(ucast3)); + assert_int_equal(port->filter.macs[2].refcnt, 1); + assert_false(port->filter.macs[2].hardware); } static void mac_del_unicast(void **) { @@ -133,16 +140,22 @@ static void mac_del_unicast(void **) { assert_return_code(port_mac_del(iface, &ucast1), errno); assert_true(iface->state & GR_IFACE_S_PROMISC_FIXED); + assert_memory_equal(&port->filter.macs[0].mac, &ucast1, sizeof(ucast1)); + assert_int_equal(port->filter.macs[0].refcnt, 1); + assert_true(port->filter.macs[0].hardware); - will_return(__wrap_rte_eth_promiscuous_disable, 0); - will_return(__wrap_rte_eth_dev_mac_addr_add, 0); + will_return(__wrap_rte_eth_dev_mac_addr_remove, 0); will_return(__wrap_rte_eth_dev_mac_addr_add, 0); + will_return(__wrap_rte_eth_promiscuous_disable, 0); assert_return_code(port_mac_del(iface, &ucast1), errno); assert_false(iface->state & GR_IFACE_S_PROMISC_FIXED); assert_int_equal(port->filter.count, 2); - assert_int_equal(port->filter.hw_limit, 0); - assert_memory_equal(&port->filter.macs[0].mac, &ucast3, sizeof(ucast3)); - assert_memory_equal(&port->filter.macs[1].mac, &ucast2, sizeof(ucast2)); + assert_memory_equal(&port->filter.macs[0].mac, &ucast2, sizeof(ucast2)); + assert_int_equal(port->filter.macs[0].refcnt, 1); + assert_true(port->filter.macs[0].hardware); + assert_memory_equal(&port->filter.macs[1].mac, &ucast3, sizeof(ucast3)); + assert_int_equal(port->filter.macs[1].refcnt, 1); + assert_true(port->filter.macs[1].hardware); } int main(void) { From fcc46138063fb0ba26397223a9d33da2b3358773 Mon Sep 17 00:00:00 2001 From: David Marchand Date: Wed, 15 Apr 2026 10:20:01 +0200 Subject: [PATCH 4/9] iface: prepare for more mac related unit tests Move the tests around port mac addresses one layer higher by calling iface_XX API. For now, only port type is tested, we can extend those tests with other iface types later. Signed-off-by: David Marchand --- .../control/{port_test.c => iface_test.c} | 40 +++++++++---------- modules/infra/control/meson.build | 2 +- modules/infra/control/port.c | 6 +-- modules/infra/control/port.h | 4 -- 4 files changed, 22 insertions(+), 30 deletions(-) rename modules/infra/control/{port_test.c => iface_test.c} (82%) diff --git a/modules/infra/control/port_test.c b/modules/infra/control/iface_test.c similarity index 82% rename from modules/infra/control/port_test.c rename to modules/infra/control/iface_test.c index 8c9f5373e..e495bd7d1 100644 --- a/modules/infra/control/port_test.c +++ b/modules/infra/control/iface_test.c @@ -4,6 +4,7 @@ #include "_cmocka.h" #include "config.h" #include "event.h" +#include "iface.h" #include "log.h" #include "mempool.h" #include "module.h" @@ -18,9 +19,7 @@ int gr_rte_log_type; struct log_types log_types = STAILQ_HEAD_INITIALIZER(log_types); struct gr_config gr_config; struct workers workers; -void __api_handler(uint32_t, api_handler_func, const char *, size_t) { } void module_register(struct module *) { } -void iface_type_register(const struct iface_type *) { } void event_push(uint32_t, const void *) { } void event_subscribe(uint32_t, event_sub_cb_t) { } int netlink_link_set_name(uint32_t, const char *) { @@ -35,17 +34,14 @@ struct rte_rcu_qsbr *gr_datapath_rcu(void) { static struct rte_rcu_qsbr rcu; return &rcu; } -uint16_t vrf_default_get_or_create(void) { - return 0; -} int vrf_incref(uint16_t) { return 0; } -int iface_set_eth_addr(struct iface *, const struct rte_ether_addr *) { - return 0; +void vrf_decref(uint16_t) { } +bool vrf_has_interfaces(uint16_t) { + return false; } -mock_func(struct iface *, iface_from_id(uint16_t)); -mock_func(struct iface *, iface_next(gr_iface_type_t, const struct iface *)); +void control_queue_drain(uint32_t, const void *) { } mock_func(int, port_unplug(struct iface_info_port *)); mock_func(int, port_plug(struct iface_info_port *)); mock_func(unsigned, worker_count(void)); @@ -93,15 +89,15 @@ static const struct rte_ether_addr ucast1 = {{0x2c, 0x4c, 0x15, 0x07, 0x99, 0x22 static const struct rte_ether_addr ucast2 = {{0x30, 0x3e, 0xa7, 0x0b, 0xea, 0x78}}; static const struct rte_ether_addr ucast3 = {{0xe6, 0x2c, 0xd9, 0xa5, 0xe7, 0x6e}}; -static void mac_add_unicast(void **) { +static void port_mac_add_unicast(void **) { const struct iface_info_port *port = iface_info_port(iface); - assert_int_equal(port_mac_add(iface, NULL), -EINVAL); - assert_return_code(port_mac_add(iface, &default_mac), errno); + assert_int_equal(iface_add_eth_addr(iface, NULL), -EINVAL); + assert_return_code(iface_add_eth_addr(iface, &default_mac), errno); assert_int_equal(port->filter.count, 0); will_return(__wrap_rte_eth_dev_mac_addr_add, 0); - assert_return_code(port_mac_add(iface, &ucast1), errno); + assert_return_code(iface_add_eth_addr(iface, &ucast1), errno); assert_false(iface->state & GR_IFACE_S_PROMISC_FIXED); assert_int_equal(port->filter.count, 1); assert_memory_equal(&port->filter.macs[0].mac, &ucast1, sizeof(ucast1)); @@ -109,14 +105,14 @@ static void mac_add_unicast(void **) { assert_true(port->filter.macs[0].hardware); will_return(__wrap_rte_eth_dev_mac_addr_add, 0); - assert_return_code(port_mac_add(iface, &ucast2), errno); + assert_return_code(iface_add_eth_addr(iface, &ucast2), errno); assert_false(iface->state & GR_IFACE_S_PROMISC_FIXED); assert_int_equal(port->filter.count, 2); assert_memory_equal(&port->filter.macs[1].mac, &ucast2, sizeof(ucast2)); assert_int_equal(port->filter.macs[1].refcnt, 1); assert_true(port->filter.macs[1].hardware); - assert_return_code(port_mac_add(iface, &ucast1), errno); + assert_return_code(iface_add_eth_addr(iface, &ucast1), errno); assert_int_equal(port->filter.count, 2); assert_memory_equal(&port->filter.macs[0].mac, &ucast1, sizeof(ucast1)); assert_int_equal(port->filter.macs[0].refcnt, 2); @@ -124,7 +120,7 @@ static void mac_add_unicast(void **) { will_return(__wrap_rte_eth_dev_mac_addr_add, -ENOSPC); will_return(__wrap_rte_eth_promiscuous_enable, 0); - assert_return_code(port_mac_add(iface, &ucast3), errno); + assert_return_code(iface_add_eth_addr(iface, &ucast3), errno); assert_true(iface->state & GR_IFACE_S_PROMISC_FIXED); assert_int_equal(port->filter.count, 3); assert_memory_equal(&port->filter.macs[2].mac, &ucast3, sizeof(ucast3)); @@ -132,13 +128,13 @@ static void mac_add_unicast(void **) { assert_false(port->filter.macs[2].hardware); } -static void mac_del_unicast(void **) { +static void port_mac_del_unicast(void **) { const struct iface_info_port *port = iface_info_port(iface); - assert_return_code(port_promisc_set(iface, false), errno); + assert_return_code(iface_set_promisc(iface, false), errno); assert_true(iface->state & GR_IFACE_S_PROMISC_FIXED); - assert_return_code(port_mac_del(iface, &ucast1), errno); + assert_return_code(iface_del_eth_addr(iface, &ucast1), errno); assert_true(iface->state & GR_IFACE_S_PROMISC_FIXED); assert_memory_equal(&port->filter.macs[0].mac, &ucast1, sizeof(ucast1)); assert_int_equal(port->filter.macs[0].refcnt, 1); @@ -147,7 +143,7 @@ static void mac_del_unicast(void **) { will_return(__wrap_rte_eth_dev_mac_addr_remove, 0); will_return(__wrap_rte_eth_dev_mac_addr_add, 0); will_return(__wrap_rte_eth_promiscuous_disable, 0); - assert_return_code(port_mac_del(iface, &ucast1), errno); + assert_return_code(iface_del_eth_addr(iface, &ucast1), errno); assert_false(iface->state & GR_IFACE_S_PROMISC_FIXED); assert_int_equal(port->filter.count, 2); assert_memory_equal(&port->filter.macs[0].mac, &ucast2, sizeof(ucast2)); @@ -160,8 +156,8 @@ static void mac_del_unicast(void **) { int main(void) { const struct CMUnitTest tests[] = { - cmocka_unit_test(mac_add_unicast), - cmocka_unit_test(mac_del_unicast), + cmocka_unit_test(port_mac_add_unicast), + cmocka_unit_test(port_mac_del_unicast), }; return cmocka_run_group_tests(tests, setup, teardown); } diff --git a/modules/infra/control/meson.build b/modules/infra/control/meson.build index cad6b435a..c3150b78a 100644 --- a/modules/infra/control/meson.build +++ b/modules/infra/control/meson.build @@ -48,7 +48,7 @@ tests += [ ], }, { - 'sources': files('port_test.c', 'port.c'), + 'sources': files('iface_test.c', 'iface.c', 'port.c'), 'link_args': [ '-Wl,--wrap=rte_eth_allmulticast_enable', '-Wl,--wrap=rte_eth_dev_mac_addr_add', diff --git a/modules/infra/control/port.c b/modules/infra/control/port.c index b9caa25de..5032cdb66 100644 --- a/modules/infra/control/port.c +++ b/modules/infra/control/port.c @@ -219,7 +219,7 @@ static int port_mac_set(struct iface *iface, const struct rte_ether_addr *mac) { return 0; } -int port_promisc_set(struct iface *iface, bool enabled) { +static int port_promisc_set(struct iface *iface, bool enabled) { struct iface_info_port *p = iface_info_port(iface); int ret; @@ -653,7 +653,7 @@ static int port_mac_get(const struct iface *iface, struct rte_ether_addr *mac) { return 0; } -int port_mac_add(struct iface *iface, const struct rte_ether_addr *mac) { +static int port_mac_add(struct iface *iface, const struct rte_ether_addr *mac) { struct iface_info_port *port = iface_info_port(iface); struct port_mac *m; int ret; @@ -720,7 +720,7 @@ int port_mac_add(struct iface *iface, const struct rte_ether_addr *mac) { return 0; } -int port_mac_del(struct iface *iface, const struct rte_ether_addr *mac) { +static int port_mac_del(struct iface *iface, const struct rte_ether_addr *mac) { struct iface_info_port *port = iface_info_port(iface); struct port_mac *m; uint8_t i; diff --git a/modules/infra/control/port.h b/modules/infra/control/port.h index 514169fcc..5190c0feb 100644 --- a/modules/infra/control/port.h +++ b/modules/infra/control/port.h @@ -40,7 +40,3 @@ GR_IFACE_INFO(GR_IFACE_TYPE_PORT, iface_info_port, { }); const struct iface *port_get_iface(uint16_t port_id); - -int port_mac_add(struct iface *, const struct rte_ether_addr *); -int port_mac_del(struct iface *, const struct rte_ether_addr *); -int port_promisc_set(struct iface *, bool enabled); From da1191ddb242ce70b9e1fb622789d0aa02d738c0 Mon Sep 17 00:00:00 2001 From: David Marchand Date: Thu, 26 Mar 2026 14:01:04 +0100 Subject: [PATCH 5/9] iface: validate mac at the API level This avoids revalidating it in the interface specific ops. The check for adding multicast mac addresses in the vlan iface code was left behind from when only multicast addresses mattered. No code directly calls ->add_eth_addr op, so validate the mac input parameter at the iface API level. Besides, checking if the passed mac address is the same as the primary mac can be moved in a single location. Signed-off-by: David Marchand --- modules/infra/control/iface.c | 23 +++++++++++++++++++++-- modules/infra/control/iface_test.c | 10 ++++++++++ modules/infra/control/port.c | 16 ---------------- modules/infra/control/vlan.c | 6 ------ 4 files changed, 31 insertions(+), 24 deletions(-) diff --git a/modules/infra/control/iface.c b/modules/infra/control/iface.c index f209067be..ad9ee2b59 100644 --- a/modules/infra/control/iface.c +++ b/modules/infra/control/iface.c @@ -465,6 +465,13 @@ struct iface *iface_from_id(uint16_t ifid) { return iface; } +static bool iface_is_default_mac(struct iface *iface, const struct rte_ether_addr *mac) { + struct rte_ether_addr default_mac; + + return iface_get_eth_addr(iface, &default_mac) == 0 + && rte_is_same_ether_addr(&default_mac, mac); +} + int iface_get_eth_addr(const struct iface *iface, struct rte_ether_addr *mac) { const struct iface_type *type; @@ -518,9 +525,15 @@ int iface_set_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) { int iface_add_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) { const struct iface_type *type; - if (iface == NULL) + if (iface == NULL || mac == NULL) return errno_set(EINVAL); + if (rte_is_multicast_ether_addr(mac)) + return 0; // ALLMULTI is always on + + if (iface_is_default_mac(iface, mac)) + return 0; + type = iface_type_get(iface->type); assert(type != NULL); if (type->add_eth_addr == NULL) @@ -532,9 +545,15 @@ int iface_add_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) { int iface_del_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) { const struct iface_type *type; - if (iface == NULL) + if (iface == NULL || mac == NULL) return errno_set(EINVAL); + if (rte_is_multicast_ether_addr(mac)) + return 0; // ALLMULTI is always on + + if (iface_is_default_mac(iface, mac)) + return 0; + type = iface_type_get(iface->type); assert(type != NULL); if (type->del_eth_addr == NULL) diff --git a/modules/infra/control/iface_test.c b/modules/infra/control/iface_test.c index e495bd7d1..ce2f547de 100644 --- a/modules/infra/control/iface_test.c +++ b/modules/infra/control/iface_test.c @@ -85,14 +85,23 @@ static int teardown(void **) { return 0; } +static const struct rte_ether_addr mcast1 = {{0x33, 0x33, 0x00, 0x00, 0x00, 0x01}}; static const struct rte_ether_addr ucast1 = {{0x2c, 0x4c, 0x15, 0x07, 0x99, 0x22}}; static const struct rte_ether_addr ucast2 = {{0x30, 0x3e, 0xa7, 0x0b, 0xea, 0x78}}; static const struct rte_ether_addr ucast3 = {{0xe6, 0x2c, 0xd9, 0xa5, 0xe7, 0x6e}}; +static void port_mac_add_multicast(void **) { + const struct iface_info_port *port = iface_info_port(iface); + + assert_return_code(iface_add_eth_addr(iface, &mcast1), errno); + assert_int_equal(port->filter.count, 0); +} + static void port_mac_add_unicast(void **) { const struct iface_info_port *port = iface_info_port(iface); assert_int_equal(iface_add_eth_addr(iface, NULL), -EINVAL); + assert_return_code(iface_add_eth_addr(iface, &default_mac), errno); assert_int_equal(port->filter.count, 0); @@ -156,6 +165,7 @@ static void port_mac_del_unicast(void **) { int main(void) { const struct CMUnitTest tests[] = { + cmocka_unit_test(port_mac_add_multicast), cmocka_unit_test(port_mac_add_unicast), cmocka_unit_test(port_mac_del_unicast), }; diff --git a/modules/infra/control/port.c b/modules/infra/control/port.c index 5032cdb66..a0b6e49cb 100644 --- a/modules/infra/control/port.c +++ b/modules/infra/control/port.c @@ -658,14 +658,6 @@ static int port_mac_add(struct iface *iface, const struct rte_ether_addr *mac) { struct port_mac *m; int ret; - if (mac == NULL) - return errno_set(EINVAL); - if (rte_is_multicast_ether_addr(mac)) - return 0; // ALLMULTI is always on - - if (rte_is_same_ether_addr(mac, &port->mac)) - return 0; - for (unsigned i = 0; i < port->filter.count; i++) { m = &port->filter.macs[i]; if (rte_is_same_ether_addr(&m->mac, mac)) { @@ -726,14 +718,6 @@ static int port_mac_del(struct iface *iface, const struct rte_ether_addr *mac) { uint8_t i; int ret; - if (mac == NULL) - return errno_set(EINVAL); - if (rte_is_multicast_ether_addr(mac)) - return 0; // ALLMULTI is always on - - if (rte_is_same_ether_addr(mac, &port->mac)) - return 0; - for (i = 0; i < port->filter.count; i++) { m = &port->filter.macs[i]; if (rte_is_same_ether_addr(&m->mac, mac)) diff --git a/modules/infra/control/vlan.c b/modules/infra/control/vlan.c index b3902cd8b..257a6841c 100644 --- a/modules/infra/control/vlan.c +++ b/modules/infra/control/vlan.c @@ -204,9 +204,6 @@ static int iface_vlan_add_eth_addr(struct iface *iface, const struct rte_ether_a const struct iface_info_vlan *vlan = iface_info_vlan(iface); struct iface *parent = iface_from_id(vlan->parent_id); - if (mac == NULL || !rte_is_multicast_ether_addr(mac)) - return errno_set(EINVAL); - return iface_add_eth_addr(parent, mac); } @@ -214,9 +211,6 @@ static int iface_vlan_del_eth_addr(struct iface *iface, const struct rte_ether_a const struct iface_info_vlan *vlan = iface_info_vlan(iface); struct iface *parent = iface_from_id(vlan->parent_id); - if (mac == NULL || !rte_is_multicast_ether_addr(mac)) - return errno_set(EINVAL); - return iface_del_eth_addr(parent, mac); } From 6714d43a4ff0cac877ab510bb668a611243ffd4d Mon Sep 17 00:00:00 2001 From: David Marchand Date: Tue, 14 Apr 2026 17:27:30 +0200 Subject: [PATCH 6/9] iface: track mac addresses Only tracking mac addresses at the port level (even with a refcount) is broken when considering stacked interfaces, and ifaces reconfigurations. Make the concept generic by introducing a vector of mac addresses at the iface level. By moving the logic to iface level, .add_eth_addr/.del_eth_addr are now only called on the first addition/last removal of a mac address. We can then remove bond and port specific mac tracking. And finally as a bonus, this removes the limit to RTE_ETH_NUM_RECEIVE_MAC_ADDR macs in port ifaces. Note: RTE_ETH_NUM_RECEIVE_MAC_ADDR is a confusing macro that was introduced for VMDq support in DPDK, while many nics support more mac addresses. Signed-off-by: David Marchand --- modules/infra/control/bond.c | 34 ++---- modules/infra/control/bond.h | 3 - modules/infra/control/iface.c | 52 ++++++++- modules/infra/control/iface.h | 11 +- modules/infra/control/iface_test.c | 168 +++++++++++++++++++++++------ modules/infra/control/meson.build | 3 +- modules/infra/control/port.c | 79 ++------------ modules/infra/control/port.h | 10 -- modules/infra/control/vlan.c | 8 +- 9 files changed, 216 insertions(+), 152 deletions(-) diff --git a/modules/infra/control/bond.c b/modules/infra/control/bond.c index 668b37397..34a8c531a 100644 --- a/modules/infra/control/bond.c +++ b/modules/infra/control/bond.c @@ -40,32 +40,16 @@ bond_all_member_del_mac(const struct iface_info_bond *bond, const struct rte_eth return 0; } -static int bond_mac_add(struct iface *iface, const struct rte_ether_addr *mac) { +static int bond_mac_add(struct iface *iface, struct iface_mac *m) { struct iface_info_bond *bond = iface_info_bond(iface); - int ret; - - // Add MAC address to all member ports - if ((ret = bond_all_member_add_mac(bond, mac)) < 0) - return ret; - vec_add(bond->extra_macs, *mac); - - return 0; + return bond_all_member_add_mac(bond, &m->mac); } -static int bond_mac_del(struct iface *iface, const struct rte_ether_addr *mac) { +static int bond_mac_del(struct iface *iface, struct iface_mac *m) { struct iface_info_bond *bond = iface_info_bond(iface); - // Remove MAC address from all member ports - bond_all_member_del_mac(bond, mac); - - for (unsigned i = 0; i < vec_len(bond->extra_macs); i++) { - if (rte_is_same_ether_addr(&bond->extra_macs[i], mac)) { - vec_del(bond->extra_macs, i); - break; - } - } - + bond_all_member_del_mac(bond, &m->mac); return 0; } @@ -173,8 +157,8 @@ static int bond_attach_member(struct iface *iface, struct iface *member) { if (bond->n_members == ARRAY_DIM(bond->members)) return errno_set(EUSERS); - vec_foreach_ref (struct rte_ether_addr *mac, bond->extra_macs) { - if (iface_add_eth_addr(member, mac) < 0) + vec_foreach_ref (struct iface_mac *m, iface->macs) { + if (iface_add_eth_addr(member, &m->mac) < 0) return errno_log(errno, "iface_add_eth_addr(member)"); } @@ -223,8 +207,8 @@ static int bond_detach_member(struct iface *iface, struct iface *member) { bond->n_members--; if (bond->primary_member >= bond->n_members) bond->primary_member--; - vec_foreach_ref (struct rte_ether_addr *mac, bond->extra_macs) { - if (iface_del_eth_addr(member, mac) < 0 && errno != ENOENT) { + vec_foreach_ref (struct iface_mac *m, iface->macs) { + if (iface_del_eth_addr(member, &m->mac) < 0 && errno != ENOENT) { LOG(WARNING, "failed to unconfigure mac address on member %s: %s", member->name, @@ -412,8 +396,6 @@ static int bond_fini(struct iface *iface) { event_push(GR_EVENT_IFACE_POST_RECONFIG, member); } - vec_free(bond->extra_macs); - return 0; } diff --git a/modules/infra/control/bond.h b/modules/infra/control/bond.h index 4a800bb8d..371c5bad5 100644 --- a/modules/infra/control/bond.h +++ b/modules/infra/control/bond.h @@ -5,7 +5,6 @@ #include "iface.h" #include "lacp.h" -#include "vec.h" #include @@ -34,8 +33,6 @@ GR_IFACE_INFO(GR_IFACE_TYPE_BOND, iface_info_bond, { uint8_t n_members; struct bond_member members[MEMBERS_MAX_LEN]; - vec struct rte_ether_addr *extra_macs; - uint8_t redirection_table[256]; }); diff --git a/modules/infra/control/iface.c b/modules/infra/control/iface.c index ad9ee2b59..e712c9ed6 100644 --- a/modules/infra/control/iface.c +++ b/modules/infra/control/iface.c @@ -524,6 +524,8 @@ int iface_set_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) { int iface_add_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) { const struct iface_type *type; + struct iface_mac new_m; + int ret; if (iface == NULL || mac == NULL) return errno_set(EINVAL); @@ -539,7 +541,27 @@ int iface_add_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) { if (type->add_eth_addr == NULL) return errno_set(EOPNOTSUPP); - return type->add_eth_addr(iface, mac); + vec_foreach_ref (struct iface_mac *m, iface->macs) { + if (!rte_is_same_ether_addr(mac, &m->mac)) + continue; + + if (m->refcnt == UINT_NUM_VALUES(m->refcnt) - 1) + return errno_set(EOVERFLOW); + + LOG(DEBUG, + "%s: mac " ETH_F " already filtered (refs=%u)", + iface->name, + mac, + m->refcnt); + m->refcnt++; + return 0; + } + + new_m = (struct iface_mac) {.refcnt = 1, .mac = *mac, .hardware = false}; + ret = type->add_eth_addr(iface, &new_m); + if (ret == 0) + vec_add(iface->macs, new_m); + return ret; } int iface_del_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) { @@ -559,7 +581,32 @@ int iface_del_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) { if (type->del_eth_addr == NULL) return errno_set(EOPNOTSUPP); - return type->del_eth_addr(iface, mac); + vec_foreach_ref (struct iface_mac *m, iface->macs) { + int ret; + + if (!rte_is_same_ether_addr(mac, &m->mac)) + continue; + + if (m->refcnt == 0) + return errno_set(EOVERFLOW); + + if (m->refcnt > 1) { + m->refcnt--; + LOG(DEBUG, + "%s: mac " ETH_F " still filtered (refs=%u)", + iface->name, + mac, + m->refcnt); + ret = 0; + } else { + ret = type->del_eth_addr(iface, m); + vec_del(iface->macs, m - iface->macs); + } + + return ret; + } + + return errno_set(ENOENT); } int iface_set_mtu(struct iface *iface, uint16_t mtu) { @@ -677,6 +724,7 @@ int iface_destroy(struct iface *iface) { free(iface->name); free(iface->description); vec_free(iface->subinterfaces); + vec_free(iface->macs); rte_free(iface); return ret; diff --git a/modules/infra/control/iface.h b/modules/infra/control/iface.h index 337a1d090..37ed8ecb2 100644 --- a/modules/infra/control/iface.h +++ b/modules/infra/control/iface.h @@ -11,6 +11,12 @@ #include #include +struct iface_mac { + uint8_t refcnt; + bool hardware; + struct rte_ether_addr mac; +}; + struct __rte_cache_aligned iface { BASE(__gr_iface_base); @@ -22,6 +28,7 @@ struct __rte_cache_aligned iface { struct event *cp_ev; // libevent to poll cp_fd unsigned promisc; bool user_promisc; + vec struct iface_mac *macs; alignas(alignof(void *)) uint8_t info[/* size depends on type */]; }; @@ -50,8 +57,8 @@ struct iface_type { int (*detach_domain)(struct iface *domain, struct iface *iface); int (*get_eth_addr)(const struct iface *, struct rte_ether_addr *); int (*set_eth_addr)(struct iface *, const struct rte_ether_addr *); - int (*add_eth_addr)(struct iface *, const struct rte_ether_addr *); - int (*del_eth_addr)(struct iface *, const struct rte_ether_addr *); + int (*add_eth_addr)(struct iface *, struct iface_mac *); + int (*del_eth_addr)(struct iface *, struct iface_mac *); int (*set_up_down)(struct iface *, bool up); int (*set_mtu)(struct iface *, uint16_t mtu); int (*set_promisc)(struct iface *, bool enabled); diff --git a/modules/infra/control/iface_test.c b/modules/infra/control/iface_test.c index ce2f547de..5612764ec 100644 --- a/modules/infra/control/iface_test.c +++ b/modules/infra/control/iface_test.c @@ -11,6 +11,7 @@ #include "netlink.h" #include "port.h" #include "rcu.h" +#include "vlan.h" #include "vrf.h" #include "worker.h" @@ -42,6 +43,7 @@ bool vrf_has_interfaces(uint16_t) { return false; } void control_queue_drain(uint32_t, const void *) { } +mock_func(struct iface *, __wrap_iface_from_id(uint16_t)); mock_func(int, port_unplug(struct iface_info_port *)); mock_func(int, port_plug(struct iface_info_port *)); mock_func(unsigned, worker_count(void)); @@ -59,9 +61,12 @@ void metric_emit(struct metrics_ctx *, const struct metric *, uint64_t) { } // test harness init static const struct rte_ether_addr default_mac = {{0x02, 0xf0, 0x00, 0xb4, 0x47, 0x01}}; static struct iface *iface; +static const struct rte_ether_addr default_vlan_mac = {{0x02, 0xff, 0x00, 0xb4, 0x47, 0x01}}; +static struct iface *vlan_iface; static int setup(void **) { struct iface_info_port *port; + struct iface_info_vlan *vlan; iface = calloc(1, sizeof(*iface) + sizeof(*port)); assert_non_null(iface); @@ -76,12 +81,27 @@ static int setup(void **) { port->n_txq = 2; port->mac = default_mac; + vlan_iface = calloc(1, sizeof(*vlan_iface) + sizeof(*vlan)); + assert_non_null(vlan_iface); + vlan_iface->name = strdup("p0.23"); + vlan_iface->type = GR_IFACE_TYPE_VLAN; + vlan_iface->flags = GR_IFACE_F_UP; + vlan_iface->state = GR_IFACE_S_RUNNING; + vlan = iface_info_vlan(vlan_iface); + vlan->parent_id = 42; + vlan->vlan_id = 23; + vlan->mac = default_vlan_mac; + return 0; } static int teardown(void **) { free(iface->name); + vec_free(iface->macs); free(iface); + free(vlan_iface->name); + vec_free(vlan_iface->macs); + free(vlan_iface); return 0; } @@ -91,76 +111,153 @@ static const struct rte_ether_addr ucast2 = {{0x30, 0x3e, 0xa7, 0x0b, 0xea, 0x78 static const struct rte_ether_addr ucast3 = {{0xe6, 0x2c, 0xd9, 0xa5, 0xe7, 0x6e}}; static void port_mac_add_multicast(void **) { - const struct iface_info_port *port = iface_info_port(iface); - assert_return_code(iface_add_eth_addr(iface, &mcast1), errno); - assert_int_equal(port->filter.count, 0); + assert_int_equal(vec_len(iface->macs), 0); } static void port_mac_add_unicast(void **) { - const struct iface_info_port *port = iface_info_port(iface); - assert_int_equal(iface_add_eth_addr(iface, NULL), -EINVAL); assert_return_code(iface_add_eth_addr(iface, &default_mac), errno); - assert_int_equal(port->filter.count, 0); + assert_int_equal(vec_len(iface->macs), 0); will_return(__wrap_rte_eth_dev_mac_addr_add, 0); assert_return_code(iface_add_eth_addr(iface, &ucast1), errno); assert_false(iface->state & GR_IFACE_S_PROMISC_FIXED); - assert_int_equal(port->filter.count, 1); - assert_memory_equal(&port->filter.macs[0].mac, &ucast1, sizeof(ucast1)); - assert_int_equal(port->filter.macs[0].refcnt, 1); - assert_true(port->filter.macs[0].hardware); + assert_int_equal(vec_len(iface->macs), 1); + assert_memory_equal(&iface->macs[0].mac, &ucast1, sizeof(ucast1)); + assert_int_equal(iface->macs[0].refcnt, 1); + assert_true(iface->macs[0].hardware); will_return(__wrap_rte_eth_dev_mac_addr_add, 0); assert_return_code(iface_add_eth_addr(iface, &ucast2), errno); assert_false(iface->state & GR_IFACE_S_PROMISC_FIXED); - assert_int_equal(port->filter.count, 2); - assert_memory_equal(&port->filter.macs[1].mac, &ucast2, sizeof(ucast2)); - assert_int_equal(port->filter.macs[1].refcnt, 1); - assert_true(port->filter.macs[1].hardware); + assert_int_equal(vec_len(iface->macs), 2); + assert_memory_equal(&iface->macs[1].mac, &ucast2, sizeof(ucast2)); + assert_int_equal(iface->macs[1].refcnt, 1); + assert_true(iface->macs[1].hardware); assert_return_code(iface_add_eth_addr(iface, &ucast1), errno); - assert_int_equal(port->filter.count, 2); - assert_memory_equal(&port->filter.macs[0].mac, &ucast1, sizeof(ucast1)); - assert_int_equal(port->filter.macs[0].refcnt, 2); - assert_true(port->filter.macs[0].hardware); + assert_int_equal(vec_len(iface->macs), 2); + assert_memory_equal(&iface->macs[0].mac, &ucast1, sizeof(ucast1)); + assert_int_equal(iface->macs[0].refcnt, 2); + assert_true(iface->macs[0].hardware); will_return(__wrap_rte_eth_dev_mac_addr_add, -ENOSPC); will_return(__wrap_rte_eth_promiscuous_enable, 0); assert_return_code(iface_add_eth_addr(iface, &ucast3), errno); assert_true(iface->state & GR_IFACE_S_PROMISC_FIXED); - assert_int_equal(port->filter.count, 3); - assert_memory_equal(&port->filter.macs[2].mac, &ucast3, sizeof(ucast3)); - assert_int_equal(port->filter.macs[2].refcnt, 1); - assert_false(port->filter.macs[2].hardware); + assert_int_equal(vec_len(iface->macs), 3); + assert_memory_equal(&iface->macs[2].mac, &ucast3, sizeof(ucast3)); + assert_int_equal(iface->macs[2].refcnt, 1); + assert_false(iface->macs[2].hardware); } static void port_mac_del_unicast(void **) { - const struct iface_info_port *port = iface_info_port(iface); - assert_return_code(iface_set_promisc(iface, false), errno); assert_true(iface->state & GR_IFACE_S_PROMISC_FIXED); assert_return_code(iface_del_eth_addr(iface, &ucast1), errno); assert_true(iface->state & GR_IFACE_S_PROMISC_FIXED); - assert_memory_equal(&port->filter.macs[0].mac, &ucast1, sizeof(ucast1)); - assert_int_equal(port->filter.macs[0].refcnt, 1); - assert_true(port->filter.macs[0].hardware); + assert_memory_equal(&iface->macs[0].mac, &ucast1, sizeof(ucast1)); + assert_int_equal(iface->macs[0].refcnt, 1); + assert_true(iface->macs[0].hardware); will_return(__wrap_rte_eth_dev_mac_addr_remove, 0); will_return(__wrap_rte_eth_dev_mac_addr_add, 0); will_return(__wrap_rte_eth_promiscuous_disable, 0); assert_return_code(iface_del_eth_addr(iface, &ucast1), errno); assert_false(iface->state & GR_IFACE_S_PROMISC_FIXED); - assert_int_equal(port->filter.count, 2); - assert_memory_equal(&port->filter.macs[0].mac, &ucast2, sizeof(ucast2)); - assert_int_equal(port->filter.macs[0].refcnt, 1); - assert_true(port->filter.macs[0].hardware); - assert_memory_equal(&port->filter.macs[1].mac, &ucast3, sizeof(ucast3)); - assert_int_equal(port->filter.macs[1].refcnt, 1); - assert_true(port->filter.macs[1].hardware); + assert_int_equal(vec_len(iface->macs), 2); + assert_memory_equal(&iface->macs[0].mac, &ucast2, sizeof(ucast2)); + assert_int_equal(iface->macs[0].refcnt, 1); + assert_true(iface->macs[0].hardware); + assert_memory_equal(&iface->macs[1].mac, &ucast3, sizeof(ucast3)); + assert_int_equal(iface->macs[1].refcnt, 1); + assert_true(iface->macs[1].hardware); +} + +static void vlan_mac_add_multicast(void **) { + assert_return_code(iface_add_eth_addr(vlan_iface, &mcast1), errno); + assert_int_equal(vec_len(vlan_iface->macs), 0); +} + +static void vlan_mac_add_unicast(void **) { + assert_int_equal(iface_add_eth_addr(vlan_iface, NULL), -EINVAL); + + assert_return_code(iface_add_eth_addr(vlan_iface, &default_vlan_mac), errno); + assert_int_equal(vec_len(vlan_iface->macs), 0); + + // the underlying port iface still knows ucast2 and ucast3 + will_return(__wrap_rte_eth_dev_mac_addr_add, -ENOSPC); + will_return(__wrap_rte_eth_promiscuous_enable, 0); + will_return(__wrap_iface_from_id, iface); + assert_return_code(iface_add_eth_addr(vlan_iface, &ucast1), errno); + assert_int_equal(vec_len(vlan_iface->macs), 1); + assert_memory_equal(&vlan_iface->macs[0].mac, &ucast1, sizeof(ucast1)); + assert_int_equal(vlan_iface->macs[0].refcnt, 1); + assert_true(iface->state & GR_IFACE_S_PROMISC_FIXED); + assert_int_equal(vec_len(iface->macs), 3); + assert_memory_equal(&iface->macs[2].mac, &ucast1, sizeof(ucast1)); + assert_int_equal(iface->macs[2].refcnt, 1); + assert_false(iface->macs[2].hardware); + + will_return(__wrap_iface_from_id, iface); + assert_return_code(iface_add_eth_addr(vlan_iface, &ucast2), errno); + assert_int_equal(vec_len(vlan_iface->macs), 2); + assert_memory_equal(&vlan_iface->macs[1].mac, &ucast2, sizeof(ucast2)); + assert_int_equal(vlan_iface->macs[1].refcnt, 1); + assert_true(iface->state & GR_IFACE_S_PROMISC_FIXED); + assert_int_equal(vec_len(iface->macs), 3); + assert_memory_equal(&iface->macs[0].mac, &ucast2, sizeof(ucast2)); + assert_int_equal(iface->macs[0].refcnt, 2); + assert_true(iface->macs[0].hardware); + + assert_return_code(iface_add_eth_addr(vlan_iface, &ucast1), errno); + assert_int_equal(vec_len(vlan_iface->macs), 2); + assert_memory_equal(&vlan_iface->macs[0].mac, &ucast1, sizeof(ucast1)); + assert_int_equal(vlan_iface->macs[0].refcnt, 2); + assert_true(iface->state & GR_IFACE_S_PROMISC_FIXED); + assert_int_equal(vec_len(iface->macs), 3); + assert_memory_equal(&iface->macs[2].mac, &ucast1, sizeof(ucast1)); + assert_int_equal(iface->macs[2].refcnt, 1); + assert_false(iface->macs[2].hardware); + + will_return(__wrap_iface_from_id, iface); + assert_return_code(iface_add_eth_addr(vlan_iface, &ucast3), errno); + assert_int_equal(vec_len(vlan_iface->macs), 3); + assert_memory_equal(&vlan_iface->macs[2].mac, &ucast3, sizeof(ucast3)); + assert_int_equal(vlan_iface->macs[2].refcnt, 1); + assert_true(iface->state & GR_IFACE_S_PROMISC_FIXED); + assert_int_equal(vec_len(iface->macs), 3); + assert_memory_equal(&iface->macs[1].mac, &ucast3, sizeof(ucast3)); + assert_int_equal(iface->macs[1].refcnt, 2); + assert_true(iface->macs[1].hardware); +} + +static void vlan_mac_del_unicast(void **) { + assert_return_code(iface_del_eth_addr(vlan_iface, &ucast1), errno); + assert_memory_equal(&vlan_iface->macs[0].mac, &ucast1, sizeof(ucast1)); + assert_int_equal(vlan_iface->macs[0].refcnt, 1); + assert_true(iface->state & GR_IFACE_S_PROMISC_FIXED); + assert_int_equal(vec_len(iface->macs), 3); + assert_memory_equal(&iface->macs[2].mac, &ucast1, sizeof(ucast1)); + assert_int_equal(iface->macs[2].refcnt, 1); + assert_false(iface->macs[2].hardware); + + will_return(__wrap_iface_from_id, iface); + will_return(__wrap_rte_eth_promiscuous_disable, 0); + assert_return_code(iface_del_eth_addr(vlan_iface, &ucast1), errno); + assert_int_equal(vec_len(vlan_iface->macs), 2); + assert_memory_equal(&vlan_iface->macs[0].mac, &ucast2, sizeof(ucast2)); + assert_int_equal(vlan_iface->macs[0].refcnt, 1); + assert_memory_equal(&vlan_iface->macs[1].mac, &ucast3, sizeof(ucast3)); + assert_int_equal(vlan_iface->macs[1].refcnt, 1); + assert_int_equal(vec_len(iface->macs), 2); + assert_memory_equal(&iface->macs[0].mac, &ucast2, sizeof(ucast2)); + assert_int_equal(iface->macs[0].refcnt, 2); + assert_memory_equal(&iface->macs[1].mac, &ucast3, sizeof(ucast3)); + assert_int_equal(iface->macs[1].refcnt, 2); } int main(void) { @@ -168,6 +265,9 @@ int main(void) { cmocka_unit_test(port_mac_add_multicast), cmocka_unit_test(port_mac_add_unicast), cmocka_unit_test(port_mac_del_unicast), + cmocka_unit_test(vlan_mac_add_multicast), + cmocka_unit_test(vlan_mac_add_unicast), + cmocka_unit_test(vlan_mac_del_unicast), }; return cmocka_run_group_tests(tests, setup, teardown); } diff --git a/modules/infra/control/meson.build b/modules/infra/control/meson.build index c3150b78a..4870ae4cf 100644 --- a/modules/infra/control/meson.build +++ b/modules/infra/control/meson.build @@ -48,8 +48,9 @@ tests += [ ], }, { - 'sources': files('iface_test.c', 'iface.c', 'port.c'), + 'sources': files('iface_test.c', 'iface.c', 'port.c', 'vlan.c'), 'link_args': [ + '-Wl,--wrap=iface_from_id', '-Wl,--wrap=rte_eth_allmulticast_enable', '-Wl,--wrap=rte_eth_dev_mac_addr_add', '-Wl,--wrap=rte_eth_dev_mac_addr_remove', diff --git a/modules/infra/control/port.c b/modules/infra/control/port.c index a0b6e49cb..a9c1e90b9 100644 --- a/modules/infra/control/port.c +++ b/modules/infra/control/port.c @@ -653,37 +653,11 @@ static int port_mac_get(const struct iface *iface, struct rte_ether_addr *mac) { return 0; } -static int port_mac_add(struct iface *iface, const struct rte_ether_addr *mac) { +static int port_mac_add(struct iface *iface, struct iface_mac *m) { struct iface_info_port *port = iface_info_port(iface); - struct port_mac *m; int ret; - for (unsigned i = 0; i < port->filter.count; i++) { - m = &port->filter.macs[i]; - if (rte_is_same_ether_addr(&m->mac, mac)) { - if (m->refcnt == UINT_NUM_VALUES(m->refcnt) - 1) - return errno_set(EOVERFLOW); - - LOG(DEBUG, - "%s: mac " ETH_F " already filtered (refs=%u)", - iface->name, - mac, - m->refcnt); - m->refcnt++; - return 0; - } - } - if (port->filter.count >= ARRAY_DIM(port->filter.macs)) - return errno_set(ENOSPC); - - m = &port->filter.macs[port->filter.count++]; - m->refcnt = 1; - m->hardware = false; - m->mac = *mac; - - LOG(INFO, "%s: enabling " ETH_F " mac filter", iface->name, mac); - - ret = rte_eth_dev_mac_addr_add(port->port_id, (struct rte_ether_addr *)mac, 0); + ret = rte_eth_dev_mac_addr_add(port->port_id, &m->mac, 0); if (ret == 0) { m->hardware = true; } else if (ret == -ENOSPC || ret == -EOPNOTSUPP) { @@ -704,70 +678,35 @@ static int port_mac_add(struct iface *iface, const struct rte_ether_addr *mac) { } } - if (ret < 0) { - port->filter.count--; + if (ret < 0) return errno_set(-ret); - } return 0; } -static int port_mac_del(struct iface *iface, const struct rte_ether_addr *mac) { +static int port_mac_del(struct iface *iface, struct iface_mac *m) { struct iface_info_port *port = iface_info_port(iface); - struct port_mac *m; - uint8_t i; int ret; - for (i = 0; i < port->filter.count; i++) { - m = &port->filter.macs[i]; - if (rte_is_same_ether_addr(&m->mac, mac)) - goto found; - } - return errno_set(ENOENT); - -found: - if (m->refcnt == 0) - return errno_set(EOVERFLOW); - - if (--m->refcnt > 0) { - LOG(DEBUG, - "%s: mac " ETH_F " still filtered (refs=%u)", - iface->name, - mac, - m->refcnt); - return 0; - } - - LOG(INFO, "%s: removing " ETH_F " mac filter", iface->name, mac); - if (m->hardware) { - ret = rte_eth_dev_mac_addr_remove(port->port_id, (struct rte_ether_addr *)mac); + ret = rte_eth_dev_mac_addr_remove(port->port_id, &m->mac); if (ret < 0 && ret != -ENOTSUP) LOG(WARNING, "%s: %s", iface->name, rte_strerror(-ret)); } - if (port->filter.count > 1 && i < port->filter.count - 1) { - memmove(&port->filter.macs[i], - &port->filter.macs[i + 1], - (port->filter.count - 1 - i) * sizeof(port->filter.macs[0])); - } - port->filter.count--; - if (iface->state & GR_IFACE_S_PROMISC_FIXED) { bool disable_promisc = true; - LOG(INFO, "i=%d, port->filter.count=%d", i, port->filter.count); - for (; i < port->filter.count; i++) { - m = &port->filter.macs[i]; - if (m->hardware) + vec_foreach_ref (struct iface_mac *m2, iface->macs) { + if (m == m2 || m2->hardware) continue; - ret = rte_eth_dev_mac_addr_add(port->port_id, &m->mac, 0); + ret = rte_eth_dev_mac_addr_add(port->port_id, &m2->mac, 0); if (ret < 0) { LOG(INFO, "%s: %s", iface->name, rte_strerror(-ret)); disable_promisc = false; break; } - m->hardware = true; + m2->hardware = true; } if (disable_promisc) { diff --git a/modules/infra/control/port.h b/modules/infra/control/port.h index 5190c0feb..a82ee4156 100644 --- a/modules/infra/control/port.h +++ b/modules/infra/control/port.h @@ -14,12 +14,6 @@ #include -struct port_mac { - uint8_t refcnt; - bool hardware; - struct rte_ether_addr mac; -}; - GR_IFACE_INFO(GR_IFACE_TYPE_PORT, iface_info_port, { BASE(__gr_iface_info_port_base); @@ -33,10 +27,6 @@ GR_IFACE_INFO(GR_IFACE_TYPE_PORT, iface_info_port, { bool virtio_offloads; uint64_t rx_offloads; rte_spinlock_t txq_locks[RTE_MAX_QUEUES_PER_PORT]; - struct { - unsigned count; - struct port_mac macs[RTE_ETH_NUM_RECEIVE_MAC_ADDR]; - } filter; }); const struct iface *port_get_iface(uint16_t port_id); diff --git a/modules/infra/control/vlan.c b/modules/infra/control/vlan.c index 257a6841c..ed054da6a 100644 --- a/modules/infra/control/vlan.c +++ b/modules/infra/control/vlan.c @@ -200,18 +200,18 @@ static int iface_vlan_set_eth_addr(struct iface *iface, const struct rte_ether_a return 0; } -static int iface_vlan_add_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) { +static int iface_vlan_add_eth_addr(struct iface *iface, struct iface_mac *m) { const struct iface_info_vlan *vlan = iface_info_vlan(iface); struct iface *parent = iface_from_id(vlan->parent_id); - return iface_add_eth_addr(parent, mac); + return iface_add_eth_addr(parent, &m->mac); } -static int iface_vlan_del_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) { +static int iface_vlan_del_eth_addr(struct iface *iface, struct iface_mac *m) { const struct iface_info_vlan *vlan = iface_info_vlan(iface); struct iface *parent = iface_from_id(vlan->parent_id); - return iface_del_eth_addr(parent, mac); + return iface_del_eth_addr(parent, &m->mac); } static void vlan_to_api(void *info, const struct iface *iface) { From b7b21464af1c3c0565dedac1a3519b442eb9c597 Mon Sep 17 00:00:00 2001 From: David Marchand Date: Tue, 21 Apr 2026 14:37:57 +0200 Subject: [PATCH 7/9] infra: add mac address management commands Introduce a new top-level "mac" CLI context for managing both primary and secondary MAC addresses on interfaces. The primary MAC address can be changed with "mac set MAC iface IFACE". Secondary MAC addresses can be added and removed with "mac add" and "mac del" commands. The "mac show" command displays all MAC addresses (both primary and secondary) with their reference counts. The primary MAC is always shown first with a "-" in the REFCNT column to distinguish it from secondary addresses. This provides a unified interface for MAC address operations instead of having them scattered under different command contexts. This also helps a lot when troubleshooting Rx filters and other fun jokes about mac addresses. Signed-off-by: David Marchand --- modules/infra/api/gr_infra.h | 42 +++++++++ modules/infra/api/iface.c | 85 ++++++++++++++++++ modules/infra/cli/mac.c | 162 ++++++++++++++++++++++++++++++++++ modules/infra/cli/meson.build | 1 + smoke/iface_mac_test.sh | 126 ++++++++++++++++++++++++++ 5 files changed, 416 insertions(+) create mode 100644 modules/infra/cli/mac.c create mode 100755 smoke/iface_mac_test.sh diff --git a/modules/infra/api/gr_infra.h b/modules/infra/api/gr_infra.h index ee1bb861e..f0895d65a 100644 --- a/modules/infra/api/gr_infra.h +++ b/modules/infra/api/gr_infra.h @@ -248,6 +248,10 @@ enum gr_infra_requests : uint32_t { GR_PACKET_TRACE_SET, GR_AFFINITY_CPU_GET, GR_AFFINITY_CPU_SET, + GR_IFACE_MAC_ADD, + GR_IFACE_MAC_DEL, + GR_IFACE_MAC_LIST, + GR_IFACE_MAC_SET, }; enum gr_infra_events : uint32_t { @@ -312,6 +316,44 @@ struct gr_iface_list_req { GR_REQ_STREAM(GR_IFACE_LIST, struct gr_iface_list_req, struct gr_iface); +// Add a secondary MAC address to an interface. +struct gr_iface_mac_add_req { + uint16_t iface_id; + struct rte_ether_addr mac; +}; + +GR_REQ(GR_IFACE_MAC_ADD, struct gr_iface_mac_add_req, struct gr_empty); + +// Remove a secondary MAC address from an interface. +struct gr_iface_mac_del_req { + uint16_t iface_id; + struct rte_ether_addr mac; +}; + +GR_REQ(GR_IFACE_MAC_DEL, struct gr_iface_mac_del_req, struct gr_empty); + +// List MAC addresses on an interface. +struct gr_iface_mac_list_req { + uint16_t iface_id; // GR_IFACE_ID_UNDEF for all interfaces. +}; + +struct gr_iface_mac { + uint16_t iface_id; + uint16_t refcnt; + bool primary; + struct rte_ether_addr mac; +}; + +GR_REQ_STREAM(GR_IFACE_MAC_LIST, struct gr_iface_mac_list_req, struct gr_iface_mac); + +// Set the primary MAC address of an interface. +struct gr_iface_mac_set_req { + uint16_t iface_id; + struct rte_ether_addr mac; +}; + +GR_REQ(GR_IFACE_MAC_SET, struct gr_iface_mac_set_req, struct gr_empty); + // Modify an existing interface. // MTU changes on parent interfaces propagate to VLAN sub-interfaces. struct gr_iface_set_req { diff --git a/modules/infra/api/iface.c b/modules/infra/api/iface.c index 45e27bac2..9d25f4e69 100644 --- a/modules/infra/api/iface.c +++ b/modules/infra/api/iface.c @@ -109,6 +109,87 @@ static struct api_out iface_list(const void *request, struct api_ctx *ctx) { return api_out(ret, 0, NULL); } +static struct api_out iface_mac_add(const void *request, struct api_ctx *) { + const struct gr_iface_mac_add_req *req = request; + struct iface *iface; + int ret; + + if ((iface = iface_from_id(req->iface_id)) == NULL) + return api_out(ENODEV, 0, NULL); + + ret = iface_add_eth_addr(iface, &req->mac); + if (ret < 0) + return api_out(errno, 0, NULL); + + return api_out(0, 0, NULL); +} + +static struct api_out iface_mac_del(const void *request, struct api_ctx *) { + const struct gr_iface_mac_del_req *req = request; + struct iface *iface; + int ret; + + if ((iface = iface_from_id(req->iface_id)) == NULL) + return api_out(ENODEV, 0, NULL); + + ret = iface_del_eth_addr(iface, &req->mac); + if (ret < 0) + return api_out(errno, 0, NULL); + + return api_out(0, 0, NULL); +} + +static struct api_out iface_mac_list(const void *request, struct api_ctx *ctx) { + const struct gr_iface_mac_list_req *req = request; + const struct iface *iface = NULL; + int ret = 0; + + while ((iface = iface_next(GR_IFACE_TYPE_UNDEF, iface)) != NULL) { + if (req->iface_id != GR_IFACE_ID_UNDEF && iface->id != req->iface_id) + continue; + + // Send primary MAC address first + struct rte_ether_addr primary_mac; + if (iface_get_eth_addr(iface, &primary_mac) == 0) { + struct gr_iface_mac mac; + + mac.iface_id = iface->id; + mac.refcnt = 0; + mac.primary = true; + mac.mac = primary_mac; + api_send(ctx, sizeof(mac), &mac); + } + + // Send secondary MAC addresses + vec_foreach_ref (struct iface_mac *m, iface->macs) { + struct gr_iface_mac mac; + + mac.iface_id = iface->id; + mac.refcnt = m->refcnt; + mac.primary = false; + mac.mac = m->mac; + api_send(ctx, sizeof(mac), &mac); + } + } + + return api_out(ret, 0, NULL); +} + +static struct api_out iface_mac_set(const void *request, struct api_ctx *) { + const struct gr_iface_mac_set_req *req = request; + struct iface *iface; + int ret; + + if ((iface = iface_from_id(req->iface_id)) == NULL) + return api_out(ENODEV, 0, NULL); + + ret = iface_set_eth_addr(iface, &req->mac); + if (ret < 0) + return api_out(errno, 0, NULL); + + return api_out(0, 0, NULL); +} + static struct api_out iface_set(const void *request, struct api_ctx *) { const struct gr_iface_set_req *req = request; int ret; @@ -232,6 +313,10 @@ RTE_INIT(infra_api_init) { api_handler(GR_IFACE_DEL, iface_del); api_handler(GR_IFACE_GET, iface_get); api_handler(GR_IFACE_LIST, iface_list); + api_handler(GR_IFACE_MAC_ADD, iface_mac_add); + api_handler(GR_IFACE_MAC_DEL, iface_mac_del); + api_handler(GR_IFACE_MAC_LIST, iface_mac_list); + api_handler(GR_IFACE_MAC_SET, iface_mac_set); api_handler(GR_IFACE_SET, iface_set); event_serializer(GR_EVENT_IFACE_ADD, iface_event_serialize); event_serializer(GR_EVENT_IFACE_POST_ADD, iface_event_serialize); diff --git a/modules/infra/cli/mac.c b/modules/infra/cli/mac.c new file mode 100644 index 000000000..0f0467f50 --- /dev/null +++ b/modules/infra/cli/mac.c @@ -0,0 +1,162 @@ +// SPDX-License-Identifier: BSD-3-Clause +// Copyright (c) 2026 David Marchand + +#include "cli.h" +#include "cli_iface.h" +#include "display.h" + +#include +#include +#include + +#include + +#include + +static cmd_status_t mac_add(struct gr_api_client *c, const struct ec_pnode *p) { + struct gr_iface_mac_add_req req = {0}; + + if (arg_iface(c, p, "IFACE", GR_IFACE_TYPE_UNDEF, &req.iface_id) < 0) + return CMD_ERROR; + + if (arg_eth_addr(p, "MAC", &req.mac) < 0) + return CMD_ERROR; + + if (gr_api_client_send_recv(c, GR_IFACE_MAC_ADD, sizeof(req), &req, NULL) < 0) + return CMD_ERROR; + + return CMD_SUCCESS; +} + +static cmd_status_t mac_del(struct gr_api_client *c, const struct ec_pnode *p) { + struct gr_iface_mac_del_req req = {0}; + + if (arg_iface(c, p, "IFACE", GR_IFACE_TYPE_UNDEF, &req.iface_id) < 0) + return CMD_ERROR; + + if (arg_eth_addr(p, "MAC", &req.mac) < 0) + return CMD_ERROR; + + if (gr_api_client_send_recv(c, GR_IFACE_MAC_DEL, sizeof(req), &req, NULL) < 0) + return CMD_ERROR; + + return CMD_SUCCESS; +} + +static cmd_status_t mac_list(struct gr_api_client *c, const struct ec_pnode *p) { + struct gr_iface_mac_list_req req = {.iface_id = GR_IFACE_ID_UNDEF}; + const struct gr_iface_mac *mac; + int ret; + + if (arg_str(p, "IFACE") != NULL) { + if (arg_iface(c, p, "IFACE", GR_IFACE_TYPE_UNDEF, &req.iface_id) < 0) + return CMD_ERROR; + } + + struct gr_table *table = gr_table_new(); + gr_table_column(table, "IFACE", GR_DISP_LEFT); + gr_table_column(table, "MAC", GR_DISP_LEFT); + gr_table_column(table, "REFCNT", GR_DISP_RIGHT | GR_DISP_INT); + + gr_api_client_stream_foreach (mac, ret, c, GR_IFACE_MAC_LIST, sizeof(req), &req) { + gr_table_cell(table, 0, "%s", iface_name_from_id(c, mac->iface_id)); + gr_table_cell(table, 1, ETH_F, &mac->mac); + if (mac->primary) + gr_table_cell(table, 2, "-"); + else + gr_table_cell(table, 2, "%u", mac->refcnt); + if (gr_table_print_row(table) < 0) + break; + } + + gr_table_free(table); + + return ret < 0 ? CMD_ERROR : CMD_SUCCESS; +} + +static cmd_status_t mac_set(struct gr_api_client *c, const struct ec_pnode *p) { + struct gr_iface_mac_set_req req = {0}; + + if (arg_iface(c, p, "IFACE", GR_IFACE_TYPE_UNDEF, &req.iface_id) < 0) + return CMD_ERROR; + + if (arg_eth_addr(p, "MAC", &req.mac) < 0) + return CMD_ERROR; + + if (gr_api_client_send_recv(c, GR_IFACE_MAC_SET, sizeof(req), &req, NULL) < 0) + return CMD_ERROR; + + return CMD_SUCCESS; +} + +#define MAC_CTX(root) CLI_CONTEXT(root, CTX_ARG("mac", "MAC address management.")) + +static int ctx_init(struct ec_node *root) { + int ret; + + ret = CLI_COMMAND( + MAC_CTX(root), + "add MAC iface IFACE", + mac_add, + "Add a secondary MAC address to an interface.", + with_help("MAC address.", ec_node_re("MAC", ETH_ADDR_RE)), + with_help( + "Interface name.", + ec_node_dyn("IFACE", complete_iface_names, INT2PTR(GR_IFACE_TYPE_UNDEF)) + ) + ); + if (ret < 0) + return ret; + + ret = CLI_COMMAND( + MAC_CTX(root), + "del MAC iface IFACE", + mac_del, + "Remove a secondary MAC address from an interface.", + with_help("MAC address.", ec_node_re("MAC", ETH_ADDR_RE)), + with_help( + "Interface name.", + ec_node_dyn("IFACE", complete_iface_names, INT2PTR(GR_IFACE_TYPE_UNDEF)) + ) + ); + if (ret < 0) + return ret; + + ret = CLI_COMMAND( + MAC_CTX(root), + "set MAC iface IFACE", + mac_set, + "Set the primary MAC address of an interface.", + with_help("MAC address.", ec_node_re("MAC", ETH_ADDR_RE)), + with_help( + "Interface name.", + ec_node_dyn("IFACE", complete_iface_names, INT2PTR(GR_IFACE_TYPE_UNDEF)) + ) + ); + if (ret < 0) + return ret; + + ret = CLI_COMMAND( + MAC_CTX(root), + "[show] [iface IFACE]", + mac_list, + "Display MAC addresses.", + with_help( + "Interface name.", + ec_node_dyn("IFACE", complete_iface_names, INT2PTR(GR_IFACE_TYPE_UNDEF)) + ) + ); + if (ret < 0) + return ret; + + return 0; +} + +static struct cli_context ctx = { + .name = "mac", + .init = ctx_init, +}; + +static void __attribute__((constructor, used)) init(void) { + cli_context_register(&ctx); +} diff --git a/modules/infra/cli/meson.build b/modules/infra/cli/meson.build index 911031c36..222f72196 100644 --- a/modules/infra/cli/meson.build +++ b/modules/infra/cli/meson.build @@ -9,6 +9,7 @@ cli_src += files( 'graph.c', 'icmp.c', 'iface.c', + 'mac.c', 'vrf.c', 'nexthop.c', 'port.c', diff --git a/smoke/iface_mac_test.sh b/smoke/iface_mac_test.sh new file mode 100755 index 000000000..2591e6973 --- /dev/null +++ b/smoke/iface_mac_test.sh @@ -0,0 +1,126 @@ +#!/bin/bash +# SPDX-License-Identifier: BSD-3-Clause +# Copyright (c) 2026 David Marchand + +. $(dirname $0)/_init.sh + +# Create a test port +grcli interface add port p0 devargs net_tap0,iface=x-p0 + +# Test adding secondary MAC addresses +mac1="02:00:00:00:00:01" +mac2="02:00:00:00:00:02" +mac3="02:00:00:00:00:03" + +echo "Adding secondary MAC addresses..." +grcli mac add "$mac1" iface p0 +grcli mac add "$mac2" iface p0 +grcli mac add "$mac3" iface p0 + +# List all MAC addresses +echo "Listing MAC addresses on p0:" +grcli mac show iface p0 + +# Verify MAC addresses are present (3 secondary + 1 primary = 4 total) +count=$(grcli -j mac show iface p0 | jq 'length') +if [ "$count" -ne 4 ]; then + fail "Expected 4 MACs (1 primary + 3 secondary), found $count" +fi + +# Verify specific MAC is present with refcnt 1 +refcnt=$(grcli -j mac show iface p0 | jq -r --arg mac "$mac1" '.[] | select(.mac == $mac) | .refcnt') +if [ "$refcnt" -ne 1 ]; then + fail "MAC $mac1 should have refcnt 1, found $refcnt" +fi + +# Test deleting a MAC address +echo "Deleting MAC $mac2..." +grcli mac del "$mac2" iface p0 + +# Verify MAC was deleted (1 primary + 2 secondary = 3 total) +count=$(grcli -j mac show iface p0 | jq 'length') +if [ "$count" -ne 3 ]; then + fail "Expected 3 MACs (1 primary + 2 secondary) after deletion, found $count" +fi + +# Verify deleted MAC is gone +if grcli -j mac show iface p0 | jq -e --arg mac "$mac2" '.[] | select(.mac == $mac)' 2>/dev/null; then + fail "MAC $mac2 should have been deleted" +fi + +# Test adding duplicate MAC (should succeed, refcount should increase) +echo "Adding duplicate MAC $mac1..." +grcli mac add "$mac1" iface p0 + +# Still should have 3 entries (1 primary + 2 secondary, refcount increased) +count=$(grcli -j mac show iface p0 | jq 'length') +if [ "$count" -ne 3 ]; then + fail "Expected 3 MACs (1 primary + 2 secondary) after duplicate add, found $count" +fi + +# Verify refcnt increased to 2 +refcnt=$(grcli -j mac show iface p0 | jq -r --arg mac "$mac1" '.[] | select(.mac == $mac) | .refcnt') +if [ "$refcnt" -ne 2 ]; then + fail "MAC $mac1 should have refcnt 2 after duplicate add, found $refcnt" +fi + +# Delete once, refcnt should decrease to 1 +echo "Deleting MAC $mac1 (first time)..." +grcli mac del "$mac1" iface p0 + +count=$(grcli -j mac show iface p0 | jq 'length') +if [ "$count" -ne 3 ]; then + fail "Expected 3 MACs (1 primary + 2 secondary) after first delete, found $count" +fi + +refcnt=$(grcli -j mac show iface p0 | jq -r --arg mac "$mac1" '.[] | select(.mac == $mac) | .refcnt') +if [ "$refcnt" -ne 1 ]; then + fail "MAC $mac1 should have refcnt 1 after first delete, found $refcnt" +fi + +# Delete again, MAC should be removed +echo "Deleting MAC $mac1 (second time)..." +grcli mac del "$mac1" iface p0 + +count=$(grcli -j mac show iface p0 | jq 'length') +if [ "$count" -ne 2 ]; then + fail "Expected 2 MACs (1 primary + 1 secondary) after second delete, found $count" +fi + +# Verify mac1 is completely gone +if grcli -j mac show iface p0 | jq -e --arg mac "$mac1" '.[] | select(.mac == $mac)' 2>/dev/null; then + fail "MAC $mac1 should have been completely removed" +fi + +# Create VLAN interface to test MAC inheritance +echo "Creating VLAN interface..." +grcli interface add vlan p0.100 parent p0 vlan 100 + +# Add MAC to VLAN, should also be added to parent port +mac4="02:00:00:00:00:04" +grcli mac add "$mac4" iface p0.100 + +# Verify MAC is on VLAN +grcli -j mac show iface p0.100 | jq -e --arg mac "$mac4" '.[] | select(.mac == $mac)' \ + || fail "MAC $mac4 not found on VLAN" + +# List all MACs across all interfaces +echo "Listing all MAC addresses:" +grcli mac show + +# Test error handling: try to add invalid MAC +if grcli mac add "invalid-mac" iface p0 2>/dev/null; then + fail "Adding invalid MAC should have failed" +fi + +# Test error handling: try to add MAC to non-existent interface +if grcli mac add "$mac1" iface nonexistent 2>/dev/null; then + fail "Adding MAC to non-existent interface should have failed" +fi + +# Test error handling: try to delete non-existent MAC +if grcli mac del "02:00:00:00:00:99" iface p0 2>/dev/null; then + fail "Deleting non-existent MAC should have failed" +fi + +echo "All MAC address tests passed" From bb66cbc17e9fcb5879538669a07943d8ae495a20 Mon Sep 17 00:00:00 2001 From: David Marchand Date: Tue, 14 Apr 2026 10:34:35 +0200 Subject: [PATCH 8/9] vlan: add promiscuous support When adding a vlan interface in a bridge, pass on the promiscuous configuration to the parent port. Signed-off-by: David Marchand --- modules/infra/control/vlan.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/modules/infra/control/vlan.c b/modules/infra/control/vlan.c index ed054da6a..a44d3d233 100644 --- a/modules/infra/control/vlan.c +++ b/modules/infra/control/vlan.c @@ -214,6 +214,13 @@ static int iface_vlan_del_eth_addr(struct iface *iface, struct iface_mac *m) { return iface_del_eth_addr(parent, &m->mac); } +static int iface_vlan_promisc_set(struct iface *iface, bool enabled) { + const struct iface_info_vlan *vlan = iface_info_vlan(iface); + struct iface *parent = iface_from_id(vlan->parent_id); + + return iface_set_promisc(parent, enabled); +} + static void vlan_to_api(void *info, const struct iface *iface) { const struct iface_info_vlan *vlan = iface_info_vlan(iface); struct gr_iface_info_vlan *api = info; @@ -233,6 +240,7 @@ static const struct iface_type iface_type_vlan = { .set_eth_addr = iface_vlan_set_eth_addr, .add_eth_addr = iface_vlan_add_eth_addr, .del_eth_addr = iface_vlan_del_eth_addr, + .set_promisc = iface_vlan_promisc_set, .to_api = vlan_to_api, }; From 715a11bf61dac2c9db1cf0b5a0666cbfbcb83060 Mon Sep 17 00:00:00 2001 From: David Marchand Date: Wed, 25 Mar 2026 16:09:10 +0100 Subject: [PATCH 9/9] l2: push external mac addresses in hardware In setups where grout physical port is a VF, traffic sent by other VFs reach grout only if the destination address is multicast or a mac address for which a rx filter was declared in hw. Make use of control path pushed FDB entries to populate hardware and let the packets flow. Signed-off-by: David Marchand --- modules/l2/control/bridge.c | 6 ++++ modules/l2/control/fdb.c | 63 +++++++++++++++++++++++++++++++++++++ modules/l2/control/l2.h | 3 ++ 3 files changed, 72 insertions(+) diff --git a/modules/l2/control/bridge.c b/modules/l2/control/bridge.c index 9223a4efa..90cd4995f 100644 --- a/modules/l2/control/bridge.c +++ b/modules/l2/control/bridge.c @@ -58,6 +58,8 @@ static int bridge_attach_member(struct iface *bridge, struct iface *member) { member->vrf_id = GR_VRF_ID_UNDEF; member->mode = GR_IFACE_MODE_BRIDGE; + fdb_sync_hardware(bridge, member, true); + return 0; } @@ -67,6 +69,9 @@ static int bridge_detach_member(struct iface *bridge, struct iface *member) { for (unsigned i = 0; i < br->n_members; i++) { if (br->members[i] == member) { unsigned last = br->n_members - 1; + + fdb_sync_hardware(bridge, member, false); + if (i < last) br->members[i] = br->members[last]; br->n_members--; @@ -86,6 +91,7 @@ static int bridge_fini(struct iface *iface) { for (unsigned i = 0; i < bridge->n_members; i++) { struct iface *member = bridge->members[i]; + fdb_sync_hardware(iface, member, false); iface_set_promisc(member, false); member->vrf_id = vrf_default_get_or_create(); if (member->vrf_id != GR_VRF_ID_UNDEF) diff --git a/modules/l2/control/fdb.c b/modules/l2/control/fdb.c index 7d3edc370..85a86347a 100644 --- a/modules/l2/control/fdb.c +++ b/modules/l2/control/fdb.c @@ -332,6 +332,67 @@ static struct api_out fdb_config_set(const void *request, struct api_ctx *) { return api_out(0, 0, NULL); } +static void push_mac_to_hw(struct iface *iface, const struct rte_ether_addr *mac, bool add) { + int ret; + + if (add) + ret = iface_add_eth_addr(iface, mac); + else + ret = iface_del_eth_addr(iface, mac); + if (ret < 0) { + LOG(DEBUG, + "failed to %s mac " ETH_F " to %s: %s", + add ? "add" : "del", + mac, + iface->name, + strerror(errno)); + } +} + +static void fdb_event_cb(uint32_t event, const void *obj) { + const struct iface_info_bridge *bridge_info; + const struct gr_fdb_entry *fdb = obj; + const struct iface *bridge; + + bridge = iface_from_id(fdb->bridge_id); + if (bridge == NULL) { + LOG(ERR, "unknown bridge %u", fdb->bridge_id); + return; + } + + if ((fdb->flags & GR_FDB_F_EXTERN) == 0) + return; + + bridge_info = iface_info_bridge(bridge); + for (unsigned i = 0; i < bridge_info->n_members; i++) { + struct iface *member = bridge_info->members[i]; + + // we have no clear idea what to do with a vlan_id if one got pushed by FRR + assert(fdb->vlan_id == 0); + // FIXME: ideally, we should ask for adding the mac in hw only when getting the + // *first* transition from !EXTERN to EXTERN to handle both learning mac on the + // wire and from FRR. + push_mac_to_hw(member, &fdb->mac, event != GR_EVENT_FDB_DEL); + } +} + +void fdb_sync_hardware(const struct iface *bridge, struct iface *member, bool add) { + struct gr_fdb_entry *fdb; + uint32_t next = 0; + const void *key; + void *data; + + while (rte_hash_iterate(fdb_hash, &key, &data, &next) >= 0) { + if (!fdb_match(data, GR_FDB_F_EXTERN, bridge->id, GR_IFACE_ID_UNDEF, NULL)) + continue; + + fdb = data; + // we have no clear idea what to do with a vlan_id if one got pushed by FRR + assert(fdb->vlan_id == 0); + push_mac_to_hw(member, &fdb->mac, add); + } +} + static void fdb_ageing_cb(evutil_socket_t, short /*what*/, void * /*priv*/) { const struct iface *bridge; struct gr_fdb_entry *fdb; @@ -409,6 +470,8 @@ RTE_INIT(init) { api_handler(GR_FDB_LIST, fdb_list); api_handler(GR_FDB_CONFIG_GET, fdb_config_get); api_handler(GR_FDB_CONFIG_SET, fdb_config_set); + event_subscribe(GR_EVENT_FDB_ADD, fdb_event_cb); + event_subscribe(GR_EVENT_FDB_DEL, fdb_event_cb); event_serializer(GR_EVENT_FDB_ADD, NULL); event_serializer(GR_EVENT_FDB_DEL, NULL); event_serializer(GR_EVENT_FDB_UPDATE, NULL); diff --git a/modules/l2/control/l2.h b/modules/l2/control/l2.h index 9b37c581c..82f137db2 100644 --- a/modules/l2/control/l2.h +++ b/modules/l2/control/l2.h @@ -39,6 +39,9 @@ void fdb_learn( // Delete all FDB entries referencing the provided interface. void fdb_purge_iface(uint16_t iface_id); +// Push or remove mac addresses from HW Rx filters. +void fdb_sync_hardware(const struct iface *bridge, struct iface *member, bool add); + // Delete all FDB entries referencing the provided bridge. void fdb_purge_bridge(uint16_t bridge_id);