bundles: Validate bundled messages.

OpenFlow bundle messages should be decoded and validated at the time
they are added to the bundle.  This commit does this for flow mod and
port mod messages.

Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index cec074f..d773dca 100644 (file)
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -2657,7 +2657,7 @@ ofp_print_bundle_add(struct ds *s, const struct ofp_header *oh, int verbosity)
     struct ofputil_bundle_add_msg badd;
     char *msg;
 
-    error = ofputil_decode_bundle_add(oh, &badd);
+    error = ofputil_decode_bundle_add(oh, &badd, NULL);
     if (error) {
         ofp_print_error(s, error);
         return;

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index e62c584..17a0c41 100644 (file)
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -8852,7 +8852,8 @@ ofputil_is_bundlable(enum ofptype type)
 
 enum ofperr
 ofputil_decode_bundle_add(const struct ofp_header *oh,
-                          struct ofputil_bundle_add_msg *msg)
+                          struct ofputil_bundle_add_msg *msg,
+                          enum ofptype *type_ptr)
 {
     const struct ofp14_bundle_ctrl_msg *m;
     struct ofpbuf b;
@@ -8879,14 +8880,17 @@ ofputil_decode_bundle_add(const struct ofp_header *oh,
     }
 
     /* Reject unbundlable messages. */
-    error = ofptype_decode(&type, msg->msg);
+    if (!type_ptr) {
+        type_ptr = &type;
+    }
+    error = ofptype_decode(type_ptr, msg->msg);
     if (error) {
         VLOG_WARN_RL(&bad_ofmsg_rl, "OFPT14_BUNDLE_ADD_MESSAGE contained "
                      "message is unparsable (%s)", ofperr_get_name(error));
         return OFPERR_OFPBFC_MSG_UNSUP; /* 'error' would be confusing. */
     }
 
-    if (!ofputil_is_bundlable(type)) {
+    if (!ofputil_is_bundlable(*type_ptr)) {
         return OFPERR_OFPBFC_MSG_UNSUP;
     }
 

diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index ee3f1be..efb5b18 100644 (file)
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -1109,6 +1109,8 @@ struct ofputil_bundle_add_msg {
     const struct ofp_header   *msg;
 };
 
+enum ofptype;
+
 enum ofperr ofputil_decode_bundle_ctrl(const struct ofp_header *,
                                        struct ofputil_bundle_ctrl_msg *);
 
@@ -1119,5 +1121,6 @@ struct ofpbuf *ofputil_encode_bundle_add(enum ofp_version ofp_version,
                                          struct ofputil_bundle_add_msg *msg);
 
 enum ofperr ofputil_decode_bundle_add(const struct ofp_header *,
-                                      struct ofputil_bundle_add_msg *);
+                                      struct ofputil_bundle_add_msg *,
+                                      enum ofptype *type);
 #endif /* ofp-util.h */

diff --git a/ofproto/bundles.c b/ofproto/bundles.c
index c409091..f6ad608 100644 (file)
--- a/ofproto/bundles.c
+++ b/ofproto/bundles.c
@@ -1,7 +1,7 @@
 /*
  * Copyright (c) 2013, 2014 Alexandru Copot <alex.mihai.c@gmail.com>, with support from IXIA.
  * Copyright (c) 2013, 2014 Daniel Baluta <dbaluta@ixiacom.com>
- * Copyright (c) 2014 Nicira, Inc.
+ * Copyright (c) 2014, 2015 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -57,11 +57,6 @@ struct ofp_bundle {
     struct ovs_list    msg_list;
 };
 
-struct bundle_message {
-    struct ofp_header *msg;
-    struct ovs_list   node;  /* Element in 'struct ofp_bundles's msg_list */
-};
-
 static uint32_t
 bundle_hash(uint32_t id)
 {
@@ -98,20 +93,19 @@ ofp_bundle_create(uint32_t id, uint16_t flags)
 }
 
 static void
