On 8/27/26 7:46 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
> v4:
> - Removed assert, replaced with conditional
> ---
> lib/flow.c | 2 +-
> lib/netdev-vport.c | 2 +-
> lib/ovs-router.c | 155 +++++++++++++++++++++--------------
> lib/ovs-router.h | 2 +-
> ofproto/ofproto-dpif-sflow.c | 2 +-
> ofproto/ofproto-dpif-xlate.c | 30 ++++---
> tests/ovs-router.at | 22 +++++
> 7 files changed, 139 insertions(+), 76 deletions(-)
>
> diff --git a/lib/flow.c b/lib/flow.c
> index a59a25c46..d14603fe3 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -3731,7 +3731,7 @@ flow_get_tunnel_netdev(struct flow_tnl *tunnel)
> return NULL;
> }
>
> - if (!ovs_router_lookup(0, &ip6, iface, NULL, &gw)) {
> + if (!ovs_router_lookup(0, &ip6, NULL, iface, NULL, &gw)) {
> return NULL;
> }
>
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index d11269d00..aa5223128 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -300,7 +300,7 @@ tunnel_check_status_change__(struct netdev_vport *netdev)
> iface[0] = '\0';
> route = &tnl_cfg->ipv6_dst;
> mark = tnl_cfg->egress_pkt_mark;
> - if (ovs_router_lookup(mark, route, iface, NULL, &gw)) {
> + if (ovs_router_lookup(mark, route, NULL, iface, NULL, &gw)) {
> struct netdev *egress_netdev;
>
> if (!netdev_open(iface, NULL, &egress_netdev)) {
> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
> index 2566386ea..bcc2ceda9 100644
> --- a/lib/ovs-router.c
> +++ b/lib/ovs-router.c
> @@ -173,90 +173,104 @@ ovs_router_lookup_fallback(const struct in6_addr
> *ip6_dst,
> return true;
> }
>
> +/* If src_in is set, src_out will be set based on it instead of the route
There is no 'src_in' in this fucntion anymore.
> + * entry. */
> bool
> ovs_router_lookup(uint32_t mark, const struct in6_addr *ip6_dst,
> + const struct in6_addr *ip6_src,
> char output_netdev[],
> - struct in6_addr *src, struct in6_addr *gw)
> + struct in6_addr *src_out, struct in6_addr *gw)
> {
> - struct flow flow = {.ipv6_dst = *ip6_dst, .pkt_mark = mark};
> - const struct in6_addr *from_src = src;
> - const struct cls_rule *cr = NULL;
> + bool is_ipv4 = IN6_IS_ADDR_V4MAPPED(ip6_dst);
> + ovs_be16 dl_type = is_ipv4 ? htons(ETH_TYPE_IP) : htons(ETH_TYPE_IPV6);
> + const struct cls_rule *cr;
> struct router_rule *rule;
> + struct classifier *cls;
> + struct flow flow;
>
> - 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 (!cls_local) {
> + if (ip6_src) {
> + if (is_ipv4 != IN6_IS_ADDR_V4MAPPED(ip6_src)) {
> return false;
> }
>
> - cr_src = classifier_lookup(cls_local, OVS_VERSION_MAX, &flow_src,
> - NULL, NULL);
> - if (!cr_src) {
> + 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};
> + }
> +
> + cls = cls_find(CLS_LOCAL);
> +
> + if (!cls) {
> return false;
> }
> - }
>
> - if (!from_src) {
> - if (IN6_IS_ADDR_V4MAPPED(ip6_dst)) {
> - from_src = &in6addr_v4mapped_any;
> + cr = classifier_lookup(cls, OVS_VERSION_MAX, &flow, NULL, NULL);
> + if (!cr) {
> + return false;
> + }
> + if (src_out) {
> + *src_out = *ip6_src;
> + src_out = NULL;
> + }
> + } else {
> + if (is_ipv4) {
> + ip6_src = &in6addr_v4mapped_any;
> } else {
> - from_src = &in6addr_any;
> + ip6_src = &in6addr_any;
> }
> }
>
> + if (is_ipv4) {
> + flow = (struct flow) {.nw_dst = in6_addr_get_mapped_ipv4(ip6_dst),
> + .pkt_mark = mark, .dl_type = dl_type};
> + } else {
> + flow = (struct flow) {.ipv6_dst = *ip6_dst, .pkt_mark = mark,
> + .dl_type = dl_type};
> + }
> +
> PVECTOR_FOR_EACH (rule, &rules) {
> 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;
> }
>
> matched = (!rule->src_prefix ||
> - ipv6_addr_equals_masked(&rule->from_addr, from_src,
> plen));
> + ipv6_addr_equals_masked(&rule->from_addr, ip6_src, plen));
>
> if (rule->invert) {
> matched = !matched;
> }
>
> - if (matched) {
> - struct classifier *cls = cls_find(rule->lookup_table);
> + if (!matched) {
> + continue;
> + }
> + cls = cls_find(rule->lookup_table);
>
> - if (!cls) {
> - /* A rule can be added before the table is created. */
> - continue;
> - }
> - cr = classifier_lookup(cls, OVS_VERSION_MAX, &flow, NULL,
> - NULL);
> - 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.
> - */
> - if (!IN6_IS_ADDR_V4MAPPED(ip6_dst) ||
> - IN6_IS_ADDR_V4MAPPED(&p->nw_addr)) {
> - break;
> - }
> - }
> + if (!cls) {
> + /* A rule can be added before the table is created. */
> + continue;
> + }
> + cr = classifier_lookup(cls, OVS_VERSION_MAX, &flow, NULL, NULL);
> + if (!cr) {
> + continue;
> }
> - }
>
> - if (cr) {
> struct ovs_router_entry *p = ovs_router_entry_cast(cr);
>
> ovs_strlcpy(output_netdev, p->output_netdev, IFNAMSIZ);
> *gw = p->gw;
> - if (src && !ipv6_addr_is_set(src)) {
> - *src = p->src_addr;
> + if (src_out) {
> + *src_out = p->src_addr;
> }
> return true;
> }
> - return ovs_router_lookup_fallback(ip6_dst, output_netdev, src, gw);
> +
> + return ovs_router_lookup_fallback(ip6_dst, output_netdev, src_out, gw);
> }
>
> static void
> @@ -270,17 +284,34 @@ static void rt_init_match(struct match *match, uint32_t
> mark,
> const struct in6_addr *ip6_dst,
> uint8_t plen)
> {
> - struct in6_addr dst;
> - struct in6_addr mask;
> -
> - mask = ipv6_create_mask(plen);
>
> - dst = ipv6_addr_bitand(ip6_dst, &mask);
> memset(match, 0, sizeof *match);
This function grew too much for us to not use the proper API.
This should probably become the match_init_catchall(match).
> - match->flow.ipv6_dst = dst;
> - match->wc.masks.ipv6_dst = mask;
> match->wc.masks.pkt_mark = UINT32_MAX;
> + match->wc.masks.dl_type = htons(UINT16_MAX);
OVS_BE16_MAX, but may also delay this to the per-family match_set.
> match->flow.pkt_mark = mark;
match_set_pkt_mark(match, mark);
> +
> + if (IN6_IS_ADDR_V4MAPPED(ip6_dst)) {
> + ovs_be32 dst = in6_addr_get_mapped_ipv4(ip6_dst);
> + ovs_be32 mask = OVS_BE32_MAX;
> +
> + plen -= 96;
> + if (plen < 32) {
> + mask ^= htonl(0xFFFFFFFF >> plen);
be32_prefix_mask(plen - 96) ?
Also, do we need that 'if'? It's an optimization for /32, right?
Not sure if that's important. This function is not used in packet
processing to care about a couple cycles in a special case.
> + }
> +
> + match->flow.nw_dst = dst & mask;
> + match->flow.dl_type = htons(ETH_TYPE_IP);
> + match->wc.masks.nw_dst = mask;
match_set_dl_type(match, htons(ETH_TYPE_IP));
match_set_nw_dst_masked(match, in6_addr_get_mapped_ipv4(ip6_dst),
be32_prefix_mask(plen - 96));
> + } else {
> + struct in6_addr dst;
> + struct in6_addr mask;
> +
> + mask = ipv6_create_mask(plen);
> + dst = ipv6_addr_bitand(ip6_dst, &mask);
> + match->flow.ipv6_dst = dst;
> + match->flow.dl_type = htons(ETH_TYPE_IPV6);
> + match->wc.masks.ipv6_dst = mask;
match_set_dl_type(match, htons(ETH_TYPE_IPV6));
match_set_ipv6_dst_masked(match, ip6_dst, ipv6_create_mask(plen));
> + }
> }
>
> static int
> @@ -396,7 +427,11 @@ ovs_router_insert__(uint32_t table, uint32_t mark,
> uint8_t priority,
> p->gw = *gw;
> }
> p->mark = mark;
> - p->nw_addr = match.flow.ipv6_dst;
> + if (match.flow.dl_type == htons(ETH_TYPE_IPV6)) {
> + p->nw_addr = match.flow.ipv6_dst;
> + } else {
> + in6_addr_set_mapped_ipv4(&p->nw_addr, match.flow.nw_dst);
> + }
> p->plen = plen;
> p->user = user;
> p->priority = priority;
> @@ -1129,7 +1164,8 @@ static void
> ovs_router_lookup_cmd(struct unixctl_conn *conn, int argc,
> const char *argv[], void *aux OVS_UNUSED)
> {
> - struct in6_addr gw, src6 = in6addr_any;
> + struct in6_addr gw, src_out, src6;
> + struct in6_addr *src_in = NULL;
> char src6_s[IPV6_SCAN_LEN + 1];
> char iface[IFNAMSIZ];
> struct in6_addr ip6;
> @@ -1159,10 +1195,13 @@ ovs_router_lookup_cmd(struct unixctl_conn *conn, int
> argc,
> if (is_ipv6) {
> if (ovs_scan(argv[i], "src="IPV6_SCAN_FMT, src6_s) &&
> ipv6_parse(src6_s, &src6)) {
> + src_in = &src6;
> continue;
> }
> } else {
> if (ovs_scan(argv[i], "src="IP_SCAN_FMT, IP_SCAN_ARGS(&src))) {
> + in6_addr_set_mapped_ipv4(&src6, src);
> + src_in = &src6;
> continue;
> }
> }
> @@ -1171,15 +1210,11 @@ ovs_router_lookup_cmd(struct unixctl_conn *conn, int
> argc,
> return;
> }
>
> - if (src) {
> - in6_addr_set_mapped_ipv4(&src6, src);
> - }
> -
> - if (ovs_router_lookup(mark, &ip6, iface, &src6, &gw)) {
> + if (ovs_router_lookup(mark, &ip6, src_in, iface, &src_out, &gw)) {
> struct ds ds = DS_EMPTY_INITIALIZER;
>
> ds_put_format(&ds, "src ");
> - ipv6_format_mapped(&src6, &ds);
> + ipv6_format_mapped(&src_out, &ds);
> ds_put_format(&ds, "\ngateway ");
> ipv6_format_mapped(&gw, &ds);
> ds_put_format(&ds, "\ndev %s\n", iface);
> diff --git a/lib/ovs-router.h b/lib/ovs-router.h
> index bd1ab7a9a..ff465a7c7 100644
> --- a/lib/ovs-router.h
> +++ b/lib/ovs-router.h
> @@ -34,7 +34,7 @@ enum {
> };
>
> bool ovs_router_lookup(uint32_t mark, const struct in6_addr *ip_dst,
> - char output_netdev[],
> + const struct in6_addr *src_in, char output_netdev[],
> struct in6_addr *src, struct in6_addr *gw);
Names changed.
> void ovs_router_init(void);
> bool ovs_router_is_referenced(uint32_t table);
> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
> index e043d7cbc..27e5955a6 100644
> --- a/ofproto/ofproto-dpif-sflow.c
> +++ b/ofproto/ofproto-dpif-sflow.c
> @@ -482,7 +482,7 @@ sflow_choose_agent_address(const char *agent_device,
>
> struct in6_addr gw, src = in6addr_any;
> char name[IFNAMSIZ];
> - if (ovs_router_lookup(0, &target_ip, name, &src, &gw)) {
> + if (ovs_router_lookup(0, &target_ip, NULL, name, &src, &gw)) {
> ip = src;
> goto success;
> }
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 764dbd662..cb4467708 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3691,16 +3691,28 @@ process_special(struct xlate_ctx *ctx, const struct
> xport *xport)
> static int
> tnl_route_lookup_flow(const struct xlate_ctx *ctx,
> const struct flow *oflow,
> - struct in6_addr *ip, struct in6_addr *src,
> + struct in6_addr *ip, struct in6_addr *out_src,
> struct xport **out_port)
> {
> - char out_dev[IFNAMSIZ];
> struct xbridge *xbridge;
> - struct in6_addr gw;
> + char out_dev[IFNAMSIZ];
> + struct in6_addr in_src;
> struct in6_addr dst;
> + struct in6_addr gw;
> + bool have_src = false;
> +
> + /* Use source address for the route lookup, if provided. */
> + if (oflow->tunnel.ip_src) {
> + in6_addr_set_mapped_ipv4(&in_src, oflow->tunnel.ip_src);
> + have_src = true;
> + } else if (ipv6_addr_is_set(&oflow->tunnel.ipv6_src)) {
> + in_src = oflow->tunnel.ipv6_src;
> + have_src = true;
> + }
>
> dst = flow_tnl_dst(&oflow->tunnel);
> - if (!ovs_router_lookup(oflow->pkt_mark, &dst, out_dev, src, &gw)) {
> + if (!ovs_router_lookup(oflow->pkt_mark, &dst, have_src ? &in_src : NULL,
> + out_dev, out_src, &gw)) {
in/out are prefixes here, but suffixes in ovs_router_lookup_cmd. While these
are different functions, it may be better to use unified names.
> return -ENOENT;
> }
>
> @@ -3890,8 +3902,8 @@ native_tunnel_output(struct xlate_ctx *ctx, const
> struct xport *xport,
> struct ovs_action_push_tnl tnl_push_data;
> struct xport *out_dev = NULL;
> ovs_be32 s_ip = 0, d_ip = 0;
> - struct in6_addr s_ip6 = in6addr_any;
> - struct in6_addr d_ip6 = in6addr_any;
> + struct in6_addr s_ip6;
> + struct in6_addr d_ip6;
> struct eth_addr smac;
> struct eth_addr dmac;
> int err;
> @@ -3909,12 +3921,6 @@ native_tunnel_output(struct xlate_ctx *ctx, const
> struct xport *xport,
> memcpy(&old_base_flow, &ctx->base_flow, sizeof old_base_flow);
> memcpy(&old_flow, &ctx->xin->flow, sizeof old_flow);
>
> - if (flow->tunnel.ip_src) {
> - in6_addr_set_mapped_ipv4(&s_ip6, flow->tunnel.ip_src);
> - } else if (ipv6_addr_is_set(&flow->tunnel.ipv6_src)) {
> - s_ip6 = flow->tunnel.ipv6_src;
> - }
> -
> err = tnl_route_lookup_flow(ctx, flow, &d_ip6, &s_ip6, &out_dev);
> if (err) {
> put_cloned_drop_action(ctx->xbridge->ofproto, ctx->odp_actions,
> diff --git a/tests/ovs-router.at b/tests/ovs-router.at
> index 97eb95fad..a45f09db4 100644
> --- a/tests/ovs-router.at
> +++ b/tests/ovs-router.at
> @@ -310,6 +310,28 @@ 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], [OK
> +])
> +AT_CHECK([ovs-appctl ovs/route/add 2.2.2.3/32 br0 192.0.2.1 table=15], [0],
> [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], [OK
> +])
> +AT_CHECK([ovs-appctl ovs/route/rule/del from=all table=15], [0], [OK
> +])
> +
> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
