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.leng...@berabera.info> Fixes: 29089a540cfa ("Implement IPFIX export") Signed-off-by: Ben Pfaff <b...@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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev