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

[Linhaifeng]

Hi Yanqin,

Thank you for your suggestions. I will send a new patch.

-----Original Message-----
From: Yanqin Wei [mailto:Yanqin.Wei at arm.com] 
Sent: Tuesday, June 2, 2020 11:51 AM
To: Linhaifeng <haifeng.lin at huawei.com>; dev at openvswitch.org
Cc: nd <nd at 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-bounces at openvswitch.org> On Behalf Of Linhaifeng
> Sent: Monday, June 1, 2020 11:13 AM
> To: dev at 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.lin at 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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev