Set ofproto's connmgr pointer to NULL after the connmgr has been
destructed, and check for NULL when sending a flow removed
notification.
Verified by sending the flow removed message unconditionally and
observing numerous core dumps in the test suite.
Found by inspection.
Fixes: f695ebfae5 ("ofproto: Postpone sending flow removed messages.")
Signed-off-by: Jarno Rajahalme <ja...@ovn.org>
---
v3: New patch for v3.
ofproto/connmgr.c | 16 ++++++++++++----
ofproto/connmgr.h | 6 ++++--
ofproto/fail-open.c | 8 +++++---
ofproto/fail-open.h | 2 +-
ofproto/in-band.c | 2 ++
ofproto/ofproto-provider.h | 2 +-
ofproto/ofproto.c | 36 ++++++++++++++++++++++++++++--------
ofproto/ofproto.h | 3 ++-
8 files changed, 55 insertions(+), 20 deletions(-)
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index d2bedad..51e676a 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -191,7 +191,10 @@ struct connmgr {
/* OpenFlow connections. */
struct hmap controllers; /* All OFCONN_PRIMARY controllers. */
- struct ovs_list all_conns; /* All controllers. */
+ struct ovs_list all_conns; /* All controllers. All modifications are
+ protected by ofproto_mutex, so that any
+ traversals from other threads can be made
+ safe by holding the ofproto_mutex. */
uint64_t master_election_id; /* monotonically increasing sequence number
* for master election */
bool master_election_id_defined;
@@ -255,6 +258,7 @@ connmgr_create(struct ofproto *ofproto,
/* Frees 'mgr' and all of its resources. */
void
connmgr_destroy(struct connmgr *mgr)
+ OVS_REQUIRES(ofproto_mutex)
{
struct ofservice *ofservice, *next_ofservice;
struct ofconn *ofconn, *next_ofconn;
@@ -264,11 +268,9 @@ connmgr_destroy(struct connmgr *mgr)
return;
}
- ovs_mutex_lock(&ofproto_mutex);
LIST_FOR_EACH_SAFE (ofconn, next_ofconn, node, &mgr->all_conns) {
ofconn_destroy(ofconn);
}
- ovs_mutex_unlock(&ofproto_mutex);
hmap_destroy(&mgr->controllers);
@@ -770,7 +772,9 @@ update_fail_open(struct connmgr *mgr)
mgr->fail_open = fail_open_create(mgr->ofproto, mgr);
}
} else {
+ ovs_mutex_lock(&ofproto_mutex);
fail_open_destroy(mgr->fail_open);
+ ovs_mutex_unlock(&ofproto_mutex);
mgr->fail_open = NULL;
}
}
@@ -1203,6 +1207,7 @@ ofconn_get_target(const struct ofconn *ofconn)
static struct ofconn *
ofconn_create(struct connmgr *mgr, struct rconn *rconn, enum ofconn_type type,
bool enable_async_msgs)
+ OVS_REQUIRES(ofproto_mutex)
{
struct ofconn *ofconn;
@@ -1607,10 +1612,13 @@ connmgr_send_requestforward(struct connmgr *mgr, const
struct ofconn *source,
}
/* Sends an OFPT_FLOW_REMOVED or NXT_FLOW_REMOVED message based on 'fr' to
- * appropriate controllers managed by 'mgr'. */
+ * appropriate controllers managed by 'mgr'.
+ *
+ * This may be called from the RCU thread. */
void
connmgr_send_flow_removed(struct connmgr *mgr,
const struct ofputil_flow_removed *fr)
+ OVS_REQUIRES(ofproto_mutex)
{
struct ofconn *ofconn;
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 0768aa0..1a75c4c 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -71,7 +71,8 @@ void ofproto_async_msg_free(struct ofproto_async_msg *);
/* Basics. */
struct connmgr *connmgr_create(struct ofproto *ofproto,
const char *dpif_name, const char *local_name);
-void connmgr_destroy(struct connmgr *);
+void connmgr_destroy(struct connmgr *)
+ OVS_REQUIRES(ofproto_mutex);
void connmgr_run(struct connmgr *,
void (*handle_openflow)(struct ofconn *,
@@ -142,7 +143,8 @@ bool connmgr_wants_packet_in_on_miss(struct connmgr *mgr);
void connmgr_send_port_status(struct connmgr *, struct ofconn *source,
const struct ofputil_phy_port *, uint8_t reason);
void connmgr_send_flow_removed(struct connmgr *,
- const struct ofputil_flow_removed *);
+ const struct ofputil_flow_removed *)
+ OVS_REQUIRES(ofproto_mutex);
void connmgr_send_async_msg(struct connmgr *,
const struct ofproto_async_msg *);
void ofconn_send_role_status(struct ofconn *ofconn, uint32_t role,
diff --git a/ofproto/fail-open.c b/ofproto/fail-open.c
index 3c3579e..e038e77 100644
--- a/ofproto/fail-open.c
+++ b/ofproto/fail-open.c
@@ -79,7 +79,7 @@ struct fail_open {
bool fail_open_active;
};
-static void fail_open_recover(struct fail_open *);
+static void fail_open_recover(struct fail_open *) OVS_REQUIRES(ofproto_mutex);
/* Returns the number of seconds of disconnection after which fail-open mode
* should activate. */
@@ -197,13 +197,15 @@ fail_open_maybe_recover(struct fail_open *fo)
{
if (fail_open_is_active(fo)
&& connmgr_is_any_controller_admitted(fo->connmgr)) {
+ ovs_mutex_lock(&ofproto_mutex);
fail_open_recover(fo);
+ ovs_mutex_unlock(&ofproto_mutex);
}
}
static void
fail_open_recover(struct fail_open *fo)
- OVS_EXCLUDED(ofproto_mutex)
+ OVS_REQUIRES(ofproto_mutex)
{
struct match match;
@@ -272,7 +274,7 @@ fail_open_create(struct ofproto *ofproto, struct connmgr
*mgr)
/* Destroys 'fo'. */
void
fail_open_destroy(struct fail_open *fo)
- OVS_EXCLUDED(ofproto_mutex)
+ OVS_REQUIRES(ofproto_mutex)
{
if (fo) {
if (fail_open_is_active(fo)) {
diff --git a/ofproto/fail-open.h b/ofproto/fail-open.h
index 4056b3e..71442ff 100644
--- a/ofproto/fail-open.h
+++ b/ofproto/fail-open.h
@@ -40,7 +40,7 @@ is_fail_open_rule(const struct rule *rule)
}
struct fail_open *fail_open_create(struct ofproto *, struct connmgr *);
-void fail_open_destroy(struct fail_open *) OVS_EXCLUDED(ofproto_mutex);
+void fail_open_destroy(struct fail_open *) OVS_REQUIRES(ofproto_mutex);
void fail_open_wait(struct fail_open *);
bool fail_open_is_active(const struct fail_open *);
void fail_open_run(struct fail_open *);
diff --git a/ofproto/in-band.c b/ofproto/in-band.c
index f69e94f..4bb47c0 100644
--- a/ofproto/in-band.c
+++ b/ofproto/in-band.c
@@ -395,7 +395,9 @@ in_band_run(struct in_band *ib)
break;
case DEL:
+ ovs_mutex_lock(&ofproto_mutex);
ofproto_delete_flow(ib->ofproto, &rule->match, rule->priority);
+ ovs_mutex_unlock(&ofproto_mutex);
hmap_remove(&ib->rules, &rule->hmap_node);
free(rule);
break;
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 7f7813e..b11bf12 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1890,7 +1890,7 @@ void ofproto_add_flow(struct ofproto *, const struct
match *, int priority,
const struct ofpact *ofpacts, size_t ofpacts_len)
OVS_EXCLUDED(ofproto_mutex);
void ofproto_delete_flow(struct ofproto *, const struct match *, int priority)
- OVS_EXCLUDED(ofproto_mutex);
+ OVS_REQUIRES(ofproto_mutex);
void ofproto_flush_flows(struct ofproto *);
enum ofperr ofproto_check_ofpacts(struct ofproto *,
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index f813c0b..f4f4fd6 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -461,6 +461,7 @@ ofproto_bump_tables_version(struct ofproto *ofproto)
int
ofproto_create(const char *datapath_name, const char *datapath_type,
struct ofproto **ofprotop)
+ OVS_EXCLUDED(ofproto_mutex)
{
const struct ofproto_class *class;
struct ofproto *ofproto;
@@ -530,7 +531,10 @@ ofproto_create(const char *datapath_name, const char
*datapath_type,
if (error) {
VLOG_ERR("failed to open datapath %s: %s",
datapath_name, ovs_strerror(error));
+ ovs_mutex_lock(&ofproto_mutex);
connmgr_destroy(ofproto->connmgr);
+ ofproto->connmgr = NULL;
+ ovs_mutex_unlock(&ofproto_mutex);
ofproto_destroy__(ofproto);
return error;
}
@@ -1486,6 +1490,8 @@ ofproto_rule_delete(struct ofproto *ofproto, struct rule
*rule)
if (ofproto->ofproto_class->rule_delete) {
ofproto->ofproto_class->rule_delete(rule);
}
+
+ /* This may not be the last reference to the rule. */
ofproto_rule_unref(rule);
}
ovs_mutex_unlock(&ofproto_mutex);
@@ -1605,9 +1611,13 @@ ofproto_destroy(struct ofproto *p, bool del)
p->ofproto_class->destruct(p);
/* We should not postpone this because it involves deleting a listening
- * socket which we may want to reopen soon. 'connmgr' should not be used
- * by other threads */
+ * socket which we may want to reopen soon. 'connmgr' may be used by other
+ * threads only if they take the ofproto_mutex and read a non-NULL
+ * 'ofproto->connmgr'. */
+ ovs_mutex_lock(&ofproto_mutex);
connmgr_destroy(p->connmgr);
+ p->connmgr = NULL;
+ ovs_mutex_unlock(&ofproto_mutex);
/* Destroying rules is deferred, must have 'ofproto' around for them. */
ovsrcu_postpone(ofproto_destroy_defer__, p);
@@ -2183,7 +2193,7 @@ ofproto_flow_mod(struct ofproto *ofproto, const struct
ofputil_flow_mod *fm)
void
ofproto_delete_flow(struct ofproto *ofproto,
const struct match *target, int priority)
- OVS_EXCLUDED(ofproto_mutex)
+ OVS_REQUIRES(ofproto_mutex)
{
struct classifier *cls = &ofproto->tables[0].cls;
struct rule *rule;
@@ -2196,10 +2206,12 @@ ofproto_delete_flow(struct ofproto *ofproto,
return;
}
- /* Execute a flow mod. We can't optimize this at all because we didn't
- * take enough locks above to ensure that the flow table didn't already
- * change beneath us. */
- simple_flow_mod(ofproto, target, priority, NULL, 0, OFPFC_DELETE_STRICT);
+ struct rule_collection rules;
+
+ rule_collection_init(&rules);
+ rule_collection_add(&rules, rule);
+ delete_flows__(&rules, OFPRR_DELETE, NULL);
+ rule_collection_destroy(&rules);
}
/* Delete all of the flows from all of ofproto's flow tables, then reintroduce
@@ -5325,7 +5337,15 @@ ofproto_rule_send_removed(struct rule *rule)
minimatch_expand(&rule->cr.match, &fr.match);
fr.priority = rule->cr.priority;
+ /* Synchronize with connmgr_destroy() calls to prevent connmgr disappearing
+ * while we use it. */
ovs_mutex_lock(&ofproto_mutex);
+ struct connmgr *connmgr = rule->ofproto->connmgr;
+ if (!connmgr) {
+ ovs_mutex_unlock(&ofproto_mutex);
+ return;
+ }
+
fr.cookie = rule->flow_cookie;
fr.reason = rule->removed_reason;
fr.table_id = rule->table_id;
@@ -5337,7 +5357,7 @@ ofproto_rule_send_removed(struct rule *rule)
ovs_mutex_unlock(&rule->mutex);
rule->ofproto->ofproto_class->rule_get_stats(rule, &fr.packet_count,
&fr.byte_count, &used);
- connmgr_send_flow_removed(rule->ofproto->connmgr, &fr);
+ connmgr_send_flow_removed(connmgr, &fr);
ovs_mutex_unlock(&ofproto_mutex);
}
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 22d94c7..d813fda 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -241,7 +241,8 @@ int ofproto_type_run(const char *datapath_type);
void ofproto_type_wait(const char *datapath_type);
int ofproto_create(const char *datapath, const char *datapath_type,
- struct ofproto **ofprotop);
+ struct ofproto **ofprotop)
+ OVS_EXCLUDED(ofproto_mutex);
void ofproto_destroy(struct ofproto *, bool del);
int ofproto_delete(const char *name, const char *type);
--
2.1.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
