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 <[email protected]>
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2020-June/371265.html
Acked-by: Ben Pfaff <[email protected]>
Signed-off-by: Ilya Maximets <[email protected]>
---
'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 [email protected]
Krishna Mohan Elluru [email protected]
László Sürü [email protected]
Len Gao [email protected]
+Linhaifeng [email protected]
Logan Rosen [email protected]
Luca Falavigna [email protected]
Luiz Henrique Ozaki [email protected]
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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
