On Wed, Jan 15, 2025 at 4:26 PM Eelco Chaudron <[email protected]> wrote: > > > > On 13 Jan 2025, at 21:45, Frode Nordahl wrote: > > > Note that the internal handling of routes in the ovs-route module > > does currently not support multipath routes, so when presented > > with one the first occurrence will be stored. This is not a > > regression as these routes were previously not considered at all. > > > > Storing the information in the route-table module data structure > > will allow external to OVS projects make use of this data. > > > > A test program run as part of the system tests that exercise the > > route table API is added in this patch. > > > > Co-Authored-by: Felix Huettner <[email protected]> > > Signed-off-by: Felix Huettner <[email protected]> > > Signed-off-by: Frode Nordahl <[email protected]> > > I have one comment question below. The rest of the patch looks fine. > > In an earlier patch the following code was added: > > 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; > } > > I’m wondering how this is related to RTA_MULTIPATH. What combinations with > RTA_MULTIPATH are valid/invalid, and need checks like this added?
It is my understanding that the nested route messages that follow RTA_MULTIPATH behave similar to the regular route messages, and as such the same checks apply, this is why I chose the recursion approach because the parsing can be reused. Looking at kernel code the v4 case has checks for not having the combination of RTA_GATEWAY and RTA_VIA: https://github.com/torvalds/linux/blob/619f0b6fad524f08d493a98d55bac9ab8895e3a6/net/ipv4/fib_semantics.c#L908-L967 This is in line with the check highlighted above. The v6 case appears to mainly use RTA_GATEWAY: https://github.com/torvalds/linux/blob/619f0b6fad524f08d493a98d55bac9ab8895e3a6/net/ipv6/route.c#L5251-L5435 Slightly tangential topic: routing a v6 destination to a v4 nexthop does not look like a thing, so I was considering dropping the AF_INET part of the RTA_VIA handling. However, iproute2 handles this generically in both directions, which is why I chose to keep it: https://github.com/iproute2/iproute2/blob/c4e85023e7ae9a226f39d8bc8f85de43e02896e5/ip/iproute.c#L1019-L1099 So strictly speaking, I guess when parsing the nested MULTIPATH attributes we should treat RTA_OIF as being unexpected. Something like: diff --git a/lib/route-table.c b/lib/route-table.c index c9f1c241f..2f9f00cc3 100644 --- a/lib/route-table.c +++ b/lib/route-table.c @@ -301,6 +301,11 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs, change->rd.rtm_dst_len = rtm->rtm_dst_len; change->rd.rtm_protocol = rtm->rtm_protocol; change->rd.rtn_local = rtm->rtm_type == RTN_LOCAL; + if (attrs[RTA_OIF] && rtnh) { + VLOG_DBG_RL(&rl, "unexpected RTA_OIF attribute while parsing " + "nested RTA_MULTIPATH attributes"); + goto error_out; + } if (attrs[RTA_OIF] || rtnh) { rta_oif = rtnh ? rtnh->rtnh_ifindex : nl_attr_get_u32(attrs[RTA_OIF]); Do you want me to send that as a follow-on, do we need a re-roll, or is this something that could be folded in as part of review/merge? -- Frode Nordahl > Cheers, > > Eelco > > <SNIP> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
