[ovs-dev] [PATCH 26/26] ofproto-dpif-xlate: Fix mirroring interaction with recirculation.

[Ben Pfaff]

Before this commit, mirroring state was not preserved across recirculation,
which could result in a packet being mirrored to the same destination both
before and after recirculation.  This commit fixes the problem and adds a
test to avoid regression.

Found by inspection.

Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 ofproto/ofproto-dpif-rid.c   |  2 ++
 ofproto/ofproto-dpif-rid.h   |  2 ++
 ofproto/ofproto-dpif-xlate.c |  4 ++++
 tests/ofproto-dpif.at        | 28 ++++++++++++++++++++++++++++
 4 files changed, 36 insertions(+)

diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
index e2867b7..b8207c4 100644
--- a/ofproto/ofproto-dpif-rid.c
+++ b/ofproto/ofproto-dpif-rid.c
@@ -137,6 +137,7 @@ recirc_metadata_hash(const struct recirc_state *state)
         hash = hash_words64((const uint64_t *) state->stack->data,
                             state->stack->size / sizeof(uint64_t), hash);
     }
+    hash = hash_int(state->mirrors, hash);
     hash = hash_int(state->action_set_len, hash);
     if (state->ofpacts_len) {
         hash = hash_words64(ALIGNED_CAST(const uint64_t *, state->ofpacts),
@@ -156,6 +157,7 @@ recirc_metadata_equal(const struct recirc_state *a,
             && (((!a->stack || !a->stack->size) &&
                  (!b->stack || !b->stack->size))
                 || (a->stack && b->stack && ofpbuf_equal(a->stack, b->stack)))
+            && a->mirrors == b->mirrors
             && a->action_set_len == b->action_set_len
             && ofpacts_equal(a->ofpacts, a->ofpacts_len,
                              b->ofpacts, b->ofpacts_len));
diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
index 9caa662..11ae486 100644
--- a/ofproto/ofproto-dpif-rid.h
+++ b/ofproto/ofproto-dpif-rid.h
@@ -23,6 +23,7 @@
 #include "cmap.h"
 #include "list.h"
 #include "ofp-actions.h"
+#include "ofproto-dpif-mirror.h"
 #include "ovs-thread.h"
 
 struct ofproto_dpif;
@@ -135,6 +136,7 @@ struct recirc_state {
     struct ofproto_dpif *ofproto; /* Post-recirculation bridge. */
     struct recirc_metadata metadata; /* Flow metadata. */
     struct ofpbuf *stack;         /* Stack if any. */
+    mirror_mask_t mirrors;        /* Mirrors already output. */
 
     /* Actions to be translated on recirculation. */
     uint32_t action_set_len;      /* How much of 'ofpacts' consists of an
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index dc16a39..e2596d9 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3522,6 +3522,7 @@ compose_recirculate_action(struct xlate_ctx *ctx)
         .ofproto = ctx->xbridge->ofproto,
         .metadata = md,
         .stack = &ctx->stack,
+        .mirrors = ctx->mirrors,
         .action_set_len = ctx->recirc_action_offset,
         .ofpacts_len = ctx->action_set.size,
         .ofpacts = ctx->action_set.data,
@@ -4868,6 +4869,9 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
             ofpbuf_put(&ctx.stack, state->stack->data, state->stack->size);
         }
 
+        /* Restore mirror state. */
+        ctx.mirrors = state->mirrors;
+
         /* Restore action set, if any. */
         if (state->action_set_len) {
             const struct ofpact *a;
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 9bfb794..58c426b 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -4144,6 +4144,34 @@ AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+# This test verifies that mirror state is preserved across recirculation.
+#
+# Otherwise, post-recirculation the ingress and the output to port 4
+# would cause the packet to be mirrored to port 3 a second time.
+AT_SETUP([ofproto-dpif - mirroring with recirculation])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], 1, 2, 3, 4)
+ovs-vsctl \
+        set Bridge br0 mirrors=@m --\
+        --id=@p3 get Port p3 --\
+        --id=@m create Mirror name=mymirror select_all=true output_port=@p3
+
+AT_DATA([flows.txt], [dnl
+in_port=1 actions=2,debug_recirc,4
+])
+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: 3,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: 4
+])
+
+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.1.3