On 9/22/26 3:18 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
> v5:
> - Convered rt_init_match to match_* functions
> - Renamed variables to in/out suffix
> - Updated prototypes and comments
> ---
> lib/flow.c | 2 +-
> lib/netdev-vport.c | 2 +-
> lib/ovs-router.c | 146 ++++++++++++++++++++---------------
> lib/ovs-router.h | 6 +-
> ofproto/ofproto-dpif-sflow.c | 2 +-
> ofproto/ofproto-dpif-xlate.c | 30 ++++---
> tests/ovs-router.at | 22 ++++++
> 7 files changed, 129 insertions(+), 81 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..f3ceb40e0 100644
> --- a/lib/ovs-router.c
> +++ b/lib/ovs-router.c
> @@ -173,90 +173,103 @@ ovs_router_lookup_fallback(const struct in6_addr
> *ip6_dst,
> return true;
> }
>
> +/* If ip6_src is set, ip6_dst will be set based on it instead of the route
> + * entry. */
This comment doesn't match the behavior. Should be out_src.
> bool
> ovs_router_lookup(uint32_t mark, const struct in6_addr *ip6_dst,
> - char output_netdev[],
> - struct in6_addr *src, struct in6_addr *gw)
> + const struct in6_addr *ip6_src, char output_netdev[],
> + struct in6_addr *out_src, 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 (out_src) {
> + *out_src = *ip6_src;
> + out_src = 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 (out_src) {
> + *out_src = 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, out_src, gw);
> }
>
> static void
> @@ -270,17 +283,20 @@ 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);
> + match_init_catchall(match);
> + match_set_pkt_mark(match, mark);
>
> - 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->flow.pkt_mark = mark;
> + if (IN6_IS_ADDR_V4MAPPED(ip6_dst)) {
> + 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));
Looks like this is will go negative is someone passes a mapped v4 address
with the prefix length below 96 into appctl. Maybe we can fail the parsing
for such input, i.e. fail the scan_ipv6_route() ? Having plen < 96 isn't
really meaningful and should not occur in any real use case.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
