From 83a31283dcb3d74b169c2c36df64994d9c8c964c Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 22 Jan 2016 15:58:55 -0800 Subject: [PATCH] ofproto-dpif-xlate: Fix recirculation for resubmit to current table. When recirculation defers actions for processing later, it decides based on the actions being saved whether it needs to record the table and cookie from which they originated. Until now, it was thought that this was only important for actions that send packets to the controller (because those actions send the table ID and cookie). This overlooked a special case of the "resubmit" action which also depends on the current table ID, which meant that this special case malfunctioned if it came after recirculation. This commit fixes the problem, and adds a test. Found while testing another feature under development. Signed-off-by: Ben Pfaff Acked-by: Jarno Rajahalme --- ofproto/ofproto-dpif-xlate.c | 21 ++++++++++++++------- tests/ofproto-dpif.at | 25 +++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 65491918a..4b25bf45f 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4165,8 +4165,7 @@ recirc_put_unroll_xlate(struct xlate_ctx *ctx) /* Copy remaining actions to the action_set to be executed after recirculation. * UNROLL_XLATE action is inserted, if not already done so, before actions that - * may generate asynchronous messages from the current table and without - * matching another rule. */ + * may depend on the current table ID or flow cookie. */ static void recirc_unroll_actions(const struct ofpact *ofpacts, size_t ofpacts_len, struct xlate_ctx *ctx) @@ -4175,17 +4174,25 @@ recirc_unroll_actions(const struct ofpact *ofpacts, size_t ofpacts_len, OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { switch (a->type) { - /* May generate asynchronous messages. */ case OFPACT_OUTPUT_REG: case OFPACT_GROUP: case OFPACT_OUTPUT: case OFPACT_CONTROLLER: case OFPACT_DEC_MPLS_TTL: case OFPACT_DEC_TTL: + /* These actions may generate asynchronous messages, which include + * table ID and flow cookie information. */ recirc_put_unroll_xlate(ctx); break; - /* These may not generate PACKET INs. */ + case OFPACT_RESUBMIT: + if (ofpact_get_RESUBMIT(a)->table_id == 0xff) { + /* This resubmit action is relative to the current table, so we + * need to track what table that is.*/ + recirc_put_unroll_xlate(ctx); + } + break; + case OFPACT_SET_TUNNEL: case OFPACT_REG_MOVE: case OFPACT_SET_FIELD: @@ -4193,8 +4200,7 @@ recirc_unroll_actions(const struct ofpact *ofpacts, size_t ofpacts_len, case OFPACT_STACK_POP: case OFPACT_LEARN: case OFPACT_WRITE_METADATA: - case OFPACT_RESUBMIT: /* May indirectly generate PACKET INs, */ - case OFPACT_GOTO_TABLE: /* but from a different table and rule. */ + case OFPACT_GOTO_TABLE: case OFPACT_ENQUEUE: case OFPACT_SET_VLAN_VID: case OFPACT_SET_VLAN_PCP: @@ -4228,11 +4234,12 @@ recirc_unroll_actions(const struct ofpact *ofpacts, size_t ofpacts_len, case OFPACT_DEBUG_RECIRC: case OFPACT_CT: case OFPACT_NAT: + /* These may not generate PACKET INs. */ break; - /* These need not be copied for restoration. */ case OFPACT_NOTE: case OFPACT_CONJUNCTION: + /* These need not be copied for restoration. */ continue; } /* Copy the action over. */ diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 3a8c893e2..bfb1b5678 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -4178,6 +4178,31 @@ AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: 4 OVS_VSWITCHD_STOP AT_CLEANUP +# This test verifies that the table ID is preserved across recirculation +# when a resubmit action requires it (because the action is relative to +# the current table rather than specifying a table). +AT_SETUP([ofproto-dpif - resubmit with recirculation]) +OVS_VSWITCHD_START +ADD_OF_PORTS([br0], [1], [2], [3]) + +AT_DATA([flows.txt], [dnl +table=0 in_port=1 actions=2,resubmit(,1) +table=1 in_port=1 actions=debug_recirc,resubmit:55 +table=1 in_port=55 actions=3 +]) +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) + +flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)" +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow" -generate], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: 2,recirc(0x1) +]) +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow,recirc_id(1)" -generate], [0], [stdout]) +AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: 3 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + # Two testcases below are for the ofproto/trace command # The first one tests all correct syntax: # ofproto/trace [dp_name] odp_flow [-generate|packet] -- 2.20.1