On 7/15/26 8:06 PM, Mike Pattrick via dev wrote:
> The source address parameter in ovs_router_lookup is both an input and
> an output. However, the interface was complex. The caller could
> inadvertently set a search over v4 or v6 rules based on if the source
> address was initialized to in6addr_any or in6addr_v4mapped_any. The
> lookup function even used these two values interchangeably.
>
> This patch uses dst address to determine if the lookup is v4 or v6, and
> considers both v6_any and v4mapped_any to be the null value equally. Now
> if the caller just wants src as output, they can initialize it to v6_any
> and lookup will still work correctly.
>
> Fixes: dc14e92bcc25 ("route-table: Introduce multi-table route lookup.")
> Signed-off-by: Mike Pattrick <[email protected]>
> ---
> v2:
> - Split src parameter into in and out versions in lookup function.
> - Big refactor of lookup function.
> - Added comment to explain their use.
> - Changed unit test formatting.
> - Added address family to router classifier.
> v3:
> - Split ipv4 addresses out of ipv6_dst field
> - Updated comments
> ---
Hi, Mike. Thanks for the update and sorry for delay.
This version looks good to me for the most part. But I ran it through
some LLM review and got the following feedback that seems reasonable.
Could you, please, take a look?
---
This patch reworks ovs_router_lookup() so that when a source address is
supplied it builds a CLS_LOCAL flow keyed on that source and reports it
back to the caller. A new assertion requires the source and destination
addresses to share the same address family.
> + if (ip6_src) {
> + ovs_assert(is_ipv4 == IN6_IS_ADDR_V4MAPPED(ip6_src));
Here is_ipv4 is derived only from the destination:
> bool is_ipv4 = IN6_IS_ADDR_V4MAPPED(ip6_dst);
Can this assert abort ovs-vswitchd when the destination and source
belong to different families? ovs_assert_failure() is OVS_NO_RETURN and
ovs_assert stays enabled in the default build, so a false condition here
takes down forwarding on the host rather than returning an error.
One reachable path is packet processing. tnl_route_lookup_flow() in
ofproto/ofproto-dpif-xlate.c derives in_src from tunnel.ip_src /
tunnel.ipv6_src and dst from flow_tnl_dst() independently. For a
flow-based tunnel (remote_ip=flow, local_ip=flow) tnl_port_send() leaves
both as set by the pipeline, and mf_set_flow_value() sets MFF_TUN_DST
without clearing ipv6_src (and MFF_TUN_IPV6_SRC without clearing ip_dst)
in lib/meta-flow.c. Can a controller-installed flow using set_field on
MFF_TUN_DST together with MFF_TUN_IPV6_SRC produce a v4-mapped dst with
a v6 src, reaching this assert with is_ipv4 true and a non-v4-mapped
ip6_src?
A second path is the unixctl command. ovs_router_lookup_cmd() in
lib/ovs-router.c parses an IPv6 destination via scan_ipv6_route but
parses the src= argument with ipv6_parse(), whose IPV6_SCAN_FMT accepts
v4-mapped text such as '::ffff:1.2.3.4'. Does
'ovs-appctl ovs/route/lookup <ipv6-dst> src=::ffff:1.2.3.4' reach the
lookup with a v6 dst and a v4-mapped src, tripping the assert? There is
no cross-family check before the call.
Would it be safer to return false on a family mismatch, the way the
pre-patch code handled an unmatched source gracefully, rather than
asserting?
---
> + if (is_ipv4) {
> + flow = (struct flow) {.nw_dst =
> in6_addr_get_mapped_ipv4(ip6_src),
> + .pkt_mark = mark, .dl_type = dl_type};
> + } else {
> + flow = (struct flow) {.ipv6_dst = *ip6_src, .pkt_mark = mark,
> + .dl_type = dl_type};
> + }
[...]
> + if (src_out) {
> + *src_out = *ip6_src;
In a build configured with --enable-ndebug the assert above becomes a
no-op (ovs_assert maps to ovs_ignore in include/openvswitch/util.h), so
this branch runs even on a mismatched-family source. When is_ipv4 is
true but ip6_src is a genuine IPv6 address,
in6_addr_get_mapped_ipv4(ip6_src) returns INADDR_ANY, so does the
CLS_LOCAL lookup then run against 0.0.0.0 instead of the intended
source? And when is_ipv4 is false but ip6_src is v4-mapped, is the
v4-mapped address used as a full IPv6 key?
In both cases *src_out = *ip6_src copies the mismatched-family source
back to the caller. Should the family consistency be enforced with a
real runtime check that survives NDEBUG, so a release build does not
silently return a wrong route and source?
---
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
