[ovs-dev] [PATCH] nx-match: Log a warning when a wildcarded bit is set to 1.

Ben Pfaff blp at nicira.com
Wed Oct 3 16:48:57 UTC 2012


This was prompted by a conversation on the openflow-discuss mailing list
where developers of some OpenFlow switches mentioned that they save an
entire copy of raw flows passed in by controllers because of the
possibility that there might be wildcarded 1-bits, e.g. something like
192.168.1.1/255.255.0.0 instead of 192.168.0.0/255.255.0.0.  I've always
intended that this not be necessary, but it was never explicitly written
down.  This commit starts the process of updating OVS to make this a
requirement, by logging a warning whenever such a NXM or OXM entry is seen,
and by updating the spec in nicira-ext.h to describe my intent.

I intend to also file a bug report in the ONF tracker for OpenFlow to get
a similar clarification into OpenFlow 1.4.   (I don't intend to make this
an error in Open vSwitch unless ONF actually accepts this change.)

Thanks to Zoltán Lajos Kis, Dan Talayco, Rob Sherwood, and HIDEyuki
Shimonishi for participating in the discussion on openflow-discuss.

Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 include/openflow/nicira-ext.h |    7 ++++---
 lib/nx-match.c                |   26 ++++++++++++++++++++++++++
 tests/ovs-ofctl.at            |   23 +++++++++++++++++++++--
 3 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index 75bf6db..5d24e8e 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -1204,9 +1204,10 @@ OFP_ASSERT(sizeof(struct nx_action_output_reg) == 24);
  *     value, called "nxm_mask".  For each 1-bit in position J in nxm_mask, the
  *     nx_match matches only packets for which bit J in the given field's value
  *     matches bit J in nxm_value.  A 0-bit in nxm_mask causes the
- *     corresponding bits in nxm_value and the field's value to be ignored.
- *     (The sense of the nxm_mask bits is the opposite of that used by the
- *     "wildcards" member of struct ofp10_match.)
+ *     corresponding bit in nxm_value is ignored (it should be 0; Open vSwitch
+ *     may enforce this someday), as is the corresponding bit in the field's
+ *     value.  (The sense of the nxm_mask bits is the opposite of that used by
+ *     the "wildcards" member of struct ofp10_match.)
  *
  *     When nxm_hasmask is 1, nxm_length is always even.
  *
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 4254747..5139733 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -90,6 +90,31 @@ nx_entry_ok(const void *p, unsigned int match_len)
     return header;
 }
 
+/* Given NXM/OXM value 'value' and mask 'mask', each 'width' bytes long,
+ * checks for any 1-bit in the value where there is a 0-bit in the mask.  If it
+ * finds one, logs a warning. */
+static void
+check_mask_consistency(const uint8_t *p, const struct mf_field *mf)
+{
+    unsigned int width = mf->n_bytes;
+    const uint8_t *value = p + 4;
+    const uint8_t *mask = p + 4 + width;
+    unsigned int i;
+
+    for (i = 0; i < width; i++) {
+        if (value[i] & ~mask[i]) {
+            if (!VLOG_DROP_WARN(&rl)) {
+                char *s = nx_match_to_string(p, width * 2 + 4);
+                VLOG_WARN_RL(&rl, "NXM/OXM entry %s has 1-bits in value for "
+                             "bits wildcarded by the mask.  (Future versions "
+                             "of OVS may report this as an OpenFlow error.)",
+                             s);
+                break;
+            }
+        }
+    }
+}
+
 static enum ofperr
 nx_pull_raw(const uint8_t *p, unsigned int match_len, bool strict,
             struct match *match, ovs_be64 *cookie, ovs_be64 *cookie_mask)
@@ -141,6 +166,7 @@ nx_pull_raw(const uint8_t *p, unsigned int match_len, bool strict,
                     error = OFPERR_OFPBMC_BAD_MASK;
                 } else {
                     error = 0;
+                    check_mask_consistency(p, mf);
                     mf_set(mf, &value, &mask, match);
                 }
             }
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index 8185827..4e4f611 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -477,7 +477,7 @@ NXM_NX_REG0_W(a0e0d050/00000000)
 00011e04(12345678)
 00011f08(12345678/12345678)
 ])
-AT_CHECK([ovs-ofctl --strict parse-nx-match < nx-match.txt], [0], [dnl
+AT_CHECK([ovs-ofctl -vPATTERN:'console:%c|%p|%m' --strict parse-nx-match < nx-match.txt], [0], [dnl
 <any>
 
 # in port
@@ -728,7 +728,16 @@ NXM_NX_REG0(12345678)
 NXM_NX_REG0_W(12345678/12345678)
 nx_pull_match() returned error OFPBMC_BAD_FIELD
 nx_pull_match() returned error OFPBMC_BAD_FIELD
+], [stderr])
+
+# Check that at least the first warning made it.  (It's rate-limited
+# so a variable number could show up, especially under valgrind etc.)
+AT_CHECK([grep 'has 1-bits in value' stderr | sed 1q], [0], [dnl
+nx_match|WARN|NXM/OXM entry NXM_OF_ETH_DST_W(ffffffffffff/010000000000) has 1-bits in value for bits wildcarded by the mask.  (Future versions of OVS may report this as an OpenFlow error.)
 ])
+
+# Check that there wasn't any other stderr output.
+AT_CHECK([grep -v 'has 1-bits in value' stderr], [1])
 AT_CLEANUP
 
 AT_SETUP([ovs-ofctl parse-ofp10-match])
@@ -1478,7 +1487,8 @@ OXM_OF_ETH_TYPE(0800) OXM_OF_IP_PROTO(3a) OXM_OF_ICMPV6_TYPE(88) OXM_OF_IPV6_ND_
 # Invalid field number.
 01020304(1111/2222)
 ])
-AT_CHECK([ovs-ofctl --strict parse-oxm < oxm.txt], [0], [dnl
+AT_CHECK([ovs-ofctl '-vPATTERN:console:%c|%p|%m' --strict parse-oxm < oxm.txt],
+  [0], [dnl
 <any>
 
 # in port
@@ -1672,7 +1682,16 @@ nx_pull_match() returned error OFPBMC_BAD_PREREQ
 
 # Invalid field number.
 nx_pull_match() returned error OFPBMC_BAD_FIELD
+], [stderr])
+
+# Check that at least the first warning made it.  (It's rate-limited
+# so a variable number could show up, especially under valgrind etc.)
+AT_CHECK([grep 'has 1-bits in value' stderr | sed 1q], [0], [dnl
+nx_match|WARN|NXM/OXM entry OXM_OF_METADATA_W(1234567890abcdef/ffff0000ffff0000) has 1-bits in value for bits wildcarded by the mask.  (Future versions of OVS may report this as an OpenFlow error.)
 ])
+
+# Check that there wasn't any other stderr output.
+AT_CHECK([grep -v 'has 1-bits in value' stderr], [1])
 AT_CLEANUP
 
 AT_SETUP([ovs-ofctl parse-oxm loose])
-- 
1.7.2.5




More information about the dev mailing list