Skip to content

infra: send GARP and NA on MAC address change #473

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rjarry wants to merge 1 commit into
base: main
Choose a base branch
from
Open

infra: send GARP and NA on MAC address change #473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions modules/infra/api/gr_infra.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ typedef enum {
GR_EVENT_IFACE_POST_RECONFIG = EVENT_TYPE(GR_INFRA_MODULE, 0x0005),
GR_EVENT_IFACE_STATUS_UP = EVENT_TYPE(GR_INFRA_MODULE, 0x0006),
GR_EVENT_IFACE_STATUS_DOWN = EVENT_TYPE(GR_INFRA_MODULE, 0x0007),
GR_EVENT_IFACE_MAC_CHANGE = EVENT_TYPE(GR_INFRA_MODULE, 0x0008),
} gr_event_iface_t;

// interface management ///////////////////////////////////////////////////////
Expand Down
3 changes: 2 additions & 1 deletion modules/infra/api/iface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ static int iface_event_serialize(const void *obj, void **buf) {

static struct gr_event_serializer iface_serializer = {
.callback = iface_event_serialize,
.ev_count = 7,
.ev_count = 8,
.ev_types = {
GR_EVENT_IFACE_ADD,
GR_EVENT_IFACE_POST_ADD,
Expand All @@ -172,6 +172,7 @@ static struct gr_event_serializer iface_serializer = {
GR_EVENT_IFACE_POST_RECONFIG,
GR_EVENT_IFACE_STATUS_UP,
GR_EVENT_IFACE_STATUS_DOWN,
GR_EVENT_IFACE_MAC_CHANGE,
},
};

Expand Down
6 changes: 5 additions & 1 deletion modules/infra/cli/iface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,9 @@ static void iface_event_print(uint32_t event, const void *obj) {
case GR_EVENT_IFACE_POST_RECONFIG:
action = "reconf";
break;
case GR_EVENT_IFACE_MAC_CHANGE:
action = "mac change";
break;
default:
action = "?";
break;
Expand All @@ -572,7 +575,7 @@ static void iface_event_print(uint32_t event, const void *obj) {

static struct cli_event_printer printer = {
.print = iface_event_print,
.ev_count = 7,
.ev_count = 8,
.ev_types = {
GR_EVENT_IFACE_ADD,
GR_EVENT_IFACE_POST_ADD,
Expand All @@ -581,6 +584,7 @@ static struct cli_event_printer printer = {
GR_EVENT_IFACE_STATUS_UP,
GR_EVENT_IFACE_STATUS_DOWN,
GR_EVENT_IFACE_POST_RECONFIG,
GR_EVENT_IFACE_MAC_CHANGE,
},
};

Expand Down
3 changes: 1 addition & 2 deletions modules/infra/control/bond.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,9 +362,8 @@ static int bond_reconfig(
} else {
mac = api->mac;
}
if (bond_mac_set(iface, &mac) < 0)
if (iface_set_eth_addr(iface, &mac) < 0)
return errno_set(errno);
bond->mac = mac;
}

if (set_attrs & (GR_BOND_SET_MEMBERS | GR_BOND_SET_PRIMARY))
Expand Down
15 changes: 13 additions & 2 deletions modules/infra/control/iface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,7 @@ void iface_del_subinterface(struct iface *parent, struct iface *sub) {

int iface_set_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) {
const struct iface_type *type;
int ret;

if (iface == NULL)
return errno_set(EINVAL);
Expand All @@ -304,7 +305,11 @@ int iface_set_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) {
if (type->set_eth_addr == NULL)
return errno_set(EOPNOTSUPP);

return type->set_eth_addr(iface, mac);
ret = type->set_eth_addr(iface, mac);
if (ret == 0)
gr_event_push(GR_EVENT_IFACE_MAC_CHANGE, iface);

return ret;
}

int iface_add_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) {
Expand Down Expand Up @@ -512,6 +517,11 @@ static void iface_event(uint32_t event, const void *obj) {
gr_event_push(event, s);
}
break;
case GR_EVENT_IFACE_MAC_CHANGE:
str = "MAC_CHANGE";
gr_vec_foreach (struct iface *s, iface->subinterfaces)
gr_event_push(event, s);
break;
Comment on lines +520 to +524
Copy link

@coderabbitai coderabbitai bot Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

MAC_CHANGE propagation can trigger GARP/NA with a stale L2 source on inheriting subinterfaces (e.g., VLAN).

GR_EVENT_IFACE_MAC_CHANGE is forwarded to iface->subinterfaces (Line 520-524), but nothing here ensures subinterfaces that inherit the parent MAC refresh their effective MAC before IPv4/IPv6 handlers react. With the current VLAN implementation, iface_vlan_get_eth_addr() returns a cached vlan->mac, so a parent MAC change can cause neighbors to be “updated” with the old MAC (defeating the PR’s goal and potentially blackholing traffic).

Fix needs to ensure “inheriting” subifaces resolve the current parent MAC when handling MAC_CHANGE (either by making their get_eth_addr dynamic when inheriting, or by refreshing their cached MAC before emitting/handling the event).

default:
str = "?";
break;
Expand All @@ -521,7 +531,7 @@ static void iface_event(uint32_t event, const void *obj) {

static struct gr_event_subscription iface_event_handler = {
.callback = iface_event,
.ev_count = 7,
.ev_count = 8,
.ev_types = {
GR_EVENT_IFACE_ADD,
GR_EVENT_IFACE_POST_ADD,
Expand All @@ -530,6 +540,7 @@ static struct gr_event_subscription iface_event_handler = {
GR_EVENT_IFACE_POST_RECONFIG,
GR_EVENT_IFACE_STATUS_UP,
GR_EVENT_IFACE_STATUS_DOWN,
GR_EVENT_IFACE_MAC_CHANGE,
},
};

Expand Down
2 changes: 1 addition & 1 deletion modules/infra/control/port.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ static int iface_port_reconfig(
return ret;
}

if (set_attrs & GR_PORT_SET_MAC && (ret = port_mac_set(iface, &api->mac)) < 0)
if (set_attrs & GR_PORT_SET_MAC && (ret = iface_set_eth_addr(iface, &api->mac)) < 0)
return ret;

if (!p->started && (ret = rte_eth_dev_start(p->port_id)) < 0)
Expand Down
3 changes: 3 additions & 0 deletions modules/infra/control/port_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ struct rte_rcu_qsbr *gr_datapath_rcu(void) {
static struct rte_rcu_qsbr rcu;
return &rcu;
}
int iface_set_eth_addr(struct iface *, const struct rte_ether_addr *) {
return 0;
}
mock_func(struct iface *, iface_from_id(uint16_t));
mock_func(struct iface *, iface_next(gr_iface_type_t, const struct iface *));
mock_func(int, port_unplug(struct iface_info_port *));
Expand Down
40 changes: 28 additions & 12 deletions modules/infra/control/vlan.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,14 @@ static int iface_vlan_reconfig(
if ((cur_parent = iface_from_id(cur->parent_id)) == NULL)
return -errno;
if (set_attrs & GR_VLAN_SET_MAC) {
// reconfig, *not initial config*
// remove previous mac filter (ignore errors)
iface_del_eth_addr(cur_parent, &cur->mac);
struct rte_ether_addr parent_mac;
if (iface_get_eth_addr(cur_parent, &parent_mac) == 0
&& rte_is_same_ether_addr(&parent_mac, &cur->mac)) {
// inherited/primary MAC: nothing to delete
} else {
// remove previous mac filter (ignore errors)
iface_del_eth_addr(cur_parent, &cur->mac);
}
}
} else {
cur_parent = NULL;
Expand Down Expand Up @@ -90,15 +95,8 @@ static int iface_vlan_reconfig(
}

if (set_attrs & GR_VLAN_SET_MAC) {
struct iface *parent = iface_from_id(cur->parent_id);
if (rte_is_zero_ether_addr(&next->mac)) {
if ((ret = iface_get_eth_addr(parent, &cur->mac)) < 0)
return ret;
} else {
if ((ret = iface_add_eth_addr(parent, &next->mac)) < 0)
return ret;
cur->mac = next->mac;
}
if ((ret = iface_set_eth_addr(iface, &next->mac)) < 0)
return ret;
}
Comment on lines 97 to 100
Copy link

@coderabbitai coderabbitai bot Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

VLAN MAC inheritance is cached, so parent MAC changes can make VLAN advertise the wrong MAC (plus missing NULL guards).

iface_vlan_set_eth_addr() (Line 158-173) copies the parent MAC into vlan->mac when “inherit” is requested (00:00:00:00:00:00). That loses the “inherit” intent and makes iface_vlan_get_eth_addr() return stale data after a parent MAC change—exactly when this PR now triggers GARP/NA.

Also, parent and mac aren’t validated here; if this is ever called with a missing parent or a NULL mac, behavior degrades to wrong errno or a crash.

A robust fix is to persist “inherit” vs “explicit” in VLAN private state and make get_eth_addr dynamic when inheriting (and only add/del parent filters when explicit).

Proposed direction (requires adding an inherit flag in struct iface_info_vlan) 
 static int iface_vlan_get_eth_addr(const struct iface *iface, struct rte_ether_addr *mac) {
 	const struct iface_info_vlan *vlan = iface_info_vlan(iface);
-	*mac = vlan->mac;
-	return 0;
+	if (vlan->inherit_mac) {
+		const struct iface *parent = iface_from_id(vlan->parent_id);
+		return iface_get_eth_addr(parent, mac);
+	}
+	*mac = vlan->mac;
+	return 0;
 }
 
 static int iface_vlan_set_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) {
 	struct iface_info_vlan *vlan = iface_info_vlan(iface);
 	struct iface *parent = iface_from_id(vlan->parent_id);
 	int ret;
 
+	if (mac == NULL)
+		return errno_set(EINVAL);
+	if (parent == NULL)
+		return -errno; /* preserve ENODEV from iface_from_id */
+
 	if (rte_is_zero_ether_addr(mac)) {
-		if ((ret = iface_get_eth_addr(parent, &vlan->mac)) < 0)
-			return ret;
+		vlan->inherit_mac = true;
+		/* keep vlan->mac as last-known cache if you want, but don't rely on it */
+		return 0;
 	} else {
+		vlan->inherit_mac = false;
 		if ((ret = iface_add_eth_addr(parent, mac)) < 0)
 			return ret;
 		vlan->mac = *mac;
 	}
 
 	return 0;
 }

You’ll also want to ensure iface_vlan_fini() and the reconfig delete path only call iface_del_eth_addr(parent, &vlan->mac) when inherit_mac == false (otherwise you risk trying to delete the parent’s primary/inherited MAC).

</details>




Also applies to: 158-173, 212-213

<details>
<summary>🤖 Prompt for AI Agents</summary>

In @modules/infra/control/vlan.c around lines 97 - 100, The VLAN MAC handling
currently copies the parent MAC and loses the “inherit” semantics, and lacks
NULL checks; add a boolean inherit_mac (or similar) to struct iface_info_vlan,
set it instead of copying when the requested MAC is 00:00:00:00:00:00 in
iface_vlan_set_eth_addr(), validate that parent and mac pointers are non-NULL
before using them, update iface_vlan_get_eth_addr() to return the parent’s
current MAC dynamically when inherit_mac==true (only return stored vlan->mac
when inherit_mac==false), and ensure iface_vlan_fini() and the reconfig delete
path only call iface_del_eth_addr(parent, &vlan->mac) when inherit_mac==false so
you never try to delete an inherited/parent primary address.


</details>

<!-- This is an auto-generated comment by CodeRabbit -->


return 0;
Expand Down Expand Up @@ -157,6 +155,23 @@ static int iface_vlan_get_eth_addr(const struct iface *iface, struct rte_ether_a
return 0;
}

static int iface_vlan_set_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) {
struct iface_info_vlan *vlan = iface_info_vlan(iface);
struct iface *parent = iface_from_id(vlan->parent_id);
int ret;

if (rte_is_zero_ether_addr(mac)) {
if ((ret = iface_get_eth_addr(parent, &vlan->mac)) < 0)
return ret;
} else {
if ((ret = iface_add_eth_addr(parent, mac)) < 0)
return ret;
vlan->mac = *mac;
}

return 0;
}

static int iface_vlan_add_eth_addr(struct iface *iface, const struct rte_ether_addr *mac) {
const struct iface_info_vlan *vlan = iface_info_vlan(iface);
struct iface *parent = iface_from_id(vlan->parent_id);
Expand Down Expand Up @@ -194,6 +209,7 @@ static struct iface_type iface_type_vlan = {
.fini = iface_vlan_fini,
.set_up_down = iface_vlan_up_down,
.get_eth_addr = iface_vlan_get_eth_addr,
.set_eth_addr = iface_vlan_set_eth_addr,
.add_eth_addr = iface_vlan_add_eth_addr,
.del_eth_addr = iface_vlan_del_eth_addr,
.to_api = vlan_to_api,
Expand Down
4 changes: 4 additions & 0 deletions modules/infra/control/worker_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ struct iface *iface_next(gr_iface_type_t /*type_id*/, const struct iface *prev)
return NULL;
}

int iface_set_eth_addr(struct iface *, const struct rte_ether_addr *) {
return 0;
}

mock_func(int, worker_graph_reload(struct worker *, gr_vec struct iface_info_port **));
mock_func(int, worker_graph_reload_all(gr_vec struct iface_info_port **));
mock_func(void, worker_graph_free(struct worker *));
Expand Down
4 changes: 2 additions & 2 deletions modules/ip/control/address.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,8 +270,8 @@ static struct gr_event_subscription iface_pre_rm_subscription = {
};
static struct gr_event_subscription iface_up_subscription = {
.callback = iface_up_cb,
.ev_count = 1,
.ev_types = {GR_EVENT_IFACE_STATUS_UP},
.ev_count = 2,
.ev_types = {GR_EVENT_IFACE_STATUS_UP, GR_EVENT_IFACE_MAC_CHANGE},
};
static struct gr_event_serializer iface_addr_serializer = {
.size = sizeof(struct gr_ip4_ifaddr),
Expand Down
4 changes: 3 additions & 1 deletion modules/ip6/control/address.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,7 @@ static void ip6_iface_event_handler(uint32_t event, const void *obj) {
gr_vec_free(addrs->nh);
break;
case GR_EVENT_IFACE_STATUS_UP:
case GR_EVENT_IFACE_MAC_CHANGE:
addrs = &iface_addrs[iface->id];
gr_vec_foreach (nh, addrs->nh) {
if (nh6_advertise(nh, NULL) < 0)
Expand Down Expand Up @@ -471,11 +472,12 @@ static struct gr_module addr6_module = {

static struct gr_event_subscription iface_event_subscription = {
.callback = ip6_iface_event_handler,
.ev_count = 3,
.ev_count = 4,
.ev_types = {
GR_EVENT_IFACE_POST_ADD,
GR_EVENT_IFACE_PRE_REMOVE,
GR_EVENT_IFACE_STATUS_UP,
GR_EVENT_IFACE_MAC_CHANGE,
},
};

Expand Down