ofproto: Don't use connmgr after destruction.

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: f695ebf ("ofproto: Postpone sending flow removed messages.") Signed-off-by: Jarno Rajahalme <[email protected]> Acked-by: Ben Pfaff <[email protected]>
water0615 · Sep 13, 2016 · 25010c6 · 25010c6
1 parent 7c127f2
commit 25010c6
Show file tree

Hide file tree

Showing 8 changed files with 55 additions and 20 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);