ofproto-dpif-xlate: Fix treatment of mirrors across patch port.

When the bridges on both sides of a patch port included mirrors, the
translation code incorrectly conflated them instead of treating them as
independent.

Reported-by: Zoltán Balogh <zoltan.balogh@ericsson.com>
Reported-by: Sugesh Chandran <sugesh.chandran@intel.com>
Reported-at: http://openvswitch.org/pipermail/discuss/2016-September/022689.html
Signed-off-by: Ben Pfaff <blp@ovn.org>
Tested-by: Zoltán Balogh <zoltan.balogh@ericsson.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>

1 files changed, 25 insertions, 3 deletions

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9dd720633..74a4b5b7c 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2911,7 +2911,6 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
 
         ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack);
         ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub);
-        ctx->xbridge = peer->xbridge;
         flow->in_port.ofp_port = peer->ofp_port;
         flow->metadata = htonll(0);
         memset(&flow->tunnel, 0, sizeof flow->tunnel);
@@ -2920,6 +2919,26 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
         ctx->conntracked = false;
         clear_conntrack(flow);
 
+        /* When the patch port points to a different bridge, then the mirrors
+         * for that bridge clearly apply independently to the packet, so we
+         * reset the mirror bitmap to zero and then restore it after the packet
+         * returns.
+         *
+         * When the patch port points to the same bridge, this is more of a
+         * design decision: can mirrors be re-applied to the packet after it
+         * re-enters the bridge, or should we treat that as doubly mirroring a
+         * single packet?  The former may be cleaner, since it respects the
+         * model in which a patch port is like a physical cable plugged from
+         * one switch port to another, but the latter may be less surprising to
+         * users.  We take the latter choice, for now at least.  (To use the
+         * former choice, hard-code 'independent_mirrors' to "true".) */
+        mirror_mask_t old_mirrors = ctx->mirrors;
+        bool independent_mirrors = peer->xbridge != ctx->xbridge;
+        if (independent_mirrors) {
+            ctx->mirrors = 0;
+        }
+        ctx->xbridge = peer->xbridge;
+
         /* The bridge is now known so obtain its table version. */
         ctx->xin->tables_version
             = ofproto_dpif_get_tables_version(ctx->xbridge->ofproto);
@@ -2938,10 +2957,10 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
                  * the learning action look at the packet, then drop it. */
                 struct flow old_base_flow = ctx->base_flow;
                 size_t old_size = ctx->odp_actions->size;
-                mirror_mask_t old_mirrors = ctx->mirrors;
+                mirror_mask_t old_mirrors2 = ctx->mirrors;
 
                 xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true);
-                ctx->mirrors = old_mirrors;
+                ctx->mirrors = old_mirrors2;
                 ctx->base_flow = old_base_flow;
                 ctx->odp_actions->size = old_size;
 
@@ -2950,6 +2969,9 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
             }
         }
 
+        if (independent_mirrors) {
+            ctx->mirrors = old_mirrors;
+        }
         ctx->xin->flow = old_flow;
         ctx->xbridge = xport->xbridge;
         ofpbuf_uninit(&ctx->action_set);