Acked-by: Jarno Rajahalme <ja...@ovn.org>
With questions for clarification below,
Jarno
> On Dec 9, 2016, at 3:12 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> 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) {
This comparison is removed due to the broken exporters not being in the map,
but they are still in the count?
> - 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) {
It is not possible that this would succeed on a broken ‘options’? I.e., it does
not matter that the broken options are still being iterated through here?
> + 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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
