[ovs-dev] [PATCH 7/9] ofproto-dpif-xlate: Generate right mask when checking prereqs.

Daniele Di Proietto diproiettod at vmware.com
Thu Dec 10 02:27:40 UTC 2015


During translation we need to unwildcard each member of the flow that we
look at.  When setting or moving a field, we need to look at (and
consequently unwildcard) the field itself an all the prerequisites.

The current code unwildcards the field and all the prerequisites and
then reads them.  This behavior is wrong, because if prerequisites are
not met we might end up unwildcarding more fields than necessary,
generating masks that don't make sense.

The bug can be triggered with an action that sets the tcp src port and
a fragmented IP packet.  The translation will not generate a set action,
but it will match on the target field (tcp src port), even though the
prerequisites for that are not met.

The wrong mask causes the flow to get deleted by revalidation.

Fix this by introducing mf_check_prereqs_and_set_mask(), which checks
the prerequisites while masking the field. It is based on the behavior
of mf_are_prereqs_ok().

mf_mask_field_and_prereqs() is not used anymore, so it can be removed.
mf_are_prereqs_ok() can be rewritten using
mf_check_prereqs_and_set_mask(), avoiding code duplication

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
---
 lib/flow.h                   |  38 +++++++++++++--
 lib/meta-flow.c              | 113 +++++++++++++++++++------------------------
 lib/meta-flow.h              |   5 +-
 lib/nx-match.c               |  10 ++--
 ofproto/ofproto-dpif-xlate.c |   4 +-
 5 files changed, 92 insertions(+), 78 deletions(-)

diff --git a/lib/flow.h b/lib/flow.h
index 5d78615..6f99297 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -999,21 +999,49 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const struct flow *flow)
     md->ct_label = flow->ct_label;
 }
 
