[ovs-dev] [PATCH] ofproto: Fix use-after-free in bridge destruction with groups.

[Ben Pfaff]

Groups were not destroyed until after lots of other important bridge
data had been destroyed, including the connection manager.  There was an
indirect dependency on the connection manager for bridge destruction
because destroying a group also destroys all of the flows that reference
the group, which in turn causes the ofmonitor to be invoked to report that
the flows had been destroyed.  This commit fixes the problem by destroying
groups earlier.

The problem can be observed by reverting the code changes in this commit
then running "make check-valgrind" with the test that this commit
introduces.

Reported-by: Simon Horman <simon.horman at netronome.com>
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 ofproto/ofproto-dpif.c     |  1 +
 ofproto/ofproto-provider.h |  5 ++++-
 ofproto/ofproto.c          | 11 ++++++++++-
 tests/ofproto.at           | 13 +++++++++++++
 4 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 05a80b7..ac3e48a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1403,6 +1403,7 @@ destruct(struct ofproto *ofproto_)
             ofproto_rule_delete(&ofproto->up, &rule->up);
         }
     }
+    ofproto_group_delete_all(&ofproto->up);
 
     guarded_list_pop_all(&ofproto->pins, &pins);
     LIST_FOR_EACH_POP (pin, list_node, &pins) {
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 527823a..e5739b0 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -521,6 +521,8 @@ bool ofproto_group_lookup(const struct ofproto *ofproto, uint32_t group_id,
 void ofproto_group_ref(struct ofgroup *);
 void ofproto_group_unref(struct ofgroup *);
 
+void ofproto_group_delete_all(struct ofproto *);
+
 /* ofproto class structure, to be defined by each ofproto implementation.
  *
  *
@@ -742,7 +744,8 @@ struct ofproto_class {
      * ===========
      *
      * ->destruct() must also destroy all remaining rules in the ofproto's
-     * tables, by passing each remaining rule to ofproto_rule_delete().
+     * tables, by passing each remaining rule to ofproto_rule_delete(), then
+     * destroy all remaining groups by calling ofproto_group_delete_all().
      *
      * The client will destroy the flow tables themselves after ->destruct()
      * returns.
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 08ba043..f0e6c9a 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1530,7 +1530,6 @@ ofproto_destroy__(struct ofproto *ofproto)
     struct oftable *table;
 
     destroy_rule_executes(ofproto);
-    delete_group(ofproto, OFPG_ALL);
 
     guarded_list_destroy(&ofproto->rule_executes);
     ovs_rwlock_destroy(&ofproto->groups_rwlock);
@@ -6504,6 +6503,16 @@ delete_group(struct ofproto *ofproto, uint32_t group_id)
     ovs_rwlock_unlock(&ofproto->groups_rwlock);
 }
 
+/* Delete all groups from 'ofproto'.
+ *
+ * This is intended for use within an ofproto provider's 'destruct'
+ * function. */
+void
+ofproto_group_delete_all(struct ofproto *ofproto)
+{
+    delete_group(ofproto, OFPG_ALL);
+}
+
 static enum ofperr
 handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh)
 {
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 08585d1..6cf422f 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -679,6 +679,19 @@ OFPST_GROUP reply (OF1.5):
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+dnl This found a use-after-free error in bridge destruction in the
+dnl presence of groups.
+AT_SETUP([ofproto - group add then bridge delete (OpenFlow 1.3)])
+OVS_VSWITCHD_START
+AT_DATA([groups.txt], [dnl
+group_id=1234,type=all,bucket=output:10
+group_id=1235,type=all,bucket=output:10
+])
+AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn add-groups br0 groups.txt])
+AT_CHECK([ovs-vsctl del-br br0])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([ofproto - mod-port (OpenFlow 1.0)])
 OVS_VSWITCHD_START
 for command_config_state in \
-- 
2.1.3