Hi Yanqin, Thank you for your suggestions. I will send a new patch.
-----Original Message----- From: Yanqin Wei [mailto:yanqin....@arm.com] Sent: Tuesday, June 2, 2020 11:51 AM To: Linhaifeng <haifeng....@huawei.com>; d...@openvswitch.org Cc: nd <n...@arm.com> Subject: RE: [PATCH] ovs rcu: update rcu pointer first Hi Haifeng, It looks indeed a risk for using ovs-rcu. A few comments inline. Best Regards, Wei Yanqin > -----Original Message----- > From: dev <ovs-dev-boun...@openvswitch.org> On Behalf Of Linhaifeng > Sent: Monday, June 1, 2020 11:13 AM > To: d...@openvswitch.org > Subject: [ovs-dev] [PATCH] ovs rcu: update rcu pointer first > > 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, this time have not > call ovsrcu_quiesce > > 2. thread rcu wait all threads call ovsrcu_quiesce > > 3. thread B call ovsrcu_quiesce > > 4. thread B get the old pointer next round > > 5. thrad A call ovsrcu_quiesce, now all threads have called > ovsrcu_quiesce [Yanqin] Thread A is a writer and does not have to call ovsrcu_quiesce. I think this scenario can be simplified as follows: reader indicates momentary quiescent and access old pointer after writer postpone free old pointer and before setting new pointer. > > 6. thread rcu free old pointer > > 7. thread B use-after-free > > Signed-off-by: Linhaifeng <haifeng....@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 --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)); [Yanqin] flowp has been set to new flow pointer here. Maybe a new variable is needed to store old pointer. > - * ovsrcu_set(&flowp, new_flow); > * ovs_mutex_unlock(&mutex); > * } > * > diff --git a/lib/pvector.c b/lib/pvector.c index cc527fdc4..aa8c6cb24 > 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 = ovsrcu_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 * > -- > 2.21.0.windows.1 > > _______________________________________________ > 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