+/* Often, during translation we need to read a value from a flow and
+ * unwildcard the corresponding bits in the wildcards. */
+
+#define FLOW_WC_GET_AND_MASK(FLOW, WC, FIELD) \
+    (((WC) ? WC_MASK_FIELD(WC, FIELD) : NULL), (FLOW)->FIELD)
+
+#define FLOW_WC_GET_AND_MASK_MASKED(FLOW, WC, FIELD, MASK) \
+    (((WC) ? WC_MASK_FIELD_MASK(WC, FIELD, MASK) : 0), \
+     (((FLOW)->FIELD) & (MASK)))
+
+static inline bool check_and_mask_ip_any(const struct flow *flow,
+                                         struct flow_wildcards *wc)
+{
+    return dl_type_is_ip_any(FLOW_WC_GET_AND_MASK(flow, wc, dl_type));
+}
+
+static inline bool check_and_mask_icmpv4(const struct flow *flow,
+                                         struct flow_wildcards *wc)
+{
+    return (FLOW_WC_GET_AND_MASK(flow, wc, dl_type) == htons(ETH_TYPE_IP)
+            && FLOW_WC_GET_AND_MASK(flow, wc, nw_proto) == IPPROTO_ICMP);
+}
+
+static inline bool check_and_mask_icmpv6(const struct flow *flow,
+                                         struct flow_wildcards *wc)
+{
+    return (FLOW_WC_GET_AND_MASK(flow, wc, dl_type) == htons(ETH_TYPE_IPV6)
+            && FLOW_WC_GET_AND_MASK(flow, wc, nw_proto) == IPPROTO_ICMPV6);
+}
+
 static inline bool is_ip_any(const struct flow *flow)
 {
-    return dl_type_is_ip_any(flow->dl_type);
+    return check_and_mask_ip_any(flow, NULL);
 }
 
 static inline bool is_icmpv4(const struct flow *flow)
 {
-    return (flow->dl_type == htons(ETH_TYPE_IP)
-            && flow->nw_proto == IPPROTO_ICMP);
+    return check_and_mask_icmpv4(flow, NULL);
 }
 
 static inline bool is_icmpv6(const struct flow *flow)
 {
-    return (flow->dl_type == htons(ETH_TYPE_IPV6)
-            && flow->nw_proto == IPPROTO_ICMPV6);
+    return check_and_mask_icmpv6(flow, NULL);
 }
 
 static inline bool is_igmp(const struct flow *flow)
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 6bd0b99..398b2f2 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -356,94 +356,79 @@ mf_is_mask_valid(const struct mf_field *mf, const union mf_value *mask)
 bool
 mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow)
 {
+    return mf_check_prereqs_and_set_mask(mf, flow, NULL);
+}
+
+/* Returns true if 'flow' meets the prerequisites for 'mf', false otherwise.
+ * If an attribute is read from 'flow', the corresponding attribute in 'wc'
+ * will be unwildcarded. */
+bool
+mf_check_prereqs_and_set_mask(const struct mf_field *mf,
+                              const struct flow *flow,
+                              struct flow_wildcards *wc)
+{
     switch (mf->prereqs) {
     case MFP_NONE:
         return true;
 
     case MFP_ARP:
-      return (flow->dl_type == htons(ETH_TYPE_ARP) ||
-              flow->dl_type == htons(ETH_TYPE_RARP));
+        return FLOW_WC_GET_AND_MASK(flow, wc, dl_type) == htons(ETH_TYPE_ARP)
+            || FLOW_WC_GET_AND_MASK(flow, wc, dl_type) == htons(ETH_TYPE_RARP);
     case MFP_IPV4:
-        return flow->dl_type == htons(ETH_TYPE_IP);
+        return FLOW_WC_GET_AND_MASK(flow, wc, dl_type) == htons(ETH_TYPE_IP);
     case MFP_IPV6:
-        return flow->dl_type == htons(ETH_TYPE_IPV6);
+        return FLOW_WC_GET_AND_MASK(flow, wc, dl_type) == htons(ETH_TYPE_IPV6);
     case MFP_VLAN_VID:
-        return (flow->vlan_tci & htons(VLAN_CFI)) != 0;
+        return FLOW_WC_GET_AND_MASK_MASKED(flow, wc,
+                                           vlan_tci, htons(VLAN_CFI)) != 0;
     case MFP_MPLS:
-        return eth_type_mpls(flow->dl_type);
+        return eth_type_mpls(FLOW_WC_GET_AND_MASK(flow, wc, dl_type));
     case MFP_IP_ANY:
-        return is_ip_any(flow);
-
+        return check_and_mask_ip_any(flow, wc);
     case MFP_TCP:
-        return is_ip_any(flow) && flow->nw_proto == IPPROTO_TCP
-            && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
+        return check_and_mask_ip_any(flow, wc)
+            && FLOW_WC_GET_AND_MASK(flow, wc, nw_proto) == IPPROTO_TCP
+            && !FLOW_WC_GET_AND_MASK_MASKED(flow, wc, nw_frag,
+                                            FLOW_NW_FRAG_LATER);
     case MFP_UDP:
-        return is_ip_any(flow) && flow->nw_proto == IPPROTO_UDP
-            && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
+        return check_and_mask_ip_any(flow, wc)
+            && FLOW_WC_GET_AND_MASK(flow, wc, nw_proto) == IPPROTO_UDP
+            && !FLOW_WC_GET_AND_MASK_MASKED(flow, wc, nw_frag,
+                                            FLOW_NW_FRAG_LATER);
     case MFP_SCTP:
-        return is_ip_any(flow) && flow->nw_proto == IPPROTO_SCTP
-            && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
+        return check_and_mask_ip_any(flow, wc)
+            && FLOW_WC_GET_AND_MASK(flow, wc, nw_proto) == IPPROTO_SCTP
+            && !FLOW_WC_GET_AND_MASK_MASKED(flow, wc, nw_frag,
+                                            FLOW_NW_FRAG_LATER);
     case MFP_ICMPV4:
-        return is_icmpv4(flow);
+        return check_and_mask_icmpv4(flow, wc);
     case MFP_ICMPV6:
-        return is_icmpv6(flow);
+        return check_and_mask_icmpv6(flow, wc);
 
     case MFP_ND:
-        return (is_icmpv6(flow)
-                && flow->tp_dst == htons(0)
-                && (flow->tp_src == htons(ND_NEIGHBOR_SOLICIT) ||
-                    flow->tp_src == htons(ND_NEIGHBOR_ADVERT)));
+        return check_and_mask_icmpv6(flow, wc)
+            && FLOW_WC_GET_AND_MASK(flow, wc, tp_dst) == htons(0)
+            && (FLOW_WC_GET_AND_MASK(flow, wc, tp_src)
+                    == htons(ND_NEIGHBOR_SOLICIT)
+                || FLOW_WC_GET_AND_MASK(flow, wc, tp_src)
+                    == htons(ND_NEIGHBOR_ADVERT));
     case MFP_ND_SOLICIT:
-        return (is_icmpv6(flow)
-                && flow->tp_dst == htons(0)
-                && (flow->tp_src == htons(ND_NEIGHBOR_SOLICIT)));
+        return check_and_mask_icmpv6(flow, wc)
+            && FLOW_WC_GET_AND_MASK(flow, wc, tp_dst)
+                    == htons(0)
+            && FLOW_WC_GET_AND_MASK(flow, wc, tp_src)
+                    == htons(ND_NEIGHBOR_SOLICIT);
     case MFP_ND_ADVERT:
-        return (is_icmpv6(flow)
-                && flow->tp_dst == htons(0)
-                && (flow->tp_src == htons(ND_NEIGHBOR_ADVERT)));
+        return check_and_mask_icmpv6(flow, wc)
+            && FLOW_WC_GET_AND_MASK(flow, wc, tp_dst)
+                    == htons(0)
+            && FLOW_WC_GET_AND_MASK(flow, wc, tp_src)
+                    == htons(ND_NEIGHBOR_ADVERT);
     }
 
     OVS_NOT_REACHED();
 }
 
