On Fri, Feb 06, 2026 at 12:05:16PM -0500, 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]>
I was reading this same function yesterday and was scratching my head
for the same reason, thanks for posting this patch.
I think it improves clarity and reduces possible pitfals. Only have some
nits and unrelated comments.
> ---
> lib/ovs-router.c | 47 ++++++++++++++++++++++++---------------------
> lib/packets.h | 5 +++++
> tests/ovs-router.at | 26 +++++++++++++++++++++++++
> 3 files changed, 56 insertions(+), 22 deletions(-)
>
> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
> index c11a52764..3ac421915 100644
> --- a/lib/ovs-router.c
> +++ b/lib/ovs-router.c
> @@ -175,28 +175,31 @@ ovs_router_lookup(uint32_t mark, const struct in6_addr
> *ip6_dst,
> struct in6_addr *src, struct in6_addr *gw)
Because of the complexity of the API, it might be worth considering some
ways to make it clearer, maybe a function-level comment would be a good
first start. This is unrelated to your patch though.
> {
> struct flow flow = {.ipv6_dst = *ip6_dst, .pkt_mark = mark};
> + bool is_ipv4 = IN6_IS_ADDR_V4MAPPED(ip6_dst);
> const struct in6_addr *from_src = src;
> const struct cls_rule *cr = NULL;
> struct router_rule *rule;
> + bool is_any = true;
>
> - if (src && ipv6_addr_is_set(src)) {
> - struct flow flow_src = {.ipv6_dst = *src, .pkt_mark = mark};
> - struct classifier *cls_local = cls_find(CLS_LOCAL);
> - const struct cls_rule *cr_src;
> + if (src) {
> + if (ipv4_addr_is_set(src) || ipv6_addr_is_set(src)) {
> + struct flow flow_src = {.ipv6_dst = *src, .pkt_mark = mark};
> + struct classifier *cls_local = cls_find(CLS_LOCAL);
> + const struct cls_rule *cr_src;
>
> - if (!cls_local) {
> - return false;
> - }
> + if (!cls_local) {
> + return false;
> + }
>
> - cr_src = classifier_lookup(cls_local, OVS_VERSION_MAX, &flow_src,
> - NULL, NULL);
> - if (!cr_src) {
> - return false;
> + cr_src = classifier_lookup(cls_local, OVS_VERSION_MAX, &flow_src,
> + NULL, NULL);
> + if (!cr_src) {
> + return false;
> + }
> + is_any = false;
> }
> - }
Also unrelated to your patch but moving this block to an independent
function called "check_addr_is_local" or something like that could help
reduce the length and improve readability.
> -
> - if (!from_src) {
> - if (IN6_IS_ADDR_V4MAPPED(ip6_dst)) {
> + } else {
> + if (is_ipv4) {
> from_src = &in6addr_v4mapped_any;
> } else {
> from_src = &in6addr_any;
> @@ -207,8 +210,7 @@ ovs_router_lookup(uint32_t mark, const struct in6_addr
> *ip6_dst,
> uint8_t plen = rule->ipv4 ? rule->src_prefix + 96 : rule->src_prefix;
> bool matched;
>
> - if ((IN6_IS_ADDR_V4MAPPED(from_src) && !rule->ipv4) ||
> - (!IN6_IS_ADDR_V4MAPPED(from_src) && rule->ipv4)) {
> + if (is_ipv4 != rule->ipv4) {
> continue;
> }
>
> @@ -231,12 +233,13 @@ ovs_router_lookup(uint32_t mark, const struct in6_addr
> *ip6_dst,
> if (cr) {
> struct ovs_router_entry *p = ovs_router_entry_cast(cr);
> /* Avoid matching mapped IPv4 of a packet against default
> IPv6
> - * route entry. Either packet dst is IPv6 or both packet and
> - * route entry dst are mapped IPv4.
> + * route entry. IPv6 status of the packet dst and route dst
> + * must be the same.
> */
> - if (!IN6_IS_ADDR_V4MAPPED(ip6_dst) ||
> - IN6_IS_ADDR_V4MAPPED(&p->nw_addr)) {
> + if (is_ipv4 == IN6_IS_ADDR_V4MAPPED(&p->nw_addr)) {
> break;
> + } else {
> + cr = NULL;
> }
> }
> }
> @@ -247,7 +250,7 @@ ovs_router_lookup(uint32_t mark, const struct in6_addr
> *ip6_dst,
>
> ovs_strlcpy(output_netdev, p->output_netdev, IFNAMSIZ);
> *gw = p->gw;
> - if (src && !ipv6_addr_is_set(src)) {
> + if (src && is_any) {
> *src = p->src_addr;
> }
> return true;
> diff --git a/lib/packets.h b/lib/packets.h
> index 61666f3ad..7e2e6be72 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -1202,6 +1202,11 @@ static inline bool ipv6_is_all_hosts(const struct
> in6_addr *addr) {
> return ipv6_addr_equals(addr, &in6addr_all_hosts);
> }
>
> +static inline bool ipv4_addr_is_set(const struct in6_addr *addr) {
> + return IN6_IS_ADDR_V4MAPPED(addr) &&
> + !ipv6_addr_equals(addr, &in6addr_v4mapped_any);
> +}
> +
> static inline bool ipv6_addr_is_set(const struct in6_addr *addr) {
> return !ipv6_addr_equals(addr, &in6addr_any);
> }
> diff --git a/tests/ovs-router.at b/tests/ovs-router.at
> index b5282fd19..258ff5c80 100644
> --- a/tests/ovs-router.at
> +++ b/tests/ovs-router.at
> @@ -315,6 +315,32 @@ OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1
> type=dummy])
> AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 192.0.2.1/24], [0], [OK
> ])
>
> +AT_CHECK([ovs-appctl ovs/route/rule/add from=all table=15], [0], [dnl
> +OK
> +])
> +AT_CHECK([ovs-appctl ovs/route/add 2.2.2.3/32 br0 192.0.2.1 table=15], [0],
> [dnl
> +OK
> +])
> +
> +AT_CHECK([ovs-appctl ovs/route/show table=all | sort], [0], [dnl
> +Cached: 192.0.2.0/24 dev br0 SRC 192.0.2.1
> +Cached: 192.0.2.1/32 dev br0 SRC 192.0.2.1 local
> +User: 2.2.2.3/32 dev br0 GW 192.0.2.1 SRC 192.0.2.1 table 15
> +])
> +
> +AT_CHECK([ovs-appctl ovs/route/lookup 2.2.2.3], [0], [dnl
> +src 192.0.2.1
> +gateway 192.0.2.1
> +dev br0
> +])
> +
> +AT_CHECK([ovs-appctl ovs/route/del 2.2.2.3/32 table=15], [0], [dnl
> +OK
> +])
> +AT_CHECK([ovs-appctl ovs/route/rule/del from=all table=15], [0], [dnl
> +OK
> +])
> +
nit: the rest of this test puts "OK" in the same line, maybe we could
keep it that way of coherence?
> AT_CHECK([ovs-appctl ovs/route/add 10.1.1.0/24 br0 192.0.2.2 table=11], [0],
> [OK
> ])
> AT_CHECK([ovs-appctl ovs/route/add 10.2.2.0/24 br0 192.0.2.2 table=12], [0],
> [OK
> --
> 2.52.0
>
> _______________________________________________
> 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
