From daab0ae6bb558411f24cee6a7bb32599a5f9e363 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Tue, 23 Jun 2015 11:38:56 -0700 Subject: [PATCH] ofproto: Fix use-after-free in bridge destruction with groups. 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 Signed-off-by: Ben Pfaff Reviewed-by: Simon Horman --- 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 04f6229d4..13d2f21e1 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 527823aac..e5739b083 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 acf504f5d..c794f07d0 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -1532,7 +1532,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); @@ -6506,6 +6505,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 3c0c5863e..a564c8156 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.39.5