-/* Set field and it's prerequisities in the mask.
- * This is only ever called for writeable 'mf's, but we do not make the
- * distinction here. */
-void
-mf_mask_field_and_prereqs(const struct mf_field *mf, struct flow_wildcards *wc)
-{
-    mf_set_flow_value(mf, &exact_match_mask, &wc->masks);
-
-    switch (mf->prereqs) {
-    case MFP_ND:
-    case MFP_ND_SOLICIT:
-    case MFP_ND_ADVERT:
-        WC_MASK_FIELD(wc, tp_src);
-        WC_MASK_FIELD(wc, tp_dst);
-        /* Fall through. */
-    case MFP_TCP:
-    case MFP_UDP:
-    case MFP_SCTP:
-    case MFP_ICMPV4:
-    case MFP_ICMPV6:
-        /* nw_frag always unwildcarded. */
-        WC_MASK_FIELD(wc, nw_proto);
-        /* Fall through. */
-    case MFP_ARP:
-    case MFP_IPV4:
-    case MFP_IPV6:
-    case MFP_MPLS:
-    case MFP_IP_ANY:
-        /* dl_type always unwildcarded. */
-        break;
-    case MFP_VLAN_VID:
-        WC_MASK_FIELD_MASK(wc, vlan_tci, htons(VLAN_CFI));
-        break;
-    case MFP_NONE:
-        break;
-    }
-}
-
 /* Set bits of 'bm' corresponding to the field 'mf' and it's prerequisities. */
 void
 mf_bitmap_set_field_and_prereqs(const struct mf_field *mf, struct mf_bitmap *bm)
diff --git a/lib/meta-flow.h b/lib/meta-flow.h
index 71c238d..1e6055b 100644
--- a/lib/meta-flow.h
+++ b/lib/meta-flow.h
@@ -1967,8 +1967,9 @@ void mf_get_mask(const struct mf_field *, const struct flow_wildcards *,
 
 /* Prerequisites. */
 bool mf_are_prereqs_ok(const struct mf_field *, const struct flow *);
-void mf_mask_field_and_prereqs(const struct mf_field *,
-                               struct flow_wildcards *);
+bool mf_check_prereqs_and_set_mask(const struct mf_field *mf,
+                                   const struct flow *flow,
+                                   struct flow_wildcards *wc);
 void mf_bitmap_set_field_and_prereqs(const struct mf_field *mf, struct
                                      mf_bitmap *bm);
 
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 11bcd95..f56c9e6 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -1605,13 +1605,13 @@ nxm_execute_reg_move(const struct ofpact_reg_move *move,
     union mf_value src_value;
     union mf_value dst_value;
 
-    mf_mask_field_and_prereqs(move->dst.field, wc);
-    mf_mask_field_and_prereqs(move->src.field, wc);
-
     /* A flow may wildcard nw_frag.  Do nothing if setting a transport
      * header field on a packet that does not have them. */
-    if (mf_are_prereqs_ok(move->dst.field, flow)
-        && mf_are_prereqs_ok(move->src.field, flow)) {
+    if (mf_check_prereqs_and_set_mask(move->dst.field, flow, wc)
+        && mf_check_prereqs_and_set_mask(move->src.field, flow, wc)) {
+
+        mf_mask_field(move->dst.field, &wc->masks);
+        mf_mask_field(move->src.field, &wc->masks);
 
         mf_get_value(move->dst.field, flow, &dst_value);
         mf_get_value(move->src.field, flow, &src_value);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index dab64b9..08807bc 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4594,8 +4594,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             }
             /* A flow may wildcard nw_frag.  Do nothing if setting a transport
              * header field on a packet that does not have them. */
-            mf_mask_field_and_prereqs(mf, wc);
-            if (mf_are_prereqs_ok(mf, flow)) {
+            if (mf_check_prereqs_and_set_mask(mf, flow, wc)) {
+                mf_mask_field(mf, &wc->masks);
                 mf_set_flow_value_masked(mf, &set_field->value,
                                          &set_field->mask, flow);
             }
-- 
2.1.4




More information about the dev mailing list