ofp-util: Clean up cookie handling.

Commit e72e793 (Add ability to restrict flow mods and flow stats
requests to cookies.) modified cookie handling.  Some of its behavior
was unintuitive and there was at least one bug (described below).
Commit f66b87d (DESIGN: Document uses for flow cookies.) attempted to
document a clean design for cookie handling.  This commit updates the
DESIGN document and brings the implementation in line with it.

In commit e72e793, the code that handled processing OpenFlow flow
modification requests set the cookie mask to exact-match.  This seems
reasonable for adding flows, but is not correct for matching, since
OpenFlow 1.0 doesn't support matching based on the cookie.  This commit
changes to cookie mask to fully wildcarded, which is the correct
behavior for modifications and deletions.  It doesn't cause any problems
for flow additions, since the mask is ignored for that operation.

Bug #9742

Reported-by: Luca Giraudo <lgiraudo@nicira.com>
Reported-by: Paul Ingram <paul@nicira.com>
Signed-off-by: Justin Pettit <jpettit@nicira.com>

5 files changed, 66 insertions, 31 deletions

diff --git a/lib/learn.c b/lib/learn.c
index e995c29c..ba33287a 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -204,8 +204,9 @@ learn_execute(const struct nx_action_learn *learn, const struct flow *flow,
     struct ofpbuf actions;
 
     cls_rule_init_catchall(&fm->cr, ntohs(learn->priority));
-    fm->cookie = learn->cookie;
-    fm->cookie_mask = htonll(UINT64_MAX);
+    fm->cookie = htonll(0);
+    fm->cookie_mask = htonll(0);
+    fm->new_cookie = learn->cookie;
     fm->table_id = learn->table_id;
     fm->command = OFPFC_MODIFY_STRICT;
     fm->idle_timeout = ntohs(learn->idle_timeout);
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 034a6de6..dac13769 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -589,6 +589,12 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
     cls_rule_init_catchall(&fm->cr, OFP_DEFAULT_PRIORITY);
     fm->cookie = htonll(0);
     fm->cookie_mask = htonll(0);
+    if (command == OFPFC_MODIFY || command == OFPFC_MODIFY_STRICT) {
+        /* For modify, by default, don't update the cookie. */
+        fm->new_cookie = htonll(UINT64_MAX);
+    } else{
+        fm->new_cookie = htonll(0);
+    }
     fm->table_id = 0xff;
     fm->command = command;
     fm->idle_timeout = OFP_FLOW_PERMANENT;
@@ -643,17 +649,24 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
                 fm->hard_timeout = str_to_u16(value, name);
             } else if (!strcmp(name, "cookie")) {
                 char *mask = strchr(value, '/');
+
                 if (mask) {
+                    /* A mask means we're searching for a cookie. */
                     if (command == OFPFC_ADD) {
                         ofp_fatal(str_, verbose, "flow additions cannot use "
                                   "a cookie mask");
                     }
                     *mask = '\0';
+                    fm->cookie = htonll(str_to_u64(value));
                     fm->cookie_mask = htonll(str_to_u64(mask+1));
                 } else {
-                    fm->cookie_mask = htonll(UINT64_MAX);
+                    /* No mask means that the cookie is being set. */
+                    if (command != OFPFC_ADD && command != OFPFC_MODIFY
+                            && command != OFPFC_MODIFY_STRICT) {
+                        ofp_fatal(str_, verbose, "cannot set cookie");
+                    }
+                    fm->new_cookie = htonll(str_to_u64(value));
                 }
-                fm->cookie = htonll(str_to_u64(value));
             } else if (mf_from_name(name)) {
                 parse_field(mf_from_name(name), value, &fm->cr);
             } else if (!strcmp(name, "duration")
@@ -667,6 +680,13 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
             }
         }
     }
+    if (!fm->cookie_mask && fm->new_cookie == htonll(UINT64_MAX)
+            && (command == OFPFC_MODIFY || command == OFPFC_MODIFY_STRICT)) {
+        /* On modifies without a mask, we are supposed to add a flow if
+         * one does not exist.  If a cookie wasn't been specified, use a
+         * default of zero. */
+        fm->new_cookie = htonll(0);
+    }
     if (fields & F_ACTIONS) {
         struct ofpbuf actions;
 
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 7479bf22..e5327ab8 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -977,8 +977,12 @@ ofp_print_flow_mod(struct ds *s, const struct ofp_header *oh,
     if (ds_last(s) != ' ') {
         ds_put_char(s, ' ');
     }
-    if (fm.cookie != htonll(0)) {
-        ds_put_format(s, "cookie:0x%"PRIx64" ", ntohll(fm.cookie));
+    if (fm.new_cookie != htonll(0)) {
+        ds_put_format(s, "cookie:0x%"PRIx64" ", ntohll(fm.new_cookie));
+    }
+    if (fm.cookie_mask != htonll(0)) {
+        ds_put_format(s, "cookie:0x%"PRIx64"/0x%"PRIx64" ",
+                ntohll(fm.cookie), ntohll(fm.cookie_mask));
     }
     if (fm.idle_timeout != OFP_FLOW_PERMANENT) {
         ds_put_format(s, "idle:%"PRIu16" ", fm.idle_timeout);
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 501ccab2..15de6076 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1402,9 +1402,10 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
         ofputil_normalize_rule(&fm->cr);
 
         /* Translate the message. */
-        fm->cookie = ofm->cookie;
-        fm->cookie_mask = htonll(UINT64_MAX);
         command = ntohs(ofm->command);
+        fm->cookie = htonll(0);
+        fm->cookie_mask = htonll(0);
+        fm->new_cookie = ofm->cookie;
         fm->idle_timeout = ntohs(ofm->idle_timeout);
         fm->hard_timeout = ntohs(ofm->hard_timeout);
         fm->buffer_id = ntohl(ofm->buffer_id);
@@ -1429,17 +1430,12 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
 
         /* Translate the message. */
         command = ntohs(nfm->command);
-        if (command == OFPFC_ADD) {
-            if (fm->cookie_mask) {
-                /* The "NXM_NX_COOKIE*" matches are not valid for flow
-                 * additions.  Additions must use the "cookie" field of
-                 * the "nx_flow_mod" structure. */
-                return OFPERR_NXBRC_NXM_INVALID;
-            } else {
-                fm->cookie = nfm->cookie;
-                fm->cookie_mask = htonll(UINT64_MAX);
-            }
+        if ((command & 0xff) == OFPFC_ADD && fm->cookie_mask) {
+            /* Flow additions may only set a new cookie, not match an
+             * existing cookie. */
+            return OFPERR_NXBRC_NXM_INVALID;
         }
+        fm->new_cookie = nfm->cookie;
         fm->idle_timeout = ntohs(nfm->idle_timeout);
         fm->hard_timeout = ntohs(nfm->hard_timeout);
         fm->buffer_id = ntohl(nfm->buffer_id);
@@ -1486,7 +1482,7 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm,
         msg = ofpbuf_new(sizeof *ofm + actions_len);
         ofm = put_openflow(sizeof *ofm, OFPT10_FLOW_MOD, msg);
         ofputil_cls_rule_to_match(&fm->cr, &ofm->match);
-        ofm->cookie = fm->cookie;
+        ofm->cookie = fm->new_cookie;
         ofm->command = htons(command);
         ofm->idle_timeout = htons(fm->idle_timeout);
         ofm->hard_timeout = htons(fm->hard_timeout);
@@ -1502,14 +1498,8 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm,
         put_nxmsg(sizeof *nfm, NXT_FLOW_MOD, msg);
         nfm = msg->data;
         nfm->command = htons(command);
-        if (command == OFPFC_ADD) {
-            nfm->cookie = fm->cookie;
-            match_len = nx_put_match(msg, &fm->cr, 0, 0);
-        } else {
-            nfm->cookie = 0;
-            match_len = nx_put_match(msg, &fm->cr,
-                                     fm->cookie, fm->cookie_mask);
-        }
+        nfm->cookie = fm->new_cookie;
+        match_len = nx_put_match(msg, &fm->cr, fm->cookie, fm->cookie_mask);
         nfm->idle_timeout = htons(fm->idle_timeout);
         nfm->hard_timeout = htons(fm->hard_timeout);
         nfm->priority = htons(fm->cr.priority);
@@ -1548,7 +1538,9 @@ ofputil_flow_mod_usable_protocols(const struct ofputil_flow_mod *fms,
         if (fm->table_id != 0xff) {
             usable_protocols &= OFPUTIL_P_TID;
         }
-        if (fm->command != OFPFC_ADD && fm->cookie_mask != htonll(0)) {
+
+        /* Matching of the cookie is only supported through NXM. */
+        if (fm->cookie_mask != htonll(0)) {
             usable_protocols &= OFPUTIL_P_NXM_ANY;
         }
     }
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index e6716636..3aa1e095 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -197,11 +197,29 @@ struct ofpbuf *ofputil_make_set_packet_in_format(enum nx_packet_in_format);
 /* NXT_FLOW_MOD_TABLE_ID extension. */
 struct ofpbuf *ofputil_make_flow_mod_table_id(bool flow_mod_table_id);
 
-/* Protocol-independent flow_mod. */
+/* Protocol-independent flow_mod.
+ *
+ * The handling of cookies across multiple versions of OpenFlow is a bit
+ * confusing.  A full description of Open vSwitch's cookie handling is
+ * in the DESIGN file.  The following table shows the expected values of
+ * the cookie-related fields for the different flow_mod commands in
+ * OpenFlow 1.0 ("OF10") and NXM.  "<used>" and "-" indicate a value
+ * that may be populated and an ignored field, respectively.
+ *
+ *               cookie  cookie_mask  new_cookie
+ *               ======  ===========  ==========
+ * OF10 Add        -          0         <used>
+ * OF10 Modify     -          0         <used>
+ * OF10 Delete     -          0           -
+ * NXM Add         -          0         <used>
+ * NXM Modify    <used>     <used>      <used>
+ * NXM Delete    <used>     <used>        -
+ */
 struct ofputil_flow_mod {
     struct cls_rule cr;
-    ovs_be64 cookie;
-    ovs_be64 cookie_mask;
+    ovs_be64 cookie;         /* Cookie bits to match. */
+    ovs_be64 cookie_mask;    /* 1-bit in each 'cookie' bit to match. */
+    ovs_be64 new_cookie;     /* New cookie to install or -1. */
     uint8_t table_id;
     uint16_t command;
     uint16_t idle_timeout;