[ovs-dev] [PATCH] ofproto-dpif-ipfix: Fix assertion failure for bad configuration.

[Ben Pfaff]

The assertions in dpif_ipfix_set_options() made some bad assumptions about
flow exporters.  The code that added and removed exporters would add a flow
exporter even if it had an invalid configuration ("broken"), but the
assertions checked that broken flow exporters were not added.  Thus, the
when a flow exporter was broken, ovs-vswitchd would crash due to an
assertion failure.

Here is an example vsctl command that, run in the sandbox, would crash
ovs-vswitchd:

    ovs-vsctl \
        -- add-br br0 \
        -- --id=@br0 get bridge br0 \
        -- --id=@ipfix create ipfix target='["xyzzy"]' \
        -- create flow_sample_collector_set id=1 bridge=@br0 ipfix=@ipfix

The minimal fix would be to remove the assertions, but this would leave
broken flow exporters in place.  This commit goes a little farther and
actually removes broken flow exporters.

This fix pulls code out of an "if" statement to a higher level, so it is a
smaller fix when viewed igoring space changes.

This bug dates back to the introduction of IPFIX in 2013.

VMware-BZ: #1779123
CC: Romain Lenglet <romain.lenglet at berabera.info>
Fixes: 29089a540cfa ("Implement IPFIX export")
Signed-off-by: Ben Pfaff <blp at ovn.org>
---
 ofproto/ofproto-dpif-ipfix.c | 47 ++++++++++++++++++++++----------------------
 tests/ofproto-dpif.at        | 11 ++++++++++-
 2 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index 86a6646..23a04ce 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -859,6 +859,15 @@ dpif_ipfix_flow_exporter_set_options(
     return true;
 }
 
+static void
+remove_flow_exporter(struct dpif_ipfix *di,
+                     struct dpif_ipfix_flow_exporter_map_node *node)
+{
+    hmap_remove(&di->flow_exporter_map, &node->node);
+    dpif_ipfix_flow_exporter_destroy(&node->exporter);
+    free(node);
+}
+
 void
 dpif_ipfix_set_options(
     struct dpif_ipfix *di,
@@ -869,7 +878,6 @@ dpif_ipfix_set_options(
     int i;
     struct ofproto_ipfix_flow_exporter_options *options;
     struct dpif_ipfix_flow_exporter_map_node *node, *next;
-    size_t n_broken_flow_exporters_options = 0;
 
     ovs_mutex_lock(&mutex);
     dpif_ipfix_bridge_exporter_set_options(&di->bridge_exporter,
@@ -888,38 +896,29 @@ dpif_ipfix_set_options(
                         hash_int(options->collector_set_id, 0));
         }
         if (!dpif_ipfix_flow_exporter_set_options(&node->exporter, options)) {
-            n_broken_flow_exporters_options++;
+            remove_flow_exporter(di, node);
         }
         options++;
     }
 
-    ovs_assert(hmap_count(&di->flow_exporter_map) >=
-               (n_flow_exporters_options - n_broken_flow_exporters_options));
-
     /* Remove dropped flow exporters, if any needs to be removed. */
-    if (hmap_count(&di->flow_exporter_map) > n_flow_exporters_options) {
-        HMAP_FOR_EACH_SAFE (node, next, node, &di->flow_exporter_map) {
-            /* This is slow but doesn't take any extra memory, and
-             * this table is not supposed to contain many rows anyway. */
-            options = (struct ofproto_ipfix_flow_exporter_options *)
-                flow_exporters_options;
-            for (i = 0; i < n_flow_exporters_options; i++) {
-              if (node->exporter.options->collector_set_id
-                  == options->collector_set_id) {
-                  break;
-              }
-              options++;
-            }
-            if (i == n_flow_exporters_options) {  // Not found.
-                hmap_remove(&di->flow_exporter_map, &node->node);
-                dpif_ipfix_flow_exporter_destroy(&node->exporter);
-                free(node);
+    HMAP_FOR_EACH_SAFE (node, next, node, &di->flow_exporter_map) {
+        /* This is slow but doesn't take any extra memory, and
+         * this table is not supposed to contain many rows anyway. */
+        options = (struct ofproto_ipfix_flow_exporter_options *)
+            flow_exporters_options;
+        for (i = 0; i < n_flow_exporters_options; i++) {
+            if (node->exporter.options->collector_set_id
+                == options->collector_set_id) {
+                break;
             }
+            options++;
+        }
+        if (i == n_flow_exporters_options) {  // Not found.
+            remove_flow_exporter(di, node);
         }
     }
 
-    ovs_assert(hmap_count(&di->flow_exporter_map) ==
-               (n_flow_exporters_options - n_broken_flow_exporters_options));
     ovs_mutex_unlock(&mutex);
 }
 
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index cd90424..4876fde 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -6279,6 +6279,13 @@ AT_SETUP([ofproto-dpif - Flow IPFIX sanity check])
 OVS_VSWITCHD_START
 add_of_ports br0 1 2
 
+# Check for regression against a bug where an invalid target caused an
+# assertion failure and a crash.
+AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
+                    -- --id=@ipfix create IPFIX targets=\"xyzzy\" \
+                    -- --id=@cs create Flow_Sample_Collector_Set id=0 bridge=@br0 ipfix=@ipfix],
+         [0], [ignore])
+
 AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
                     -- --id=@ipfix create IPFIX targets=\"127.0.0.1:4739\" \
                     -- --id=@cs create Flow_Sample_Collector_Set id=1 bridge=@br0 ipfix=@ipfix],
@@ -6312,7 +6319,9 @@ flow-dump from non-dpdk interfaces:
 packets:2, bytes:68, used:0.001s, actions:drop
 ])
 
-OVS_VSWITCHD_STOP(["/sending to collector failed/d"])
+OVS_VSWITCHD_STOP(["/sending to collector failed/d
+/xyzzy/d
+/no collectors/d"])
 AT_CLEANUP
 
 dnl Flow IPFIX sanity check for tunnel set
-- 
2.10.2