ofproto-dpif-ipfix: Fix assertion failure for bad configuration.

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@berabera.info>
Fixes: 29089a540cfa ("Implement IPFIX export")
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Jarno Rajahalme <jarno@ovn.org>

1 files changed, 23 insertions, 24 deletions

diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index 86a6646ac..23a04ce6c 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);
 }