-ofp_bundle_remove(struct ofconn *ofconn, struct ofp_bundle *item)
+ofp_bundle_remove(struct ofconn *ofconn, struct ofp_bundle *bundle)
 {
-    struct bundle_message *msg;
+    struct ofp_bundle_entry *msg;
     struct hmap *bundles;
 
-    LIST_FOR_EACH_POP (msg, node, &item->msg_list) {
-        free(msg->msg);
-        free(msg);
+    LIST_FOR_EACH_POP (msg, node, &bundle->msg_list) {
+        ofp_bundle_entry_free(msg);
     }
 
     bundles = ofconn_get_bundles(ofconn);
-    hmap_remove(bundles, &item->node);
+    hmap_remove(bundles, &bundle->node);
 
-    free(item);
+    free(bundle);
 }
 
 void
@@ -187,7 +181,7 @@ ofp_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
     struct hmap *bundles;
     struct ofp_bundle *bundle;
     enum ofperr error = 0;
-    struct bundle_message *msg;
+    struct ofp_bundle_entry *msg;
 
     bundles = ofconn_get_bundles(ofconn);
     bundle = ofp_bundle_find(bundles, id);
@@ -227,31 +221,29 @@ ofp_bundle_discard(struct ofconn *ofconn, uint32_t id)
 }
 
 enum ofperr
-ofp_bundle_add_message(struct ofconn *ofconn, struct ofputil_bundle_add_msg *badd)
+ofp_bundle_add_message(struct ofconn *ofconn, uint32_t id, uint16_t flags,
+                       struct ofp_bundle_entry *bmsg)
 {
     struct hmap *bundles;
     struct ofp_bundle *bundle;
-    struct bundle_message *bmsg;
 
     bundles = ofconn_get_bundles(ofconn);
-    bundle = ofp_bundle_find(bundles, badd->bundle_id);
+    bundle = ofp_bundle_find(bundles, id);
 
     if (!bundle) {
-        bundle = ofp_bundle_create(badd->bundle_id, badd->flags);
+        bundle = ofp_bundle_create(id, flags);
         bundle->state = BS_OPEN;
 
         bundles = ofconn_get_bundles(ofconn);
-        hmap_insert(bundles, &bundle->node, bundle_hash(badd->bundle_id));
+        hmap_insert(bundles, &bundle->node, bundle_hash(id));
     } else if (bundle->state == BS_CLOSED) {
         ofp_bundle_remove(ofconn, bundle);
         return OFPERR_OFPBFC_BUNDLE_CLOSED;
-    } else if (badd->flags != bundle->flags) {
+    } else if (flags != bundle->flags) {
         ofp_bundle_remove(ofconn, bundle);
         return OFPERR_OFPBFC_BAD_FLAGS;
     }
 
-    bmsg = xmalloc(sizeof *bmsg);
-    bmsg->msg = xmemdup(badd->msg, ntohs(badd->msg->length));
     list_push_back(&bundle->msg_list, &bmsg->node);
     return 0;
 }

diff --git a/ofproto/bundles.h b/ofproto/bundles.h
index 9a6dfa5..c8ce5c9 100644 (file)
--- a/ofproto/bundles.h
+++ b/ofproto/bundles.h
@@ -1,6 +1,7 @@
 /*
  * Copyright (c) 2013, 2014 Alexandru Copot <alex.mihai.c@gmail.com>, with support from IXIA.
  * Copyright (c) 2013, 2014 Daniel Baluta <dbaluta@ixiacom.com>
+ * Copyright (c) 2014, 2015 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,27 +21,57 @@
 
 #include <sys/types.h>
 
-#include "ofp-msgs.h"
 #include "connmgr.h"
+#include "list.h"
+#include "ofp-msgs.h"
 #include "ofp-util.h"
 
 #ifdef  __cplusplus
 extern "C" {
 #endif
 
+struct ofp_bundle_entry {
+    struct ovs_list   node;
+    ovs_be32          xid;   /* For error returns. */
+    enum ofptype      type;  /* OFPTYPE_FLOW_MOD or OFPTYPE_PORT_MOD. */
+    union {
+        struct ofputil_flow_mod fm;   /* 'fm.ofpacts' must be malloced. */
+        struct ofputil_port_mod pm;
+    };
+};
 
