| Message ID | 20260827174627.2594602-1-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v4] route-table: Correct handling of src addr in lookup. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_FreeBSD_Build_and_Test | success | github build: passed |
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 <mkp@redhat.com> > --- > 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
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 + * 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); - 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); match->flow.pkt_mark = 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); + } + + match->flow.nw_dst = dst & mask; + match->flow.dl_type = htons(ETH_TYPE_IP); + match->wc.masks.nw_dst = mask; + } 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; + } } 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); 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)) { 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
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 <mkp@redhat.com> --- 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(-)