On Thu, Sep 10, 2026 at 9:51 AM Han Zhou <[email protected]> wrote:
> '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]>
> ---
>
Hi Han,
thank you for the patch, just one small comment down below.
> 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. */
>
I would just remove this comment, it doesn't add much value.
> + 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 416d5f9d0642..c6943ee0b05c 100644
> --- a/tests/test-ovn-netlink.c
> +++ b/tests/test-ovn-netlink.c
> @@ -340,6 +340,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[])
> {
> @@ -355,6 +395,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
>
>
With that:
Acked-by: Ales Musil <[email protected]>
Regards,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
