diff mbox series

[ovs-dev] ovs rcu: update rcu pointer first

Message ID 4099DE2E54AFAD489356C6C9161D533397358B01@dggeml522-mbs.china.huawei.com
State Superseded
Headers show
Series [ovs-dev] ovs rcu: update rcu pointer first | expand

Commit Message

Linhaifeng June 1, 2020, 2:26 a.m. UTC 
We should update rcu pointer first then use ovsrcu_postpone to free
otherwise maybe cause use-after-free.

e.g, thead are two threads, A and B:
1. thread A call ovsrcu_postpone and flush cbset, have not call ovsrcu_quiesce
2. thread rcu wait all threads call ovsrcu_quiesce
3. thread B call ovsrcu_quiesce
4. thread B next round get the old pointer
5. thrad A call ovsrcu_quiesce
6. thread rcu free old pointer
7. thread B use-after-free

Signed-off-by: Haifeng Lin <haifeng.lin@huawei.com>
---
 lib/classifier.c              |  4 ++--
 lib/ovs-rcu.h                 |  2 +-
 lib/pvector.c                 | 15 ++++++++-------
 ofproto/ofproto-dpif-mirror.c |  4 ++--
 ofproto/ofproto-dpif-upcall.c |  3 +--
 5 files changed, 14 insertions(+), 14 deletions(-)
diff mbox series

Patch

diff --git a/lib/classifier.c b/lib/classifier.c
index f2c3497c2..6bff76e07 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -249,11 +249,11 @@  cls_rule_set_conjunctions(struct cls_rule *cr,
     unsigned int old_n = old ? old->n : 0;
 
     if (old_n != n || (n && memcmp(old_conj, conj, n * sizeof *conj))) {
+        ovsrcu_set(&match->conj_set,
+                   cls_conjunction_set_alloc(match, conj, n));
         if (old) {
             ovsrcu_postpone(free, old);
         }
-        ovsrcu_set(&match->conj_set,
-                   cls_conjunction_set_alloc(match, conj, n));
     }
 }
 
diff --git a/lib/ovs-rcu.h b/lib/ovs-rcu.h
index ecc4c9201..a66d868ea 100644
--- a/lib/ovs-rcu.h
+++ b/lib/ovs-rcu.h
@@ -119,9 +119,9 @@ 
  *     change_flow(struct flow *new_flow)
  *     {
  *         ovs_mutex_lock(&mutex);
+ *         ovsrcu_set(&flowp, new_flow);
  *         ovsrcu_postpone(free,
  *                         ovsrcu_get_protected(struct flow *, &flowp));
- *         ovsrcu_set(&flowp, new_flow);
  *         ovs_mutex_unlock(&mutex);
  *     }
  *
diff --git a/lib/pvector.c b/lib/pvector.c
index cc527fdc4..6a295ec53 100644
--- a/lib/pvector.c
+++ b/lib/pvector.c
@@ -67,10 +67,11 @@  pvector_init(struct pvector *pvec)
 void
 pvector_destroy(struct pvector *pvec)
 {
+    struct pvector_impl *old = pvector_impl_get(pvec);
     free(pvec->temp);
     pvec->temp = NULL;
-    ovsrcu_postpone(free, pvector_impl_get(pvec));
     ovsrcu_set(&pvec->impl, NULL); /* Poison. */
+    ovsrcu_postpone(free, old);
 }
 
 /* Iterators for callers that need the 'index' afterward. */
@@ -205,11 +206,11 @@  pvector_change_priority(struct pvector *pvec, void *ptr, int priority)
 /* Make the modified pvector available for iteration. */
 void pvector_publish__(struct pvector *pvec)
 {
-    struct pvector_impl *temp = pvec->temp;
-
+    struct pvector_impl *new = pvec->temp;
+    struct pvector_impl *old = vsrcu_get_protected(struct pvector_impl *,
+                                                   &pvec->impl);
     pvec->temp = NULL;
-    pvector_impl_sort(temp); /* Also removes gaps. */
-    ovsrcu_postpone(free, ovsrcu_get_protected(struct pvector_impl *,
-                                               &pvec->impl));
-    ovsrcu_set(&pvec->impl, temp);
+    pvector_impl_sort(new); /* Also removes gaps. */
+    ovsrcu_set(&pvec->impl, new);
+    ovsrcu_postpone(free, old);
 }
diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
index 343b75f0e..343100c08 100644
--- a/ofproto/ofproto-dpif-mirror.c
+++ b/ofproto/ofproto-dpif-mirror.c
@@ -276,9 +276,9 @@  mirror_set(struct mbridge *mbridge, void *aux, const char *name,
     hmapx_destroy(&dsts_map);
 
     if (vlans || src_vlans) {
+        unsigned long *new_vlans = vlan_bitmap_clone(src_vlans);
+        ovsrcu_set(&mirror->vlans, new_vlans);
         ovsrcu_postpone(free, vlans);
-        vlans = vlan_bitmap_clone(src_vlans);
-        ovsrcu_set(&mirror->vlans, vlans);
     }
 
     mirror->out = out;
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 5e08ef10d..be6dafb78 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1658,11 +1658,10 @@  ukey_set_actions(struct udpif_key *ukey, const struct ofpbuf *actions)
     struct ofpbuf *old_actions = ovsrcu_get_protected(struct ofpbuf *,
                                                       &ukey->actions);
 
+    ovsrcu_set(&ukey->actions, ofpbuf_clone(actions));
     if (old_actions) {
         ovsrcu_postpone(ofpbuf_delete, old_actions);
     }
-
-    ovsrcu_set(&ukey->actions, ofpbuf_clone(actions));
 }
 
 static struct udpif_key *