Hi Haifeng, One more comment. Since this is a bug fix, it is possible that the maintainer will backport to the previous branch. Therefore, it is recommended to split into several patches and add fixes tag. http://docs.openvswitch.org/en/latest/internals/contributing/submitting-patches/?highlight=submit e.g. "Fixes: 63bc9fb1c69f ("packets: Reorder CS_* flags to remove gap.") If you would like to record which commit introduced a bug being fixed, you may do that with a "Fixes" header. This assists in determining which OVS releases have the bug, so the patch can be applied to all affected versions. The easiest way to generate the header in the proper format is with this git command. This command also CCs the author of the commit being fixed, which makes sense unless the author also made the fix or is already named in another tag: $ git log -1 --pretty=format:"CC: %an <%ae>%nFixes: %h (\"%s\")" \ --abbrev=12 COMMIT_REF"
Best Regards, Wei Yanqin > -----Original Message----- > From: Linhaifeng <haifeng....@huawei.com> > Sent: Tuesday, June 2, 2020 3:13 PM > To: Yanqin Wei <yanqin....@arm.com>; d...@openvswitch.org > Cc: nd <n...@arm.com> > Subject: RE: [PATCH] ovs rcu: update rcu pointer first > > 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
