On Tue, Oct 20, 2026 at 9:40 AM Ilya Maximets <[email protected]> wrote: > > On 9/13/26 9:14 AM, Han Zhou wrote: > > A route can describe its next hop with a separate nexthop object (see > > 'ip nexthop') that it refers to through a nexthop id carried in the > > RTA_NH_ID attribute, instead of encoding the next hop inline. Routing > > daemons commonly install routes this way, as it lets many routes share > > one next hop and be redirected at once. > > Hi, Han. Thanks for the update and sorry for delay.
Hi Ilya, thanks a lot for the review. > > > > > By default the kernel still reports the next hop inline as well, so such > > a route is parsed as any other and the id is merely extra information. > > With the net.ipv4.nexthop_compat_mode sysctl turned off the inline > > information is left out, and the route carries none of RTA_OIF, > > RTA_GATEWAY, RTA_VIA and RTA_MULTIPATH, so it was considered unparseable > > and dropped. > > This part of the justification is a little strange, as running OVS in an > environment with nexthop_compat_mode disabled would cause issues and can't > be recommended. Though it doesn't mean we can't parse the IDs and provide > them to the user of the library. > Reworded. The sysctl is now only mentioned as the reason a route can come without an inline next hop, and the justification is that users of the library that track the kernel nexthop table themselves can follow the id. > > Parse RTA_NH_ID and report it in 'struct route_data'. Nothing in the > > tree resolves it: users that want to follow the id have to maintain the > > kernel nexthop table themselves, and an empty nexthop list next to a > > non-zero id tells them that they have to. In particular ovs-router does > > not resolve it, so it still cannot route through such a route when the > > sysctl is off. > > > > Assisted-by: Claude Opus 5, Cursor > > Signed-off-by: Han Zhou <[email protected]> > > --- > > v2: Addressed Ilya's comments. > > > > lib/route-table.c | 19 +++++++++++--- > > lib/route-table.h | 3 +++ > > tests/system-route.at | 49 ++++++++++++++++++++++++++++++++++++ > > tests/test-lib-route-table.c | 5 ++-- > > 4 files changed, 71 insertions(+), 5 deletions(-) > > > > diff --git a/lib/route-table.c b/lib/route-table.c > > index 2a13a5cc7d93..ac6f8e74e50f 100644 > > --- a/lib/route-table.c > > +++ b/lib/route-table.c > > @@ -45,6 +45,7 @@ > > * in case we're building with old headers. (We can't test for it with > > #ifdef > > * because it's an enum.) */ > > #define RTA_MARK 16 /* Linux 2.6.36 */ > > +#define RTA_NH_ID 30 /* Linux 5.3 */ > > #define FRA_SUPPRESS_PREFIXLEN 14 /* Linux 3.12 */ > > #define FRA_TABLE 15 /* Linux 2.6.19 */ > > #define FRA_PROTOCOL 21 /* Linux 4.17 */ > > @@ -468,6 +469,7 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs, > > [RTA_PRIORITY] = { .type = NL_A_U32, .optional = true }, > > [RTA_VIA] = { .type = NL_A_RTA_VIA, .optional = true }, > > [RTA_MULTIPATH] = { .type = NL_A_NESTED, .optional = true }, > > + [RTA_NH_ID] = { .type = NL_A_U32, .optional = true }, > > }; > > > > static const struct nl_policy policy6[] = { > > @@ -480,6 +482,7 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs, > > [RTA_PRIORITY] = { .type = NL_A_U32, .optional = true }, > > [RTA_VIA] = { .type = NL_A_RTA_VIA, .optional = true }, > > [RTA_MULTIPATH] = { .type = NL_A_NESTED, .optional = true }, > > + [RTA_NH_ID] = { .type = NL_A_U32, .optional = true }, > > }; > > > > struct nlattr *attrs[ARRAY_SIZE(policy)]; > > @@ -585,6 +588,9 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs, > > if (attrs[RTA_PRIORITY]) { > > change->rd.rta_priority = nl_attr_get_u32(attrs[RTA_PRIORITY]); > > } > > + if (attrs[RTA_NH_ID]) { > > + change->rd.rta_nhid = nl_attr_get_u32(attrs[RTA_NH_ID]); > > + } > > if (attrs[RTA_VIA]) { > > const struct rtvia *rtvia = nl_attr_get(attrs[RTA_VIA]); > > ovs_be32 addr; > > @@ -673,9 +679,16 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs, > > if (route_type_needs_nexthop(rtm->rtm_type) > > && !attrs[RTA_OIF] && !attrs[RTA_GATEWAY] > > && !attrs[RTA_VIA] && !attrs[RTA_MULTIPATH]) { > > - VLOG_DBG_RL(&rl, "route message needs an RTA_OIF, RTA_GATEWAY, > > " > > - "RTA_VIA or RTA_MULTIPATH attribute"); > > - goto error_out; > > + if (!attrs[RTA_NH_ID]) { > > + VLOG_DBG_RL(&rl, "route message needs an RTA_OIF, " > > + "RTA_GATEWAY, RTA_VIA, RTA_MULTIPATH or " > > + "RTA_NH_ID attribute"); > > + goto error_out; > > + } > > + > > + /* The next hop is in the nexthop object named by RTA_NH_ID, so > > + * the entry added at the start of parsing was never filled > > in. */ > > + route_data_destroy_nexthops__(&change->rd); > > } > > /* Add any additional RTA attribute processing before > > RTA_MULTIPATH. */ > > > > diff --git a/lib/route-table.h b/lib/route-table.h > > index b49fbb14ebe0..45a9a012480a 100644 > > --- a/lib/route-table.h > > +++ b/lib/route-table.h > > @@ -141,6 +141,9 @@ struct route_data { > > uint32_t rta_mark; /* 0 if missing. */ > > uint32_t rta_table_id; /* 0 if missing. */ > > uint32_t rta_priority; /* 0 if missing. */ > > + uint32_t rta_nhid; /* Nexthop object id, 0 if missing. > > Resolved > > + by the user; 'nexthops' is empty > > unless the > > + kernel also reported it inline. */ > > }; > > > > struct rule_data { > > diff --git a/tests/system-route.at b/tests/system-route.at > > index a074c51f9fd0..439901375b61 100644 > > --- a/tests/system-route.at > > +++ b/tests/system-route.at > > @@ -333,6 +333,55 @@ AT_CHECK([ovstest test-lib-route-table-dump | \ > > > > AT_CLEANUP > > > > +dnl Checks that routes whose next hop is described by a separate nexthop > > +dnl object (referenced through a nexthop id, RTA_NH_ID) are parsed and > > expose > > +dnl the nexthop id, instead of being dropped. > > This description is probably not needed. Too verbose a bit obvious. > Also, "instead of being dropped" doesn't make a lot of sense in the > context of the file as opposed to context of the patch. > Dropped. > > +AT_SETUP([route-table - route with nexthop id]) > > +AT_KEYWORDS([route]) > > + > > +dnl Skip if the running kernel / iproute2 does not support nexthop objects. > > +AT_SKIP_IF([! ip nexthop show >/dev/null 2>&1]) > > + > > +dnl Create a port. A nexthop object can only reference a device that has > > +dnl carrier, which an unattached tap port does not have. > > +AT_CHECK([ip link add p1-route type dummy]) > > Our testsuite doesn't load the dummy module and doesn't check if it > is available. It happens to be available in GHA, but that's not a > guarantee. I'd suggest creating a veth instead. It's fine to just > use one side of it later in the test. > Done. > > +AT_CHECK([ip link set p1-route up]) > > +on_exit 'ip link del p1-route' > > I see we have this pattern in the other test, but the on_ext hook should > really be called before we add the device or right after. > Done. > > + > > +AT_CHECK([ip addr add 10.0.0.17/24 dev p1-route], [0], [stdout]) > > +AT_CHECK([ip -6 addr add fc00:db8:cafe::17/64 dev p1-route], [0], [stdout]) > > + > > +dnl Create standalone nexthop objects and a nexthop group referencing them. > > +on_exit 'ip nexthop del id 110; ip nexthop del id 100; ip nexthop del id > > 101; ip nexthop del id 102' > > This line is a bit too long. I'd suggest one line per nexthop and also > maybe do the calls right before adding a corresponding nexthop. We do > similar thing for routes in the other test. > > Also, maybe use some higher-numbered IDs that are less likely to be used? > Done. > > +AT_CHECK([ip nexthop add id 100 via 10.0.0.18 dev p1-route]) > > +AT_CHECK([ip nexthop add id 101 via 10.0.0.19 dev p1-route]) > > +AT_CHECK([ip nexthop add id 110 group 100/101]) > > +AT_CHECK([ip nexthop add id 102 via fc00:db8:cafe::18 dev p1-route]) > > + > > +dnl Add routes that reference their next hop indirectly through a nexthop > > id. > > +AT_CHECK([ip route add 192.168.10.0/24 nhid 100], [0], [stdout]) > > +AT_CHECK([ip route add 192.168.20.0/24 nhid 110], [0], [stdout]) > > +OVS_WAIT_UNTIL([ip -6 route add fc00:db8:beef::/64 nhid 102]) > > May also be better to use /32 /96 routes to reduce the impact if there > is an overlap with something else in the system. Maybe we can also add > these into some non-default table? > Done, in table 42. I used /128 rather than /96 so both are host routes. > > + > > +dnl The routes must be parsed as relevant and report the nexthop id, for a > > +dnl single nexthop object as well as for a group of them. The kernel also > > +dnl reports the resolved next hop inline unless the nexthop_compat_mode > > sysctl > > +dnl is turned off, which this test leaves alone as it affects the whole > > host. > > Not sure if we need this large comment. Some part may be moved to the > commit message instead. > Done, one line left, the rest is in the commit message. PTAL at v3: https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/ Best, Han > > +AT_CHECK([ovstest test-lib-route-table-dump | \ > > + awk '/^192.168.10.0/{print$1" "$3" "$19" "$20}'], [0], [dnl > > +192.168.10.0/24 1 rta_nhid: 100 > > +]) > > +AT_CHECK([ovstest test-lib-route-table-dump | \ > > + awk '/^192.168.20.0/{print$1" "$3" "$19" "$20}'], [0], [dnl > > +192.168.20.0/24 1 rta_nhid: 110 > > +]) > > +AT_CHECK([ovstest test-lib-route-table-dump | \ > > + awk '/^fc00:db8:beef::/{print$1" "$3" "$19" "$20}'], [0], [dnl > > +fc00:db8:beef::/64 1 rta_nhid: 102 > > +]) > > + > > +AT_CLEANUP > > + > > dnl Checks that OVS ignores unsupported routing rules. > > AT_SETUP([ovs-route - unsupported rules]) > > AT_KEYWORDS([route]) > > diff --git a/tests/test-lib-route-table.c b/tests/test-lib-route-table.c > > index f99f056c8ddc..d6fe20cb180e 100644 > > --- a/tests/test-lib-route-table.c > > +++ b/tests/test-lib-route-table.c > > @@ -86,11 +86,12 @@ test_lib_route_table_handle_msg(const struct > > route_table_msg *change, > > > > printf("%s/%u relevant: %d nlmsg_type: %d rtm_protocol: %s (%u) " > > "rtn_local: %d rta_prefsrc: %s rta_mark: %"PRIu32" " > > - "rta_table_id: %s rta_priority: %"PRIu32"\n", > > + "rta_table_id: %s rta_priority: %"PRIu32" rta_nhid: > > %"PRIu32"\n", > > ds_cstr(&rta_dst), rd->rtm_dst_len, change->relevant, > > change->nlmsg_type, rt_prot_name(rd->rtm_protocol), > > rd->rtm_protocol, rd->rtn_local, ds_cstr(&rta_prefsrc), > > - rd->rta_mark, rt_table_name(rd->rta_table_id), > > rd->rta_priority); > > + rd->rta_mark, rt_table_name(rd->rta_table_id), rd->rta_priority, > > + rd->rta_nhid); > > > > LIST_FOR_EACH (rdnh, nexthop_node, &rd->nexthops) { > > ds_clear(&nexthop_addr); > > Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
