[ovs-dev] [PATCH] ovs-rcu: Avoid flushing callbacks during postponing.

Ilya Maximets i.maximets at ovn.org
Wed Jun 10 19:37:38 UTC 2020


ovsrcu_flush_cbset() call during ovsrcu_postpone() could cause
use after free in case the caller sets new pointer only after
postponing free for the old one:

 ------------------  ------------------  -------------------
 Thread 1            Thread 2            RCU Thread
 ------------------  ------------------  -------------------
 pointer = A

 ovsrcu_quiesce():
  thread->seqno = 30
  global_seqno = 31
  quiesced

 read pointer A
 postpone(free(A)):
   flush cbset
                                         pop flushed_cbsets
                                         ovsrcu_synchronize:
                                           target_seqno = 31
                     ovsrcu_quiesce():
                      thread->seqno = 31
                      global_seqno = 32
                      quiesced

                     read pointer A
                     use pointer A

                     ovsrcu_quiesce():
                      thread->seqno = 32
                      global_seqno = 33
                      quiesced

                     read pointer A
 pointer = B

 ovsrcu_quiesce():
  thread->seqno = 33
  global_seqno = 34
  quiesced

                                         target_seqno exceeded
                                         by all threads
                                         call cbs to free A
                     use pointer A
                     (use after free)
 -----------------------------------------------------------

Fix that by using dynamically re-allocated array without flushing
to the global flushed_cbsets until writer enters quiescent state.

Fixes: 0f2ea84841e1 ("ovs-rcu: New library.")
Reported-by: Linhaifeng <haifeng.lin at huawei.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-June/371265.html
Acked-by: Ben Pfaff <blp at ovn.org>
Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
---

'Reported-at' tag pointed to v2 of the patch from Linhaifeng, since it
contains a main discussion.  Also Linhaifeng added to a list of people
who provided valuable bug reports and suggestions.

This patch is already acked, so I will just test it a little bit more
and apply.

 AUTHORS.rst   |  1 +
 lib/ovs-rcu.c | 17 ++++++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/AUTHORS.rst b/AUTHORS.rst
index 3f7eee54f..7a3b12610 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -563,6 +563,7 @@ Krishna Miriyala                miriyalak at vmware.com
 Krishna Mohan Elluru            elluru.kri.mohan at hpe.com
 László Sürü                     laszlo.suru at ericsson.com
 Len Gao                         leng at vmware.com
+Linhaifeng                      haifeng.lin at huawei.com
 Logan Rosen                     logatronico at gmail.com
 Luca Falavigna                  dktrkranz at debian.org
 Luiz Henrique Ozaki             luiz.ozaki at gmail.com
diff --git a/lib/ovs-rcu.c b/lib/ovs-rcu.c
index ebc8120f0..cde1e925b 100644
--- a/lib/ovs-rcu.c
+++ b/lib/ovs-rcu.c
@@ -30,6 +30,8 @@
 
 VLOG_DEFINE_THIS_MODULE(ovs_rcu);
 
+#define MIN_CBS 16
+
 struct ovsrcu_cb {
     void (*function)(void *aux);
     void *aux;
@@ -37,7 +39,8 @@ struct ovsrcu_cb {
 
 struct ovsrcu_cbset {
     struct ovs_list list_node;
-    struct ovsrcu_cb cbs[16];
+    struct ovsrcu_cb *cbs;
+    size_t n_allocated;
     int n_cbs;
 };
 
@@ -310,16 +313,19 @@ ovsrcu_postpone__(void (*function)(void *aux), void *aux)
     cbset = perthread->cbset;
     if (!cbset) {
         cbset = perthread->cbset = xmalloc(sizeof *perthread->cbset);
+        cbset->cbs = xmalloc(MIN_CBS * sizeof *cbset->cbs);
+        cbset->n_allocated = MIN_CBS;
         cbset->n_cbs = 0;
     }
 
+    if (cbset->n_cbs == cbset->n_allocated) {
+        cbset->cbs = x2nrealloc(cbset->cbs, &cbset->n_allocated,
+                                sizeof *cbset->cbs);
+    }
+
     cb = &cbset->cbs[cbset->n_cbs++];
     cb->function = function;
     cb->aux = aux;
-
-    if (cbset->n_cbs >= ARRAY_SIZE(cbset->cbs)) {
-        ovsrcu_flush_cbset(perthread);
-    }
 }
 
 static bool
@@ -341,6 +347,7 @@ ovsrcu_call_postponed(void)
         for (cb = cbset->cbs; cb < &cbset->cbs[cbset->n_cbs]; cb++) {
             cb->function(cb->aux);
         }
+        free(cbset->cbs);
         free(cbset);
     }
 
-- 
2.25.4



More information about the dev mailing list