ofproto-dpif-xlate: Fix IGMP megaflow matching.

IGMP translations wasn't setting enough bits in the wildcards to ensure
different packets were handled differently.

Reported-by: "O'Reilly, Darragh" <darragh.oreilly@hpe.com>
Reported-at: http://openvswitch.org/pipermail/discuss/2016-April/021036.html
Signed-off-by: Ben Pfaff <blp@ovn.org>

diff --git a/lib/flow.h b/lib/flow.h
index 0196ee7..22b245c 100644 (file)
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -832,41 +832,72 @@ static inline bool is_ip_any(const struct flow *flow)
     return dl_type_is_ip_any(flow->dl_type);
 }
 
-static inline bool is_icmpv4(const struct flow *flow)
+static inline bool is_icmpv4(const struct flow *flow,
+                             struct flow_wildcards *wc)
 {
-    return (flow->dl_type == htons(ETH_TYPE_IP)
-            && flow->nw_proto == IPPROTO_ICMP);
+    if (flow->dl_type == htons(ETH_TYPE_IP)) {
+        if (wc) {
+            memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
+        }
+        return flow->nw_proto == IPPROTO_ICMP;
+    }
+    return false;
 }
 
-static inline bool is_icmpv6(const struct flow *flow)
+static inline bool is_icmpv6(const struct flow *flow,
+                             struct flow_wildcards *wc)
 {
-    return (flow->dl_type == htons(ETH_TYPE_IPV6)
-            && flow->nw_proto == IPPROTO_ICMPV6);
+    if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
+        if (wc) {
+            memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
+        }
+        return flow->nw_proto == IPPROTO_ICMPV6;
+    }
+    return false;
 }
 
-static inline bool is_igmp(const struct flow *flow)
+static inline bool is_igmp(const struct flow *flow, struct flow_wildcards *wc)
 {
-    return (flow->dl_type == htons(ETH_TYPE_IP)
-            && flow->nw_proto == IPPROTO_IGMP);
+    if (flow->dl_type == htons(ETH_TYPE_IP)) {
+        if (wc) {
+            memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
+        }
+        return flow->nw_proto == IPPROTO_IGMP;
+    }
+    return false;
 }
 
-static inline bool is_mld(const struct flow *flow)
+static inline bool is_mld(const struct flow *flow,
+                          struct flow_wildcards *wc)
 {
-    return is_icmpv6(flow)
-           && (flow->tp_src == htons(MLD_QUERY)
-               || flow->tp_src == htons(MLD_REPORT)
-               || flow->tp_src == htons(MLD_DONE)
-               || flow->tp_src == htons(MLD2_REPORT));
+    if (is_icmpv6(flow, wc)) {
+        if (wc) {
+            memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
+        }
+        return (flow->tp_src == htons(MLD_QUERY)
+                || flow->tp_src == htons(MLD_REPORT)
+                || flow->tp_src == htons(MLD_DONE)
+                || flow->tp_src == htons(MLD2_REPORT));
+    }
+    return false;
 }
 
-static inline bool is_mld_query(const struct flow *flow)
+static inline bool is_mld_query(const struct flow *flow,
+                                struct flow_wildcards *wc)
 {
-    return is_icmpv6(flow) && flow->tp_src == htons(MLD_QUERY);
+    if (is_icmpv6(flow, wc)) {
+        if (wc) {
+            memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
+        }
+        return flow->tp_src == htons(MLD_QUERY);
+    }
+    return false;
 }
 
-static inline bool is_mld_report(const struct flow *flow)
+static inline bool is_mld_report(const struct flow *flow,
+                                 struct flow_wildcards *wc)
 {
-    return is_mld(flow) && !is_mld_query(flow);
+    return is_mld(flow, wc) && !is_mld_query(flow, wc);
 }
 
 static inline bool is_stp(const struct flow *flow)

diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 2a129ea..136295d 100644 (file)
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -393,21 +393,21 @@ mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow)
         return is_ip_any(flow) && flow->nw_proto == IPPROTO_SCTP
             && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
     case MFP_ICMPV4:
-        return is_icmpv4(flow);
+        return is_icmpv4(flow, NULL);
     case MFP_ICMPV6:
-        return is_icmpv6(flow);
+        return is_icmpv6(flow, NULL);
 
     case MFP_ND:
-        return (is_icmpv6(flow)
+        return (is_icmpv6(flow, NULL)
                 && flow->tp_dst == htons(0)
                 && (flow->tp_src == htons(ND_NEIGHBOR_SOLICIT) ||
                     flow->tp_src == htons(ND_NEIGHBOR_ADVERT)));
     case MFP_ND_SOLICIT:
-        return (is_icmpv6(flow)
+        return (is_icmpv6(flow, NULL)
                 && flow->tp_dst == htons(0)
                 && (flow->tp_src == htons(ND_NEIGHBOR_SOLICIT)));
     case MFP_ND_ADVERT:
-        return (is_icmpv6(flow)
+        return (is_icmpv6(flow, NULL)
                 && flow->tp_dst == htons(0)
                 && (flow->tp_src == htons(ND_NEIGHBOR_ADVERT)));
     }

diff --git a/lib/nx-match.c b/lib/nx-match.c
index 3c48570..aad6e02 100644 (file)
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -861,7 +861,7 @@ nxm_put_ip(struct ofpbuf *b, const struct match *match, enum ofp_version oxm)
                         match->wc.masks.tp_src);
             nxm_put_16m(b, MFF_SCTP_DST, oxm, flow->tp_dst,
                         match->wc.masks.tp_dst);
-        } else if (is_icmpv4(flow)) {
+        } else if (is_icmpv4(flow, NULL)) {
             if (match->wc.masks.tp_src) {
                 nxm_put_8(b, MFF_ICMPV4_TYPE, oxm,
                           ntohs(flow->tp_src));
@@ -870,7 +870,7 @@ nxm_put_ip(struct ofpbuf *b, const struct match *match, enum ofp_version oxm)
                 nxm_put_8(b, MFF_ICMPV4_CODE, oxm,
                           ntohs(flow->tp_dst));
             }
-        } else if (is_icmpv6(flow)) {
+        } else if (is_icmpv6(flow, NULL)) {
             if (match->wc.masks.tp_src) {
                 nxm_put_8(b, MFF_ICMPV6_TYPE, oxm,
                           ntohs(flow->tp_src));

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 2cdd504..d9ace90 100644 (file)
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -5719,9 +5719,9 @@ commit_set_icmp_action(const struct flow *flow, struct flow *base_flow,
     struct ovs_key_icmp key, mask, base;
     enum ovs_key_attr attr;
 
-    if (is_icmpv4(flow)) {
+    if (is_icmpv4(flow, NULL)) {
         attr = OVS_KEY_ATTR_ICMP;
-    } else if (is_icmpv6(flow)) {
+    } else if (is_icmpv6(flow, NULL)) {
         attr = OVS_KEY_ATTR_ICMPV6;
     } else {
         return 0;

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9ccad5e..9aa69ac 100644 (file)
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2388,6 +2388,20 @@ xlate_normal_flood(struct xlate_ctx *ctx, struct xbundle *in_xbundle,
     ctx->nf_output_iface = NF_OUT_FLOOD;
 }
 
+static bool
+is_ip_local_multicast(const struct flow *flow, struct flow_wildcards *wc)
+{
+    if (flow->dl_type == htons(ETH_TYPE_IP)) {
+        memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
+        return ip_is_local_multicast(flow->nw_dst);
+    } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
+        memset(&wc->masks.ipv6_dst, 0xff, sizeof wc->masks.ipv6_dst);
+        return ipv6_is_all_hosts(&flow->ipv6_dst);
+    } else {
+        return false;
+    }
+}
+
 static void
 xlate_normal(struct xlate_ctx *ctx)
 {
@@ -2471,7 +2485,8 @@ xlate_normal(struct xlate_ctx *ctx)
         struct mcast_snooping *ms = ctx->xbridge->ms;
         struct mcast_group *grp = NULL;
 
-        if (is_igmp(flow)) {
+        if (is_igmp(flow, wc)) {
+            memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
             if (mcast_snooping_is_membership(flow->tp_src) ||
                 mcast_snooping_is_query(flow->tp_src)) {
                 if (ctx->xin->may_learn && ctx->xin->packet) {
@@ -2504,13 +2519,13 @@ xlate_normal(struct xlate_ctx *ctx)
                 xlate_normal_flood(ctx, in_xbundle, vlan);
             }
             return;
-        } else if (is_mld(flow)) {
+        } else if (is_mld(flow, wc)) {
             ctx->xout->slow |= SLOW_ACTION;
             if (ctx->xin->may_learn && ctx->xin->packet) {
                 update_mcast_snooping_table(ctx->xbridge, flow, vlan,
                                             in_xbundle, ctx->xin->packet);
             }
-            if (is_mld_report(flow)) {
+            if (is_mld_report(flow, wc)) {
                 ovs_rwlock_rdlock(&ms->rwlock);
                 xlate_normal_mcast_send_mrouters(ctx, ms, in_xbundle, vlan);
                 xlate_normal_mcast_send_rports(ctx, ms, in_xbundle, vlan);
@@ -2520,10 +2535,7 @@ xlate_normal(struct xlate_ctx *ctx)
                 xlate_normal_flood(ctx, in_xbundle, vlan);
             }
         } else {
-            if ((flow->dl_type == htons(ETH_TYPE_IP)
-                 && ip_is_local_multicast(flow->nw_dst))
-                || (flow->dl_type == htons(ETH_TYPE_IPV6)
-                    && ipv6_is_all_hosts(&flow->ipv6_dst))) {
+            if (is_ip_local_multicast(flow, wc)) {
                 /* RFC4541: section 2.1.2, item 2: Packets with a dst IP
                  * address in the 224.0.0.x range which are not IGMP must
                  * be forwarded on all ports */
@@ -5066,7 +5078,7 @@ xlate_wc_finish(struct xlate_ctx *ctx)
      * Avoid the problem here by making sure that only the low 8 bits of
      * either field can be unwildcarded for ICMP.
      */
-    if (is_icmpv4(&ctx->xin->flow) || is_icmpv6(&ctx->xin->flow)) {
+    if (is_icmpv4(&ctx->xin->flow, NULL) || is_icmpv6(&ctx->xin->flow, NULL)) {
         ctx->wc->masks.tp_src &= htons(UINT8_MAX);
         ctx->wc->masks.tp_dst &= htons(UINT8_MAX);
     }