On Fri, Sep 11, 2026 at 1:16 PM Ilya Maximets <[email protected]> wrote:
>
> On 8/8/26 8:00 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.
> >
> > With the net.ipv4.nexthop_compat_mode sysctl turned off the kernel
> > reports nothing but that id. Such a route then carries none of RTA_OIF,
> > RTA_GATEWAY, RTA_VIA and RTA_MULTIPATH, so it was considered unparseable
> > and dropped.
> >
> > Accept RTA_NH_ID and store it in 'struct route_data' so that users can
> > look the nexthop object up in the kernel nexthop table themselves. The
> > placeholder next hop is removed from the list whenever the route carries
> > nothing but the id, so an empty list tells the user that the next hop
> > still has to be resolved.
> >
> > Assisted-by: Claude Opus 5, Cursor
> > Signed-off-by: Han Zhou <[email protected]>
>
> Hi, Han. Thanks for the patch and sorry for delay.
>
> The subject line is a bit misleading as this patch doesn't alow OVS
> function in this situation it just parses the nexthop id. Which is fine
> as it is, but the subject line makes it look like we support using such
> routes now, which is not the case.
Agreed. Renamed to "route-table: Parse the nexthop id of a route." and
reworded the message accordingly.
>
> In general, OVS will not work properly with nexthop_compat_mode disabled
> as the routing for userspace tunnels may be broken and tc offload with
> the kernel datapath and well as a few other features. And since the
> syctl is global and not namespace-scoped, system adminiastrators on a
> node that runs OVS should not turn off this option.
>
> So, if the goal is actually to have it disabled on the system we need
> to add a full support in the ovs-router for these types of route records.
That is not the goal. With nexthop_compat_mode enabled the kernel reports
the next hop inline as before, and it also re-notifies the route whenever
the nexthop object changes -- I checked, replacing a group member and
changing group membership both produce an RTM_NEWROUTE with refreshed next
hops -- so nothing is lost in that mode and no nexthop table is needed.
This patch is only so that a consumer of lib/route-table that maintains
its own nexthop table does not silently drop these routes on a node where
an operator has already turned the sysctl off.
> This means maintaining the nexthop table, perform routing using that
> information and properly revalidate datapath flows when nexthops change.
> There is a performance concern here however. While we can limit the
> route dump to specific routing tables, I'm not sure if the same can be
> doen for nexthops as any route can refer to them and so we need all of
> them. If a routing daemon is churning them all the time this might
> create a noticeable load on ovs-vswitchd. Do you know what is a typical
> churn levels and the toal number of nexthop objects in such setups?
Resolving nexthop objects in ovs-router is separate work that I am not
proposing, so I would leave its scale question to that effort. The object
count tracks the number of distinct
next hop sets rather than the number of routes, so it is normally much
smaller than the route count.
>
> > ---
> > lib/route-table.c | 21 +++++++++++--
> > lib/route-table.h | 7 +++++
> > tests/system-route.at | 57 ++++++++++++++++++++++++++++++++++++
> > tests/test-lib-route-table.c | 5 ++--
> > 4 files changed, 86 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/route-table.c b/lib/route-table.c
> > index 2a13a5cc7d93..157479da850c 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;
> > @@ -672,9 +678,10 @@ 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]) {
> > + && !attrs[RTA_VIA] && !attrs[RTA_MULTIPATH]
> > + && !attrs[RTA_NH_ID]) {
> > VLOG_DBG_RL(&rl, "route message needs an RTA_OIF,
RTA_GATEWAY, "
> > - "RTA_VIA or RTA_MULTIPATH attribute");
> > + "RTA_VIA, RTA_MULTIPATH or RTA_NH_ID
attribute");
> > goto error_out;
> > }
> > /* Add any additional RTA attribute processing before
RTA_MULTIPATH. */
> > @@ -684,6 +691,16 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > if (!route_type_needs_nexthop(rtm->rtm_type)) {
> > route_data_destroy_nexthops__(&change->rd);
> > }
> > +
> > + /* When the route resolves through a separate nexthop object
> > + * (RTA_NH_ID) there is no inline next hop information in this
message.
> > + * Drop the empty placeholder next hop so that consumers can
tell that
> > + * the next hop(s) have to be resolved via the kernel nexthop
table. */
> > + if (change->rd.rta_nhid
> > + && !attrs[RTA_OIF] && !attrs[RTA_GATEWAY]
> > + && !attrs[RTA_VIA] && !attrs[RTA_MULTIPATH]) {
> > + route_data_destroy_nexthops__(&change->rd);
> > + }
>
> The comment is too long. Also, if the data was aleady provided and
parsed,
> why destroying it? It would be just inefficient to fetch a nexthop again.
> If the user doesn't need this info, they can destroy it themselves.
>
No parsed data is destroyed. The condition excludes RTA_OIF, RTA_GATEWAY,
RTA_VIA and RTA_MULTIPATH, so it only fires when the message carried no
next hop information at all. What is freed is the placeholder entry that
route_table_parse__() puts on the list before parsing, so that error_out
can clean up, and that nothing then filled in.
Removing it matters for ovs-router: route_table_handle_msg() is guarded by
!ovs_list_is_empty(&rd.nexthops), so with the placeholder gone such a
route is ignored exactly as it was before this patch. If it stays,
ovs-router takes the all-zero entry as the first next hop and inserts a
route with an empty ifname and a zero gateway. On a system with compat
mode enabled the branch never runs at all.
To make that visible I folded it into the existing "needs a next hop"
check instead of repeating the condition.
> > } else {
> > VLOG_DBG_RL(&rl, "received unparseable rtnetlink route
message");
> > goto error_out;
> > diff --git a/lib/route-table.h b/lib/route-table.h
> > index b49fbb14ebe0..4ef0b6c8d0f6 100644
> > --- a/lib/route-table.h
> > +++ b/lib/route-table.h
> > @@ -141,6 +141,13 @@ 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. */
> > +
> > + /* Id of the nexthop object (RTA_NH_ID) this route resolves
through, 0 if
> > + * missing. When set, the route's next hop(s) are not described
inline in
> > + * this message; they are stored in a separate nexthop object that
has to
> > + * be looked up in the kernel nexthop table. In that case the
'nexthops'
> > + * list above is left empty. */
>
> These, I assume, LLM-generated comments are way too long.
Yes :o).
>
> > + uint32_t rta_nhid;
> > };
> >
> > struct rule_data {
> > diff --git a/tests/system-route.at b/tests/system-route.at
> > index a074c51f9fd0..3f28e3d93cbf 100644
> > --- a/tests/system-route.at
> > +++ b/tests/system-route.at
> > @@ -333,6 +333,63 @@ 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.
> > +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 Make the kernel describe these routes through the nexthop id
alone. In
> > +dnl compatibility mode, which is the default, it also reports the
resolved
> > +dnl next hop inline, and that is precisely what must not be relied
upon here.
> > +compat_mode=$(sysctl -n net.ipv4.nexthop_compat_mode)
> > +on_exit "sysctl -wq net.ipv4.nexthop_compat_mode=$compat_mode"
> > +AT_CHECK([sysctl -wq net.ipv4.nexthop_compat_mode=0])
>
> I don't think we should do that. Since the config is not even
namespace-scoped
> running these test may break other software in the system. It should be
> enough to just check that the value is parsed out, which is all this
patch is
> doing anyway.
Dropped. The id is reported in compatibility mode as well, so the test
checks rta_nhid at the default setting and still fails without the patch,
where it would read 0. I removed the one assertion that required the
sysctl to be off, namely that no inline next hop is present.
I sent a v2:
https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/
Thanks,
Han
>
> If we'll have a full support for separate nexthop objects in OVS, then we
> still should not do this in system tests. Testing for the actual routing
> would need to be limited to manually injected nexthops and routes in the
> dummy datapath. System test would need to just make sure the IDs are
parsed
> and that nexthop objects are fetched from the kernel properly.
>
> Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
