[ovs-dev] [branch 2.1] ipfix, sflow: Fix a race.

[Alex Wang]

In ovs 2.1, the unref functions for ipfix and sflow are called
in ofproto/ofproto-dpif-upcall.c without the protection of
any lock.  If the unprotected call of unref function removes
the last reference, and at the same time, other thread is
trying to ref the same struct, the race could cause abortion
of ovs.

To fix this issue, this commit protects the ref and unref
functions using the global mutex of the ipfix and sflow
modules respectively.

Reported-by: Xiao Ma <cumtb_max...@163.com>
Signed-off-by: Alex Wang <al...@nicira.com>
---
 ofproto/ofproto-dpif-ipfix.c |    6 ++++--
 ofproto/ofproto-dpif-sflow.c |   14 +++++---------
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index a9cc73e..8deace8 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -647,11 +647,13 @@ struct dpif_ipfix *
 dpif_ipfix_ref(const struct dpif_ipfix *di_)
 {
     struct dpif_ipfix *di = CONST_CAST(struct dpif_ipfix *, di_);
+    ovs_mutex_lock(&mutex);
     if (di) {
         int orig;
         atomic_add(&di->ref_cnt, 1, &orig);
         ovs_assert(orig > 0);
     }
+    ovs_mutex_unlock(&mutex);
     return di;
 }
 
@@ -689,16 +691,16 @@ dpif_ipfix_unref(struct dpif_ipfix *di) 
OVS_EXCLUDED(mutex)
         return;
     }
 
+    ovs_mutex_lock(&mutex);
     atomic_sub(&di->ref_cnt, 1, &orig);
     ovs_assert(orig > 0);
     if (orig == 1) {
-        ovs_mutex_lock(&mutex);
         dpif_ipfix_clear(di);
         dpif_ipfix_bridge_exporter_destroy(&di->bridge_exporter);
         hmap_destroy(&di->flow_exporter_map);
         free(di);
-        ovs_mutex_unlock(&mutex);
     }
+    ovs_mutex_unlock(&mutex);
 }
 
 static void
diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index 158887f..07a5c4b 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -292,14 +292,6 @@ dpif_sflow_clear__(struct dpif_sflow *ds) 
OVS_REQUIRES(mutex)
     ds->probability = 0;
 }
 
-void
-dpif_sflow_clear(struct dpif_sflow *ds) OVS_EXCLUDED(mutex)
-{
-    ovs_mutex_lock(&mutex);
-    dpif_sflow_clear__(ds);
-    ovs_mutex_unlock(&mutex);
-}
-
 bool
 dpif_sflow_is_enabled(const struct dpif_sflow *ds) OVS_EXCLUDED(mutex)
 {
@@ -336,11 +328,13 @@ struct dpif_sflow *
 dpif_sflow_ref(const struct dpif_sflow *ds_)
 {
     struct dpif_sflow *ds = CONST_CAST(struct dpif_sflow *, ds_);
+    ovs_mutex_lock(&mutex);
     if (ds) {
         int orig;
         atomic_add(&ds->ref_cnt, 1, &orig);
         ovs_assert(orig > 0);
     }
+    ovs_mutex_unlock(&mutex);
     return ds;
 }
 
@@ -366,19 +360,21 @@ dpif_sflow_unref(struct dpif_sflow *ds) 
OVS_EXCLUDED(mutex)
         return;
     }
 
+    ovs_mutex_lock(&mutex);
     atomic_sub(&ds->ref_cnt, 1, &orig);
     ovs_assert(orig > 0);
     if (orig == 1) {
         struct dpif_sflow_port *dsp, *next;
 
         route_table_unregister();
-        dpif_sflow_clear(ds);
+        dpif_sflow_clear__(ds);
         HMAP_FOR_EACH_SAFE (dsp, next, hmap_node, &ds->ports) {
             dpif_sflow_del_port__(ds, dsp);
         }
         hmap_destroy(&ds->ports);
         free(ds);
     }
+    ovs_mutex_unlock(&mutex);
 }
 
 static void
-- 
1.7.9.5

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev