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 <[email protected]> --- 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
