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. > > 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. > 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. > +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. > +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. > + > +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? > +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? > + > +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. > +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
