ofproto-dpif-xlate: Don't consider mirrors used when excluded by VLAN.

Mirrors can be configured to select packets for mirroring on the basis
of multiple criteria: input ports, output ports, and VLANs.  A packet P
is to be mirrored if there exists a mirror M such that either:

    - P ingresses on an input port selected by M, or

    - P egresses on an output port selected by M

AND P is in a VLAN selected by M.

In addition, every mirror has a destination, which can be an output port
or an output VLAN.  Either way, if a packet is mirrored to a particular
destination, it is done only once, even if different mirrors both select
a packet and have the same destination.

Since commit efbc3b7c4006c (ofproto-dpif-xlate: Rewrite mirroring to better
fit flow translation.), these requirements have been implemented
incorrectly: if a packet satisfies one of the bulleted requirements
above for mirror M1, but not the VLAN selection requirement for M1,
then it was not sent to M's destination, but it was still considered
as having been sent to M1's destination for the purpose of avoid output
duplication.  Thus, if P satisfied *all* of the requirements for a
second mirror M2, if M1 and M2 had the same destination, the packet was
still not mirrored.  This commit fixes that problem.

(The issue only occurred if M1 happened to have a smaller index than
M2 in OVS's internal data structures.  That's just a matter of luck.)

Reported-by: Huanle Han <hanxueluo@gmail.com>
Reported-at: http://openvswitch.org/pipermail/dev/2016-January/064531.html
Fixes: 7efbc3b7c4006c (ofproto-dpif-xlate: Rewrite mirroring to better fit flow translation.)
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Jarno Rajahalme <jarno@ovn.org>

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index c486201..37f2879 100644 (file)
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1621,10 +1621,15 @@ lookup_input_bundle(const struct xbridge *xbridge, ofp_port_t in_port,
     return NULL;
 }
 
+/* Mirrors the packet represented by 'ctx' to appropriate mirror destinations,
+ * given the packet is ingressing or egressing on 'xbundle', which has ingress
+ * or egress (as appropriate) mirrors 'mirrors'. */
 static void
 mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
               mirror_mask_t mirrors)
 {
+    /* Figure out what VLAN the packet is in (because mirrors can select
+     * packets on basis of VLAN). */
     bool warn = ctx->xin->packet != NULL;
     uint16_t vid = vlan_tci_to_vid(ctx->xin->flow.vlan_tci);
     if (!input_vid_is_valid(vid, xbundle, warn)) {
@@ -1640,9 +1645,6 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
         return;
     }
 
-    /* Record these mirrors so that we don't mirror to them again. */
-    ctx->mirrors |= mirrors;
-
     if (ctx->xin->resubmit_stats) {
         mirror_update_stats(xbridge->mbridge, mirrors,
                             ctx->xin->resubmit_stats->n_packets,
@@ -1656,27 +1658,36 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
         entry->u.mirror.mirrors = mirrors;
     }
 
+    /* 'mirrors' is a bit-mask of candidates for mirroring.  Iterate as long as
+     * some candidates remain.  */
     while (mirrors) {
         const unsigned long *vlans;
         mirror_mask_t dup_mirrors;
         struct ofbundle *out;
         int out_vlan;
 
+        /* Get the details of the mirror represented by the rightmost 1-bit. */
         bool has_mirror = mirror_get(xbridge->mbridge, raw_ctz(mirrors),
                                      &vlans, &dup_mirrors, &out, &out_vlan);
         ovs_assert(has_mirror);
 
+        /* If this mirror selects on the basis of VLAN, and it does not select
+         * 'vlan', then discard this mirror and go on to the next one. */
         if (vlans) {
             ctx->wc->masks.vlan_tci |= htons(VLAN_CFI | VLAN_VID_MASK);
         }
-
         if (vlans && !bitmap_is_set(vlans, vlan)) {
             mirrors = zero_rightmost_1bit(mirrors);
             continue;
         }
 
-        mirrors &= ~dup_mirrors;
+        /* Record the mirror, and the mirrors that output to the same
+         * destination, so that we don't mirror to them again.  This must be
+         * done now to ensure that output_normal(), below, doesn't recursively
+         * output to the same mirrors. */
         ctx->mirrors |= dup_mirrors;
+
+        /* Send the packet to the mirror. */
         if (out) {
             struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
             struct xbundle *out_xbundle = xbundle_lookup(xcfg, out);
@@ -1694,6 +1705,10 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
                 }
             }
         }
+
+        /* output_normal() could have recursively output (to different
+         * mirrors), so make sure that we don't send duplicates. */
+        mirrors &= ~ctx->mirrors;
     }
 }
 

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index a372d36..5fdf5e6 100644 (file)
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -4148,6 +4148,32 @@ AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+# This verifies that we don't get duplicate mirroring when mirror_packet()
+# might be invoked recursively, as a check against regression.
+AT_SETUP([ofproto-dpif - multiple VLAN output mirrors])
+OVS_VSWITCHD_START
+add_of_ports br0 1 2 3
+ovs-vsctl \
+        -- set Bridge br0 fail-mode=standalone mirrors=@m1,@m2 \
+       -- --id=@m1 create Mirror name=m1 select_all=true output_vlan=500 \
+       -- --id=@m2 create Mirror name=m2 select_all=true output_vlan=501 \
+       -- set Port br0 tag=0 \
+       -- set Port p1 tag=0 \
+       -- set Port p2 tag=500 \
+       -- set Port p3 tag=501
+
+flow='in_port=1'
+AT_CHECK([ovs-appctl ofproto/trace br0 "$flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout | sed 's/Datapath actions: //
+s/,/\
+/g' | sort], [0], [100
+2
+3
+])
+
+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