From 862d8eed48618b5bfd65e61613ff267a66caa980 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 25 Oct 2013 13:31:50 -0700 Subject: [PATCH] ofproto: Avoid abandoning an ofopgroup without committing it. Commit e3b5693319c (Fix table checking for goto table instruction.) moved action checking into modify_flows__(), for good reason, but as a side effect made modify_flows__() abandon and never commit the ofopgroup that it started, if action checking failed. This commit fixes the problem. The following commands, run under "make sandbox", illustrate the problem. Without this change, the final command hangs because the barrier request that ovs-ofctl sends never gets a response (because barriers wait for all ofopgroups to complete, which never happens). With this commit, the commands complete quickly: ovs-vsctl add-br br0 ovs-vsctl set bridge br0 protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13 ovs-ofctl add-flow -O OpenFlow11 br0 table=1,action=goto_table:2 ovs-ofctl add-flow -O OpenFlow11 br0 table=1,action=goto_table:1 Reported-by: Jarno Rajahalme Signed-off-by: Ben Pfaff --- ofproto/ofproto.c | 21 +++++++++++++-------- tests/ofproto.at | 26 ++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 8fa5e8473..1d340c9b2 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -4045,6 +4045,19 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn, enum ofperr error; size_t i; + /* Verify actions before we start to modify any rules, to avoid partial + * flow table modifications. */ + for (i = 0; i < rules->n; i++) { + struct rule *rule = rules->rules[i]; + + error = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow, + u16_to_ofp(ofproto->max_ports), rule->table_id, + request && request->version > OFP10_VERSION); + if (error) { + return error; + } + } + type = fm->command == OFPFC_ADD ? OFOPERATION_REPLACE : OFOPERATION_MODIFY; group = ofopgroup_create(ofproto, ofconn, request, fm->buffer_id); error = OFPERR_OFPBRC_EPERM; @@ -4063,14 +4076,6 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn, continue; } - /* Verify actions, enforce consistency check on OF1.1+. */ - error = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow, - u16_to_ofp(ofproto->max_ports), rule->table_id, - request && request->version > OFP10_VERSION); - if (error) { - return error; - } - actions_changed = !ofpacts_equal(fm->ofpacts, fm->ofpacts_len, rule->actions->ofpacts, rule->actions->ofpacts_len); diff --git a/tests/ofproto.at b/tests/ofproto.at index 0e1d41ba0..668246770 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -275,6 +275,32 @@ AT_CHECK([ovs-ofctl -O OpenFlow11 dump-flows br0 | ofctl_strip], [0], [OFPST_FLO OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto - flow_mod negative test (OpenFlow 1.1)]) +OVS_VSWITCHD_START( + [set bridge br0 protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13]) +AT_CHECK([ovs-ofctl add-flow -O OpenFlow11 br0 table=1,action=goto_table:2]) +AT_CHECK([ovs-ofctl add-flow -O OpenFlow11 br0 table=1,action=goto_table:1], + [1], [], [stderr]) + +# The output should look like this: +# +# OFPT_ERROR (OF1.1) (xid=0x2): OFPBRC_BAD_TABLE_ID +# OFPT_FLOW_MOD (OF1.1) (xid=0x2): +# (***truncated to 64 bytes from 160***) +# 00000000 02 0e 00 a0 00 00 00 02-00 00 00 00 00 00 00 00 |................| +# 00000010 00 00 00 00 00 00 00 00-01 00 00 00 00 00 80 00 |................| +# 00000020 ff ff ff ff ff ff ff ff-ff ff ff ff 00 00 00 00 |................| +# 00000030 00 00 00 58 00 00 00 00-00 00 03 ff 00 00 00 00 |...X............| +# +# This 'sed' command captures the error message but drops details. +AT_CHECK([sed '/truncated/d +/^000000.0/d' stderr | STRIP_XIDS], [0], + [OFPT_ERROR (OF1.1): OFPBRC_BAD_TABLE_ID +OFPT_FLOW_MOD (OF1.1): +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ofproto - set-field flow_mod commands (NXM)]) OVS_VSWITCHD_START AT_CHECK([ovs-ofctl add-flow br0 ipv6,table=1,in_port=3,actions=drop]) -- 2.43.0