The current implementation triggers mirroring even when a packet is being recirculated (e.g., for bond member selection). This leads to incorrect mirroring behavior in the following ways:
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: 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 not invoked during recirculation and avoid mirroring on recirculation. Signed-off-by: Jun Gu <[email protected]> --- ofproto/ofproto-dpif-xlate.c | 9 ++++++++- tests/ofproto-dpif.at | 37 ++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 32ee49e7e..6f94c007d 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4556,6 +4556,8 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, out_port = odp_port; } + bool skip_mirrors = false; + if (out_port != ODPP_NONE) { /* Commit accumulated flow updates before output. */ xlate_commit_actions(ctx); @@ -4589,6 +4591,10 @@ 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); + + /* Recirculation does not send the packet anywhere, so it + * should not trigger mirroring. */ + skip_mirrors = true; } else if (is_native_tunnel) { /* Output to native tunnel port. */ native_tunnel_output(ctx, xport, flow, odp_port, truncate, @@ -4631,7 +4637,8 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, ctx->nf_output_iface = ofp_port; } - if (mbridge_has_mirrors(ctx->xbridge->mbridge) && xport->xbundle) { + if (!skip_mirrors && mbridge_has_mirrors(ctx->xbridge->mbridge) + && xport->xbundle) { mirror_packet(ctx, xport->xbundle, xbundle_mirror_dst(xport->xbundle->xbridge, xport->xbundle)); diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index b791bf4e4..895b1e8af 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -6112,6 +6112,43 @@ 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 +]) + +# Waits for all ifaces enabled. +OVS_WAIT_UNTIL([test `ovs-appctl bond/show | grep -c "may_enable: true"` -ge 4]) + +add_of_ports br1 5 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
