[ovs-dev] [PATCH] ofproto:fix abort issue when delete bridge

Ben Pfaff blp at ovn.org
Mon Jan 8 21:04:59 UTC 2018


This seems like the wrong approach to me.  If there's a problem that
struct rule_dpif's 'new_rule' member can sometimes point to a rule that
gets freed, then one would normally fix that through some kind of
synchronization between the rules (for example, one could make sure that
RCU keeps ->new_rule from being freed while it is still in use), not by
adding a refcount on a data structure multiple levels higher.

On Mon, Jan 08, 2018 at 11:09:58AM +0800, Haibo Zhang wrote:
> When delete a birdge, all the rule of the bridge wil be removed. When destruct a rule, it is possible that the rule has a non-NULL new_rule A, and the new_rule A might has a non-NULL new_rule B, and the new_rule B might has a non-NULL new_rule C... in this case, the ofproto has been freed before rule B or C was freed, and it will cause crash issue when free rule B or C using rcu mechanism. To fix the issue, a reference count is introduced to ofproto to make sure all rules of the ofproto were freed completely before the ofproto was freed.
> 
> Signed-off-by: Haibo Zhang <zhanghaibo5 at huawei.com>
> ---
>  ofproto/ofproto-dpif.c     | 14 ++++++++++++--
>  ofproto/ofproto-provider.h |  9 ++++++++-
>  ofproto/ofproto.c          | 26 ++++++++++++++++----------
>  3 files changed, 36 insertions(+), 13 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 43b7b89..968a51a 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4254,16 +4254,26 @@ static struct rule_dpif *rule_dpif_cast(const struct rule *rule)
>  }
>  
>  static struct rule *
> -rule_alloc(void)
> +rule_alloc(struct ofproto * ofproto)
>  {
> +    struct rule * rule_;
>      struct rule_dpif *rule = xzalloc(sizeof *rule);
> -    return &rule->up;
> +
> +    if (OVS_UNLIKELY(!rule)) {
> +        return NULL;
> +    }
> +
> +    rule_ = &rule->up;
> +    *CONST_CAST(struct ofproto **, &rule_->ofproto) = ofproto;
> +    ofproto_ref(ofproto);
> +    return rule_;
>  }
>  
>  static void
>  rule_dealloc(struct rule *rule_)
>  {
>      struct rule_dpif *rule = rule_dpif_cast(rule_);
> +    ofproto_unref(rule_->ofproto);
>      free(rule);
>  }
>  
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 9dc73c4..ce3e0f7 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -133,6 +133,11 @@ struct ofproto {
>      /* Variable length mf_field mapping. Stores all configured variable length
>       * meta-flow fields (struct mf_field) in a switch. */
>      struct vl_mff_map vl_mff_map;
> +    /* Number of references.
> +     * bridge keep one reference
> +     * Any rule trying to keep ofproto from being freed should hold its own
> +     * reference. */
> +    struct ovs_refcount ref_count;
>  };
>  
>  void ofproto_init_tables(struct ofproto *, int n_tables);
> @@ -433,6 +438,8 @@ struct rule {
>  void ofproto_rule_ref(struct rule *);
>  bool ofproto_rule_try_ref(struct rule *);
>  void ofproto_rule_unref(struct rule *);
> +void ofproto_ref(struct ofproto *);
> +void ofproto_unref(struct ofproto *);
>  
>  static inline const struct rule_actions * rule_get_actions(const struct rule *);
>  static inline bool rule_is_table_miss(const struct rule *);
> @@ -1288,7 +1295,7 @@ struct ofproto_class {
>       * ->rule_destruct() must uninitialize derived state.
>       *
>       * Rule destruction must not fail. */
> -    struct rule *(*rule_alloc)(void);
> +    struct rule *(*rule_alloc)(struct ofproto *);
>      enum ofperr (*rule_construct)(struct rule *rule)
>          /* OVS_REQUIRES(ofproto_mutex) */;
>      void (*rule_insert)(struct rule *rule, struct rule *old_rule,
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 84eb18e..d68de6b 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -523,6 +523,7 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
>      ofproto->connmgr = connmgr_create(ofproto, datapath_name, datapath_name);
>      ofproto->min_mtu = INT_MAX;
>      cmap_init(&ofproto->groups);
> +    ovs_refcount_init(&ofproto->ref_count);
>      ovs_mutex_unlock(&ofproto_mutex);
>      ofproto->ogf.types = 0xf;
>      ofproto->ogf.capabilities = OFPGFC_CHAINING | OFPGFC_SELECT_LIVENESS |
> @@ -1616,14 +1617,20 @@ ofproto_destroy__(struct ofproto *ofproto)
>      ofproto->ofproto_class->dealloc(ofproto);
>  }
>  
> -/* Destroying rules is doubly deferred, must have 'ofproto' around for them.
> - * - 1st we defer the removal of the rules from the classifier
> - * - 2nd we defer the actual destruction of the rules. */
> -static void
> -ofproto_destroy_defer__(struct ofproto *ofproto)
> -    OVS_EXCLUDED(ofproto_mutex)
> +void
> +ofproto_ref(struct ofproto *ofproto)
>  {
> -    ovsrcu_postpone(ofproto_destroy__, ofproto);
> +    if (ofproto) {
> +        ovs_refcount_ref(&ofproto->ref_count);
> +    }
> +}
> +
> +void
> +ofproto_unref(struct ofproto *ofproto)
> +{
> +    if (ofproto && ovs_refcount_unref_relaxed(&ofproto->ref_count) == 1) {
> +        ovsrcu_postpone(ofproto_destroy__, ofproto);
> +    }
>  }
>  
>  void
> @@ -1660,8 +1667,7 @@ ofproto_destroy(struct ofproto *p, bool del)
>      p->connmgr = NULL;
>      ovs_mutex_unlock(&ofproto_mutex);
>  
> -    /* Destroying rules is deferred, must have 'ofproto' around for them. */
> -    ovsrcu_postpone(ofproto_destroy_defer__, p);
> +    ofproto_unref(p);
>  }
>  
>  /* Destroys the datapath with the respective 'name' and 'type'.  With the Linux
> @@ -4888,7 +4894,7 @@ ofproto_rule_create(struct ofproto *ofproto, struct cls_rule *cr,
>      enum ofperr error;
>  
>      /* Allocate new rule. */
> -    rule = ofproto->ofproto_class->rule_alloc();
> +    rule = ofproto->ofproto_class->rule_alloc(ofproto);
>      if (!rule) {
>          cls_rule_destroy(cr);
>          VLOG_WARN_RL(&rl, "%s: failed to allocate a rule.", ofproto->name);
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list