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 egress. If mirroring is coupled with recirculation in the first datapath flow, the packet is mirrored before its final fate is known. For instance, if a bond member is removed while the first flow is installed, the packet may undergo a MISS upcall after recirculation and subsequently be dropped by the userspace. In this case, the packet never leaves the switch, yet it has already been mirrored, creating "ghost" traffic in the mirror destination. This patch refactors compose_output_action__() to ensure mirror_packet() is only called within the actual output, we ensure that packets are only mirrored when they are successfully committed to a destination port, aligning the mirroring logic with the true egress of the traffic. Signed-off-by: Jun Gu <[email protected]> Change-Id: Ie4b802794af448a417f78ada1d2de53fac327ca0 --- 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). -- 2.34.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
