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); > 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); > 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. In addition, we should add a check to make sure one of the two attributes is set, or else rdnh is not initialized. if (!attrs[RTA_GATEWAY] && !attrs[RTA_VIA]) { VLOG_DBG_RL(&rl, "route message needs an RTA_GATEWAY or RTA_VIA attribute"); goto error_out; } > + 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 > + 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. > + 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); > + 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]) 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 > +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
