'struct nexthop_grp_entry' points directly at the 'struct nexthop_entry'
of the group member, and those pointers are only refreshed for the groups
that show up in the netlink batch. The kernel however does not report a
group when one of its members is replaced:
# ip nexthop add id 1 via 10.0.0.2 fdb
# ip nexthop add id 2 via 10.0.0.3 fdb
# ip nexthop add id 10 group 1/2 fdb
# ip monitor nexthop &
# ip nexthop replace id 1 via 10.0.0.44 fdb
id 1 via 10.0.0.44 scope link fdb
Only the member is announced, so nexthops_handle_changes() frees the old
entry of id 1 and inserts a new one while group 10 keeps pointing at the
freed entry. evpn_fdb_resolve_paths() then reads it while resolving the
ECMP paths of a static FDB entry.
Member deletion is not affected, it changes the group membership so the
kernel does announce the group as well.
Refresh the member pointers of all the groups after processing a batch
instead of only of those we got a message for.
Fixes: ff05f129bbf4 ("controller: Add nexthop exchange node.")
Assisted-by: Claude Opus 5, Cursor
Signed-off-by: Han Zhou <[email protected]>
---
controller/nexthop-exchange.c | 18 +++++----------
tests/system-ovn-netlink.at | 30 +++++++++++++++++++++++++
tests/test-ovn-netlink.c | 42 +++++++++++++++++++++++++++++++++++
3 files changed, 78 insertions(+), 12 deletions(-)
diff --git a/controller/nexthop-exchange.c b/controller/nexthop-exchange.c
index 37e80f161db5..8075dd4937a8 100644
--- a/controller/nexthop-exchange.c
+++ b/controller/nexthop-exchange.c
@@ -19,7 +19,6 @@
#include "lib/netlink.h"
#include "lib/netlink-socket.h"
-#include "hmapx.h"
#include "openvswitch/ofpbuf.h"
#include "openvswitch/vlog.h"
#include "packets.h"
@@ -157,8 +156,6 @@ nexthops_handle_changes(struct hmap *nexthops, struct
vector *msgs)
return false;
}
- struct hmapx updated_groups = HMAPX_INITIALIZER(&updated_groups);
-
struct nh_table_msg *msg;
VECTOR_FOR_EACH_PTR (msgs, msg) {
struct nexthop_entry *nhe = nexthop_entry_find(nexthops, msg->nhe->id);
@@ -171,23 +168,20 @@ nexthops_handle_changes(struct hmap *nexthops, struct
vector *msgs)
hmap_insert(nexthops, &msg->nhe->hmap_node,
nexthop_entry_hash(msg->nhe->id));
- if (msg->nhe->n_grps) {
- hmapx_add(&updated_groups, msg->nhe);
- }
-
/* The nexthop entry moved into the hmap, prevent double free. */
msg->nhe = NULL;
}
}
- struct hmapx_node *hmapx_node;
- HMAPX_FOR_EACH (hmapx_node, &updated_groups) {
- struct nexthop_entry *nhe = hmapx_node->data;
+ /* Replacing a group member frees the old entry without the kernel
+ * necessarily reporting the groups referencing it, so refresh the member
+ * pointers of every group instead of only the ones we got a message for.
+ * Otherwise those groups would keep pointing at freed entries. */
+ struct nexthop_entry *nhe;
+ HMAP_FOR_EACH (nhe, hmap_node, nexthops) {
nh_populate_grp_pointers(nhe, nexthops);
}
- hmapx_destroy(&updated_groups);
-
return true;
}
diff --git a/tests/system-ovn-netlink.at b/tests/system-ovn-netlink.at
index 626ec16ac891..dedfd14f2d6f 100644
--- a/tests/system-ovn-netlink.at
+++ b/tests/system-ovn-netlink.at
@@ -723,3 +723,33 @@ NS_CHECK_EXEC([nh], [ovstest test-ovn-netlink
nexthop-table-notify \
])
AT_CLEANUP
+
+AT_SETUP([sync netlink nexthops - group member update])
+AT_KEYWORDS([netlink-nexthops])
+
+ADD_NAMESPACES(nh)
+NS_EXEC([nh], [ip link set up lo])
+
+NS_CHECK_EXEC([nh], [ip nexthop add id 1 via 192.168.1.1 fdb], [0])
+NS_CHECK_EXEC([nh], [ip nexthop add id 2 via 192.168.1.2 fdb], [0])
+NS_CHECK_EXEC([nh], [ip nexthop add id 10 group 1/2 fdb], [0])
+
+dnl Replacing a member leaves the membership of the group untouched, so the
+dnl kernel reports the member alone. The group must still end up pointing at
+dnl the new address of that member.
+NS_CHECK_EXEC([nh], [ovstest test-ovn-netlink nexthop-table-update \
+ "ip nexthop replace id 1 via 192.168.1.9 fdb" | sort], [0], [dnl
+Nexthop id=1, address=192.168.1.9
+Nexthop id=10, group=[[1;192.168.1.9;1, 2;192.168.1.2;1]]
+Nexthop id=2, address=192.168.1.2
+])
+
+dnl Deleting a member does change the membership, so here the kernel reports
+dnl the group as well.
+NS_CHECK_EXEC([nh], [ovstest test-ovn-netlink nexthop-table-update \
+ "ip nexthop del id 2" | sort], [0], [dnl
+Nexthop id=1, address=192.168.1.9
+Nexthop id=10, group=[[1;192.168.1.9;1]]
+])
+
+AT_CLEANUP
diff --git a/tests/test-ovn-netlink.c b/tests/test-ovn-netlink.c
index 84de2bd63073..2019c18cf29c 100644
--- a/tests/test-ovn-netlink.c
+++ b/tests/test-ovn-netlink.c
@@ -334,6 +334,46 @@ test_nexthop_table_notify(struct ovs_cmdl_context *ctx)
ovn_netlink_notifiers_destroy();
}
+/* Dumps the nexthop table after applying the changes caused by running
+ * 'shell_command' to it. Unlike "nexthop-sync", which builds the table from
+ * scratch, this goes through the incremental update path. */
+static void
+test_nexthop_table_update(struct ovs_cmdl_context *ctx)
+{
+ unsigned int shift = 1;
+
+ const char *cmd = test_read_value(ctx, shift++, "shell_command");
+ if (!cmd) {
+ return;
+ }
+
+ struct hmap nexthops = HMAP_INITIALIZER(&nexthops);
+ struct ds ds = DS_EMPTY_INITIALIZER;
+
+ ovn_netlink_update_notifier(OVN_NL_NOTIFIER_NEXTHOP, true);
+ nexthops_sync(&nexthops);
+ /* The table is up to date, anything reported so far is already in it. */
+ ovn_netlink_notifier_flush(OVN_NL_NOTIFIER_NEXTHOP);
+
+ run_command_under_notifier(cmd);
+
+ nexthops_handle_changes(&nexthops,
+ ovn_netlink_get_msgs(OVN_NL_NOTIFIER_NEXTHOP));
+ ovn_netlink_notifier_flush(OVN_NL_NOTIFIER_NEXTHOP);
+
+ struct nexthop_entry *nhe;
+ HMAP_FOR_EACH (nhe, hmap_node, &nexthops) {
+ ds_clear(&ds);
+ nexthop_entry_format(&ds, nhe);
+ printf("Nexthop %s\n", ds_cstr(&ds));
+ }
+
+ ds_destroy(&ds);
+ nexthops_destroy(&nexthops);
+ hmap_destroy(&nexthops);
+ ovn_netlink_notifiers_destroy();
+}
+
static void
test_ovn_netlink(int argc, char *argv[])
{
@@ -349,6 +389,8 @@ test_ovn_netlink(int argc, char *argv[])
{"nexthop-sync", NULL, 0, 0, test_nexthop_sync, OVS_RO},
{"nexthop-table-notify", NULL, 1, 1,
test_nexthop_table_notify, OVS_RO},
+ {"nexthop-table-update", NULL, 1, 1,
+ test_nexthop_table_update, OVS_RO},
{NULL, NULL, 0, 0, NULL, OVS_RO},
};
struct ovs_cmdl_context ctx;
--
2.38.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
