In the current implementation, mirror_packet() is invoked at the end of
compose_output_action__(). This logic is flawed because it triggers
mirroring even when the packet is not actually being output to a port,
but is instead being recirculated (e.g., for bond member selection).
This causes two major issues:
1. Redundant Mirrors: If a packet is mirrored during the recirculation
phase and then mirrored again when it finally hits an actual output
action, the mirror destination receives duplicate traffic.
2. False Mirroring on Dropped Packets: Mirroring should only occur on
actual output. If for some reason the post-recurculation rules are
not available, we should not be mirroring the packet. For example,
if a bond is removed while we have the first datapath flow installed,
but the packet after recirculation goes to userspace via MISS upcall
and gets dropped, then we should not mirror that packet.
This patch refactors compose_output_action__() to ensure mirror_packet()
is only called within the actual output.
Signed-off-by: Jun Gu <[email protected]>
---
ofproto/ofproto-dpif-xlate.c | 21 ++++++++++++++-----
tests/ofproto-dpif.at | 40 ++++++++++++++++++++++++++++++++++++
2 files changed, 56 insertions(+), 5 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 32ee49e7e..335a7e68f 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4560,6 +4560,9 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t
ofp_port,
/* Commit accumulated flow updates before output. */
xlate_commit_actions(ctx);
+ /* Flag to decide whether the output should be mirrored. */
+ bool mirrored = true;
+
if (xr && bond_use_lb_output_action(xport->xbundle->bond)) {
/*
* If bond mode is balance-tcp and optimize balance tcp is enabled
@@ -4589,6 +4592,9 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t
ofp_port,
/* Recirc action. */
nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_RECIRC,
xr->recirc_id);
+
+ /* Avoid mirroring on recirculation. */
+ mirrored = false;
} else if (is_native_tunnel) {
/* Output to native tunnel port. */
native_tunnel_output(ctx, xport, flow, odp_port, truncate,
@@ -4629,12 +4635,17 @@ compose_output_action__(struct xlate_ctx *ctx,
ofp_port_t ofp_port,
ctx->sflow_odp_port = odp_port;
ctx->sflow_n_outputs++;
ctx->nf_output_iface = ofp_port;
- }
- if (mbridge_has_mirrors(ctx->xbridge->mbridge) && xport->xbundle) {
- mirror_packet(ctx, xport->xbundle,
- xbundle_mirror_dst(xport->xbundle->xbridge,
- xport->xbundle));
+ /* Mirror only when we actually perform an output action.
+ * Recirculation does not send the packet anywhere, so it
+ * should not trigger mirroring.
+ */
+ if (mirrored && mbridge_has_mirrors(ctx->xbridge->mbridge)
+ && xport->xbundle) {
+ mirror_packet(ctx, xport->xbundle,
+ xbundle_mirror_dst(xport->xbundle->xbridge,
+ xport->xbundle));
+ }
}
out:
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index b791bf4e4..86ade6066 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -6112,6 +6112,46 @@ AT_CHECK([tail -1 stdout | grep -E
"trunc\(200\),2,trunc\(300\),3,100|trunc\(300
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - mirroring on balance-tcp bonding with dp_hash])
+AT_KEYWORDS([mirror mirrors mirroring])
+OVS_VSWITCHD_START(
+ [add-bond br0 bond0 p1 p2 bond_mode=balance-tcp lacp=active \
+ other-config:lacp-time=fast other-config:bond-rebalance-interval=0 -- \
+ set interface p1 type=dummy options:pstream=punix:$OVS_RUNDIR/p1.sock
ofport_request=1 -- \
+ set interface p2 type=dummy options:pstream=punix:$OVS_RUNDIR/p2.sock
ofport_request=2 -- \
+ add-br br1 -- \
+ set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
+ set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \
+ fail-mode=standalone -- \
+ add-bond br1 bond1 p3 p4 bond_mode=balance-tcp lacp=active \
+ other-config:lacp-time=fast other-config:bond-rebalance-interval=0 -- \
+ set interface p3 type=dummy options:stream=unix:$OVS_RUNDIR/p1.sock
ofport_request=3 -- \
+ set interface p4 type=dummy options:stream=unix:$OVS_RUNDIR/p2.sock
ofport_request=4 -- \
+ add-port br1 p5 -- set interface p5 ofport_request=5 type=dummy
+])
+AT_CHECK([ovs-appctl netdev-dummy/set-admin-state up], 0, [OK
+])
+
+# Waits for all ifaces enabled.
+OVS_WAIT_UNTIL([test `ovs-appctl bond/show | grep -c "may_enable: true"` -ge
4])
+
+add_of_ports br1 6
+AT_CHECK([ovs-vsctl \
+ set Bridge br1 mirrors=@m --\
+ --id=@bond1 get Port bond1 -- --id=@p6 get Port p6 --\
+ --id=@m create Mirror name=mymirror select_dst_port=@bond1
select_src_port=@bond1 output_port=@p6],
+ [0], [ignore])
+
+# Sends a packet to trigger recirculation.
+AT_CHECK([ovs-appctl netdev-dummy/receive p5
"in_port(5),eth(src=50:54:00:00:00:05,dst=50:54:00:00:01:00),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1)"])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep 'in_port(p5)' | \
+ sed 's/.*actions://' | grep -c p6 | tr -d '\n'], [0], [1])
+
+OVS_VSWITCHD_STOP()
+AT_CLEANUP
+
+
# This test verifies that the table ID is preserved across recirculation
# when a resubmit action requires it (because the action is relative to
# the current table rather than specifying a table).
