On 15 Jan 2025, at 22:24, Ilya Maximets wrote:
> On 1/15/25 17:51, Frode Nordahl wrote: >> 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? >> > > I didn't follow this discussion very closely, but if Eelco acks the change, > I can fold it in before applying. Thanks Forde for the explanation, and I think the addition looks good. > (We discussed the workflow with Eelco today, and I'll be applying the set > tomorrow, just to avoid waiting on each other or stepping on each other's > toes right before branching. If all the changes are Acked, of course.) Ilya, please go ahead with applying the series, with the additional change above. So with this change folded in; Acked-by: Eelco Chaudron <[email protected]> This should have all patches in the series ACKed. Cheers, Eelco > Best regards, Ilya Maximets. > >> -- >> Frode Nordahl >> >>> Cheers, >>> >>> Eelco >>> >>> <SNIP> >>> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
