ofproto-dpif: Fix rule delete deadlock.

When doing rule expiration, ofproto_rule_delete__() take the
ofproto_mutex and calls rule_get_stats().  rule_get_stats() can do an
xlate_actions() which may take the ofproto_mutex, causing a deadlock.

Signed-off-by: Ethan Jackson <ethan@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index a0ba728..2dbb8c6 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -105,7 +105,8 @@ struct rule_dpif {
     uint64_t byte_count OVS_GUARDED;    /* Number of bytes received. */
 };
 
-static void rule_get_stats(struct rule *, uint64_t *packets, uint64_t *bytes);
+static void rule_get_stats(struct rule *, uint64_t *packets, uint64_t *bytes,
+                           bool push);
 static struct rule_dpif *rule_dpif_cast(const struct rule *);
 
 struct ofbundle {
@@ -1678,9 +1679,11 @@ get_tables(struct ofproto *ofproto_, struct ofp12_table_stats *ots)
     strcpy(ots->name, "classifier");
 
     dpif_get_dp_stats(ofproto->backer->dpif, &s);
-    rule_get_stats(&ofproto->miss_rule->up, &n_miss, &n_bytes);
-    rule_get_stats(&ofproto->no_packet_in_rule->up, &n_no_pkt_in, &n_bytes);
-    rule_get_stats(&ofproto->drop_frags_rule->up, &n_dropped_frags, &n_bytes);
+    rule_get_stats(&ofproto->miss_rule->up, &n_miss, &n_bytes, true);
+    rule_get_stats(&ofproto->no_packet_in_rule->up, &n_no_pkt_in, &n_bytes,
+                   true);
+    rule_get_stats(&ofproto->drop_frags_rule->up, &n_dropped_frags, &n_bytes,
+                   true);
 
     n_lookup = s.n_hit + s.n_missed - n_dropped_frags;
     ots->lookup_count = htonll(n_lookup);
@@ -4880,7 +4883,8 @@ rule_destruct(struct rule *rule_)
 }
 
 static void
-rule_get_stats(struct rule *rule_, uint64_t *packets, uint64_t *bytes)
+rule_get_stats(struct rule *rule_, uint64_t *packets, uint64_t *bytes,
+               bool push)
 {
     struct rule_dpif *rule = rule_dpif_cast(rule_);
 
@@ -4888,7 +4892,9 @@ rule_get_stats(struct rule *rule_, uint64_t *packets, uint64_t *bytes)
      * action, can cause rules to be added and deleted.  This can corrupt our
      * caller's datastructures which assume that rule_get_stats() doesn't have
      * an impact on the flow table. To be safe, we disable miss handling. */
-    push_all_stats__(false);
+    if (push) {
+        push_all_stats__(false);
+    }
 
     /* Start from historical data for 'rule' itself that are no longer tracked
      * in facets.  This counts, for example, facets that have expired. */

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 65d7e33..8a6e960 100644 (file)
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1228,9 +1228,10 @@ struct ofproto_class {
     /* Obtains statistics for 'rule', storing the number of packets that have
      * matched it in '*packet_count' and the number of bytes in those packets
      * in '*byte_count'.  UINT64_MAX indicates that the packet count or byte
-     * count is unknown. */
+     * count is unknown.  If 'push' is true, the provider may try to update
+     * statistics for 'rule', possibly taking the 'ofproto_mutex'.*/
     void (*rule_get_stats)(struct rule *rule, uint64_t *packet_count,
-                           uint64_t *byte_count)
+                           uint64_t *byte_count, bool push)
         /* OVS_EXCLUDED(ofproto_mutex) */;
 
     /* Applies the actions in 'rule' to 'packet'.  (This implements sending

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 030ec58..790aa71 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -3423,7 +3423,7 @@ handle_flow_stats_request(struct ofconn *ofconn,
         fs.idle_age = age_secs(now - used);
         fs.hard_age = age_secs(now - modified);
         ofproto->ofproto_class->rule_get_stats(rule, &fs.packet_count,
-                                               &fs.byte_count);
+                                               &fs.byte_count, true);
         fs.ofpacts = actions->ofpacts;
         fs.ofpacts_len = actions->ofpacts_len;
 
@@ -3453,8 +3453,8 @@ flow_stats_ds(struct rule *rule, struct ds *results)
     struct rule_actions *actions;
     long long int created;
 
-    rule->ofproto->ofproto_class->rule_get_stats(rule,
-                                                 &packet_count, &byte_count);
+    rule->ofproto->ofproto_class->rule_get_stats(rule, &packet_count,
+                                                 &byte_count, true);
 
     ovs_mutex_lock(&rule->mutex);
     actions = rule_get_actions__(rule);
@@ -3567,7 +3567,7 @@ handle_aggregate_stats_request(struct ofconn *ofconn,
         uint64_t byte_count;
 
         ofproto->ofproto_class->rule_get_stats(rule, &packet_count,
-                                               &byte_count);
+                                               &byte_count, true);
 
         if (packet_count == UINT64_MAX) {
             unknown_packets = true;
@@ -4191,7 +4191,7 @@ ofproto_rule_send_removed(struct rule *rule, uint8_t reason)
     fr.hard_timeout = rule->hard_timeout;
     ovs_mutex_unlock(&rule->mutex);
     rule->ofproto->ofproto_class->rule_get_stats(rule, &fr.packet_count,
-                                                 &fr.byte_count);
+                                                 &fr.byte_count, false);
 
     connmgr_send_flow_removed(rule->ofproto->connmgr, &fr);
 }