On Fri, Jan 3, 2025 at 6:02 PM Eelco Chaudron <[email protected]> wrote:
>
> On 18 Dec 2024, at 20:12, Frode Nordahl wrote:
>
> > It is possible to add routes for IPv4 destinations using an IPv6
> > address for next hop.
> >
> > In such configurations the next hop information is provided in the
> > RTA_VIA attribute instead of the RTA_GATEWAY attribute.
>
> Some comments below.
>
> //Eelco
>
> > Signed-off-by: Frode Nordahl <[email protected]>
> > ---
> > lib/netlink.c | 2 ++
> > lib/netlink.h | 1 +
> > lib/route-table.c | 41 +++++++++++++++++++++++++++++++++++++++++
> > tests/system-route.at | 18 ++++++++++++++++++
> > 4 files changed, 62 insertions(+)
> >
> > diff --git a/lib/netlink.c b/lib/netlink.c
> > index 1e8d5a8ec..566c6c694 100644
> > --- a/lib/netlink.c
> > +++ b/lib/netlink.c
> > @@ -819,6 +819,7 @@ min_attr_len(enum nl_attr_type type)
> > case NL_A_IPV6: return 16;
> > case NL_A_NESTED: return 0;
> > case NL_A_LL_ADDR: return 6; /* ETH_ALEN */
> > + case NL_A_RTA_VIA: return 6; /* rtvia header + AF_INET address */
>
> Rather than adding a comment, what about:
>
> case NL_A_RTA_VIA: return sizeof(struct rtvia) + sizeof(struct in_addr);
That does make sense to me, my default behavior is to adhere to
existing style and I take your comment as acceptance of deviating from
it.
> > case N_NL_ATTR_TYPES: default: OVS_NOT_REACHED();
> > }
> > }
> > @@ -840,6 +841,7 @@ max_attr_len(enum nl_attr_type type)
> > case NL_A_IPV6: return 16;
> > case NL_A_NESTED: return SIZE_MAX;
> > case NL_A_LL_ADDR: return 20; /* INFINIBAND_ALEN */
> > + case NL_A_RTA_VIA: return 18; /* rtvia header + AF_INET6 address */
>
> case NL_A_RTA_VIA: return sizeof(struct rtvia) + sizeof(struct in6_addr);
Done.
> > case N_NL_ATTR_TYPES: default: OVS_NOT_REACHED();
> > }
> > }
> > diff --git a/lib/netlink.h b/lib/netlink.h
> > index 008604aa6..d98ef3a98 100644
> > --- a/lib/netlink.h
> > +++ b/lib/netlink.h
> > @@ -152,6 +152,7 @@ enum nl_attr_type
> > NL_A_IPV6,
> > NL_A_NESTED,
> > NL_A_LL_ADDR,
> > + NL_A_RTA_VIA,
> > N_NL_ATTR_TYPES
> > };
> >
> > diff --git a/lib/route-table.c b/lib/route-table.c
> > index b6f0223c0..4e5f89f7e 100644
> > --- a/lib/route-table.c
> > +++ b/lib/route-table.c
> > @@ -51,6 +51,7 @@ COVERAGE_DEFINE(route_table_dump);
> > struct route_data_nexthop {
> > struct ovs_list nexthop_node;
> >
> > + sa_family_t family;
> > struct in6_addr addr;
> > char ifname[IFNAMSIZ]; /* Interface name. */
> > };
> > @@ -277,6 +278,7 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > [RTA_PREFSRC] = { .type = NL_A_U32, .optional = true },
> > [RTA_TABLE] = { .type = NL_A_U32, .optional = true },
> > [RTA_PRIORITY] = { .type = NL_A_U32, .optional = true },
> > + [RTA_VIA] = { .type = NL_A_RTA_VIA, .optional = true },
> > };
> >
> > static const struct nl_policy policy6[] = {
> > @@ -287,6 +289,7 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > [RTA_PREFSRC] = { .type = NL_A_IPV6, .optional = true },
> > [RTA_TABLE] = { .type = NL_A_U32, .optional = true },
> > [RTA_PRIORITY] = { .type = NL_A_U32, .optional = true },
> > + [RTA_VIA] = { .type = NL_A_RTA_VIA, .optional = true },
> > };
> >
> > struct nlattr *attrs[ARRAY_SIZE(policy)];
> > @@ -311,6 +314,7 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > /* ovs_list_init / ovs_list_insert does not allocate any memory */
> > ovs_list_init(&change->rd.nexthops);
> > rdnh = &change->rd._primary_next_hop;
> > + rdnh->family = rtm->rtm_family;
> > ovs_list_insert(&change->rd.nexthops, &rdnh->nexthop_node);
> >
> > change->relevant = true;
> > @@ -385,6 +389,43 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> > if (attrs[RTA_PRIORITY]) {
> > change->rd.rta_priority = nl_attr_get_u32(attrs[RTA_PRIORITY]);
> > }
>
> Above in 'if (attrs[RTA_GATEWAY])' you also need to set rdnh->family.
rdnh->family is initialized to rtm->rtm_family at the top, and
overridden in case of RTA_VIA.
> In addition, we should add a check to make sure one of the two attributes is
> set, or else rdnh is not initialized.
In any case it will be initialized due to call to memset/xzalloc, and
I guess the extra check does not hurt.
> if (!attrs[RTA_GATEWAY] && !attrs[RTA_VIA]) {
> VLOG_DBG_RL(&rl, "route message needs an RTA_GATEWAY or RTA_VIA
> attribute");
> goto error_out;
> }
We'd have to include a check for RTA_OIF here, as having a route with
only an OIF is valid. Done.
> > + if (attrs[RTA_VIA]) {
> > + const struct rtvia *rtvia = nl_attr_get(attrs[RTA_VIA]);
> > + ovs_be32 addr;
> > +
> > + if (attrs[RTA_GATEWAY]) {
> > + VLOG_DBG_RL(&rl, "route message can not contain both "
> > + "RTA_GATEWAY and RTA_VIA.");
> > + goto error_out;
> > + }
> > +
> > + rdnh->family = rtvia->rtvia_family;
> > + switch (rdnh->family) {
> > + case AF_INET:
> > + if (nl_attr_get_size(attrs[RTA_VIA])
> > + - sizeof rtvia->rtvia_family < sizeof addr) {
>
> This does not look right (same for ipv6). Should this be; - sizeof *rtvia <
> sizeof addr
The reason for the choice is that the rtvia_addr element is just a
placeholder whose real size is different, however the struct might
change in the future, in which case this would become a bug, thanks!
Done.
> > + VLOG_DBG_RL(&rl, "Got short message while parsing
> > RTA_VIA "
> > + "attribute for family AF_INET.");
>
> Log messages in this module do not seem to start with a capital. And in
> general log messages do not end with a dot. So please update all new messages
> you added.
Ack.
> > + goto error_out;
> > + }
> > + memcpy(&addr, rtvia->rtvia_addr, sizeof addr);
> > + in6_addr_set_mapped_ipv4(&rdnh->addr, addr);
> > + break;
> > + case AF_INET6:
> > + if (nl_attr_get_size(attrs[RTA_VIA])
> > + - sizeof rtvia->rtvia_family < sizeof rdnh->addr) {
> > + VLOG_DBG_RL(&rl, "Got short message while parsing
> > RTA_VIA "
> > + "attribute for family AF_INET6.");
> > + goto error_out;
> > + }
> > + memcpy(&rdnh->addr, rtvia->rtvia_addr, sizeof rdnh->addr);
> > + break;
> > + default:
> > + VLOG_DBG_RL(&rl, "No address family in via attribute.");
>
> What about:
>
> VLOG_DBG_RL(&rl, "unsupported address family, %d, in via attribute",
> rdnh->family);
Thanks, done.
> > + goto error_out;
> > + }
> > + }
> > } else {
> > VLOG_DBG_RL(&rl, "received unparseable rtnetlink route message");
> > goto error_out;
> > diff --git a/tests/system-route.at b/tests/system-route.at
> > index c99a2aff5..c9fed2fc6 100644
> > --- a/tests/system-route.at
> > +++ b/tests/system-route.at
> > @@ -65,6 +65,24 @@ Cached: fc00:db8:beef::13/128 dev br0 GW
> > fc00:db8:cafe::1 SRC fc00:db8:cafe::2])
> > OVS_TRAFFIC_VSWITCHD_STOP
> > AT_CLEANUP
> >
> > +AT_SETUP([ovs-route - add system route - ipv4 via ipv6 nexthop])
> > +AT_KEYWORDS([route])
> > +OVS_TRAFFIC_VSWITCHD_START()
> > +AT_CHECK([ovs-vsctl set bridge br0 other-config:hwaddr=00:53:00:00:00:42])
> > +AT_CHECK([ip link set br0 up])
> > +
> > +AT_CHECK([ip addr add 192.168.9.2/24 dev br0], [0], [stdout])
> > +
> > +AT_CHECK([ip route add 192.168.10.12/32 via inet6 fe80::253:ff:fe00:51 dev
> > br0], [0], [stdout])
>
> Maybe try to keep the test lines under 80 chars for new additions. Maybe
> something like:
>
> -AT_CHECK([ip route add 192.168.10.12/32 via inet6 fe80::253:ff:fe00:51 dev
> br0], [0], [stdout])
> +AT_CHECK([ip route add 192.168.10.12/32 via inet6 fe80::253:ff:fe00:51 dev
> br0],
> + [0], [stdout])
Sure, I don't see this mode of formatting anywhere else, but I do like
shorter than 79 length lines too, let's try it!
I chose to split the command in the middle, because we would otherwise
end up > 79 chars anyway.
> AT_CHECK([ovs-appctl revalidator/wait])
>
> -OVS_WAIT_UNTIL_EQUAL([ovs-appctl ovs/route/show | grep -E '192.168.10.12/32'
> | sort], [dnl
> +OVS_WAIT_UNTIL_EQUAL([ovs-appctl ovs/route/show | \
> + grep -E '192.168.10.12/32' | sort], [dnl
Done.
--
Frode Nordahl
> > +AT_CHECK([ovs-appctl revalidator/wait])
> > +
> > +OVS_WAIT_UNTIL_EQUAL([ovs-appctl ovs/route/show | grep -E
> > '192.168.10.12/32' | sort], [dnl
> > +Cached: 192.168.10.12/32 dev br0 GW fe80::253:ff:fe00:51 SRC
> > fe80::253:ff:fe00:42])
> > +
> > +OVS_TRAFFIC_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> > dnl Checks that OVS doesn't use routes from non-standard tables.
> > AT_SETUP([ovs-route - route tables])
> > AT_KEYWORDS([route])
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
