[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