-enum ofperr ofp_bundle_open(struct ofconn *ofconn, uint32_t id, uint16_t flags);
-
-enum ofperr ofp_bundle_close(struct ofconn *ofconn, uint32_t id, uint16_t flags);
+static inline struct ofp_bundle_entry *ofp_bundle_entry_alloc(
+    enum ofptype type, ovs_be32 xid);
+static inline void ofp_bundle_entry_free(struct ofp_bundle_entry *);
 
-enum ofperr ofp_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags);
+enum ofperr ofp_bundle_open(struct ofconn *, uint32_t id, uint16_t flags);
+enum ofperr ofp_bundle_close(struct ofconn *, uint32_t id, uint16_t flags);
+enum ofperr ofp_bundle_commit(struct ofconn *, uint32_t id, uint16_t flags);
+enum ofperr ofp_bundle_discard(struct ofconn *, uint32_t id);
+enum ofperr ofp_bundle_add_message(struct ofconn *, uint32_t id,
+                                   uint16_t flags, struct ofp_bundle_entry *);
+void ofp_bundle_remove_all(struct ofconn *);
+\f
+static inline struct ofp_bundle_entry *
+ofp_bundle_entry_alloc(enum ofptype type, ovs_be32 xid)
+{
+    struct ofp_bundle_entry *entry = xmalloc(sizeof *entry);
 
-enum ofperr ofp_bundle_discard(struct ofconn *ofconn, uint32_t id);
+    entry->xid = xid;
+    entry->type = type;
 
-enum ofperr ofp_bundle_add_message(struct ofconn *ofconn,
-                                   struct ofputil_bundle_add_msg *badd);
+    return entry;
+}
 
-void ofp_bundle_remove_all(struct ofconn *ofconn);
+static inline void ofp_bundle_entry_free(struct ofp_bundle_entry *entry)
+{
+    if (entry) {
+        if (entry->type == OFPTYPE_FLOW_MOD) {
+            free(entry->fm.ofpacts);
+        }
+        free(entry);
+    }
+}
 
 #ifdef  __cplusplus
 }

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 9c4e97d..0a8c82a 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -6357,20 +6357,57 @@ handle_bundle_control(struct ofconn *ofconn, const struct ofp_header *oh)
 static enum ofperr
 handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh)
 {
+    struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
     enum ofperr error;
     struct ofputil_bundle_add_msg badd;
+    struct ofp_bundle_entry *bmsg;
+    enum ofptype type;
 
     error = reject_slave_controller(ofconn);
     if (error) {
         return error;
     }
 
-    error = ofputil_decode_bundle_add(oh, &badd);
+    error = ofputil_decode_bundle_add(oh, &badd, &type);
     if (error) {
         return error;
     }
 
-    return ofp_bundle_add_message(ofconn, &badd);
+    bmsg = ofp_bundle_entry_alloc(type, badd.msg->xid);
+
+    if (type == OFPTYPE_PORT_MOD) {
+        error = ofputil_decode_port_mod(badd.msg, &bmsg->pm, false);
+    } else if (type == OFPTYPE_FLOW_MOD) {
+        struct ofpbuf ofpacts;
+        uint64_t ofpacts_stub[1024 / 8];
+
+        ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
+        error = ofputil_decode_flow_mod(&bmsg->fm, badd.msg,
+                                        ofconn_get_protocol(ofconn),
+                                        &ofpacts,
+                                        u16_to_ofp(ofproto->max_ports),
+                                        ofproto->n_tables);
+        /* Move actions to heap. */
+        bmsg->fm.ofpacts = ofpbuf_steal_data(&ofpacts);
+
+        if (!error && bmsg->fm.ofpacts_len) {
+            error = ofproto_check_ofpacts(ofproto, bmsg->fm.ofpacts,
+                                          bmsg->fm.ofpacts_len);
+        }
+    } else {
+        OVS_NOT_REACHED();
+    }
+
+    if (!error) {
+        error = ofp_bundle_add_message(ofconn, badd.bundle_id, badd.flags,
+                                       bmsg);
+    }
+
+    if (error) {
+        ofp_bundle_entry_free(bmsg);
+    }
+
+    return error;
 }
 
 static enum ofperr