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