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 <b...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
