ofproto-dpif-xlate: Roll back group bucket actions after every bucket.

We used to roll back group bucket changes only for 'all' and
'indirect' group types, but the expected semantics of all group types
is that any changes by the group bucket are not visible after the
group has been executed.

Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 7de8c96..b0f3f89 100644 (file)
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2158,6 +2158,7 @@ xlate_group_bucket(struct xlate_ctx *ctx, const struct ofputil_bucket *bucket)
 {
     uint64_t action_list_stub[1024 / 8];
     struct ofpbuf action_list, action_set;
+    struct flow old_flow = ctx->xin->flow;
 
     ofpbuf_use_const(&action_set, bucket->ofpacts, bucket->ofpacts_len);
     ofpbuf_use_stub(&action_list, action_list_stub, sizeof action_list_stub);
@@ -2169,6 +2170,25 @@ xlate_group_bucket(struct xlate_ctx *ctx, const struct ofputil_bucket *bucket)
 
     ofpbuf_uninit(&action_set);
     ofpbuf_uninit(&action_list);
+
+    /* Roll back flow to previous state.
+     * This is equivalent to cloning the packet for each bucket.
+     *
+     * As a side effect any subsequently applied actions will
+     * also effectively be applied to a clone of the packet taken
+     * just before applying the all or indirect group.
+     *
+     * Note that group buckets are action sets, hence they cannot modify the
+     * main action set.  Also any stack actions are ignored when executing an
+     * action set, so group buckets cannot change the stack either. */
+    ctx->xin->flow = old_flow;
+
+    /* The fact that the group bucket exits (for any reason) does not mean that
+     * the translation after the group action should exit.  Specifically, if
+     * the group bucket recirculates (which typically modifies the packet), the
+     * actions after the group action must continue processing with the
+     * original, not the recirculated packet! */
+    ctx->exit = false;
 }
 
 static void
@@ -2176,19 +2196,11 @@ xlate_all_group(struct xlate_ctx *ctx, struct group_dpif *group)
 {
     const struct ofputil_bucket *bucket;
     const struct list *buckets;
-    struct flow old_flow = ctx->xin->flow;
 
     group_dpif_get_buckets(group, &buckets);
 
     LIST_FOR_EACH (bucket, list_node, buckets) {
         xlate_group_bucket(ctx, bucket);
-        /* Roll back flow to previous state.
-         * This is equivalent to cloning the packet for each bucket.
-         *
-         * As a side effect any subsequently applied actions will
-         * also effectively be applied to a clone of the packet taken
-         * just before applying the all or indirect group. */
-        ctx->xin->flow = old_flow;
     }
 }
 

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 29884c2..983cc38 100644 (file)
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -367,6 +367,18 @@ AT_CHECK([tail -1 stdout], [0],
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([ofproto-dpif - group actions have no effect afterwards])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], [1], [10])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 'group_id=1234,type=select,bucket=set_field:192.168.3.90->ip_src,output:10'])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip actions=group:1234,output:10'])
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=1,nw_tos=0,nw_ttl=128,icmp_type=8,icmp_code=0'], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: set(ipv4(src=192.168.3.90,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no)),10,set(ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no)),10
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([ofproto-dpif - all group in action set])
 OVS_VSWITCHD_START
 ADD_OF_PORTS([br0], [1], [10], [11])