On 10 Jul 2023, at 17:34, Mike Pattrick wrote:
> Currently OVS keeps track of which mirrors that each packet has been
> sent to for the purpose of deduplication. However, this doesn't consider
> that openflow rules can make significant changes to packets after
> ingress.
>
> For example, OVN can create OpenFlow rules that turn an echo request
> into an echo response by flipping source/destination addresses and
> setting the ICMP type to Reply. When a mirror is configured, only the
> request gets mirrored even though a response is recieved.
>
> This can cause a false impression of the actual traffic on wire if
> someone inspects the mirror and doesn't see an echo reply even though
> one has been sent.
>
> This patch resets the mirrors every time a packet is modified, so
> mirrors will recieve every copy of a packet that is sent for output.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2155579
> Signed-off-by: Mike Pattrick <m...@redhat.com>
Thanks Mike for fixing my v3 comments. This patch looks good to me.
Acked-by: Eelco Chaudron <echau...@redhat.com>
> ---
> v2: Refactored all code into a single function
> v3: Cleaned up a code change that was incorrectly retained in v2 but
> not needed
> v4: Removed the default case from reset_mirror_ctx()
> ---
> ofproto/ofproto-dpif-xlate.c | 86 ++++++++++++++++++++++++++++++++++++
> tests/ofproto-dpif.at | 6 +--
> 2 files changed, 89 insertions(+), 3 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 29f4daa63..aba0cdb6e 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -6989,6 +6989,90 @@ xlate_ofpact_unroll_xlate(struct xlate_ctx *ctx,
> "cookie=%#"PRIx64, a->rule_table_id, a->rule_cookie);
> }
>
> +/* Reset the mirror context if we modify the packet and would like to mirror
> + * the new copy. */
> +static void
> +reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow *flow, uint8_t
> type)
> +{
> + switch (type) {
> + case OFPACT_STRIP_VLAN:
> + case OFPACT_PUSH_VLAN:
> + case OFPACT_SET_ETH_SRC:
> + case OFPACT_SET_ETH_DST:
> + case OFPACT_PUSH_MPLS:
> + case OFPACT_POP_MPLS:
> + case OFPACT_SET_MPLS_LABEL:
> + case OFPACT_SET_MPLS_TC:
> + case OFPACT_SET_MPLS_TTL:
> + case OFPACT_DEC_MPLS_TTL:
> + case OFPACT_DEC_NSH_TTL:
> + case OFPACT_DEC_TTL:
> + case OFPACT_SET_FIELD:
> + case OFPACT_SET_VLAN_VID:
> + case OFPACT_SET_VLAN_PCP:
> + case OFPACT_ENCAP:
> + case OFPACT_DECAP:
> + case OFPACT_NAT:
> + ctx->mirrors = 0;
> + return;
> + case OFPACT_SET_IPV4_SRC:
> + case OFPACT_SET_IPV4_DST:
> + if (flow->dl_type == htons(ETH_TYPE_IP)) {
> + ctx->mirrors = 0;
> + }
> + return;
> + case OFPACT_SET_IP_DSCP:
> + case OFPACT_SET_IP_ECN:
> + case OFPACT_SET_IP_TTL:
> + if (is_ip_any(flow)) {
> + ctx->mirrors = 0;
> + }
> + return;
> + case OFPACT_SET_L4_SRC_PORT:
> + case OFPACT_SET_L4_DST_PORT:
> + if (is_ip_any(flow) && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) {
> + ctx->mirrors = 0;
> + }
> + return;
> + case OFPACT_OUTPUT_REG:
> + case OFPACT_OUTPUT_TRUNC:
> + case OFPACT_GROUP:
> + case OFPACT_OUTPUT:
> + case OFPACT_CONTROLLER:
> + case OFPACT_RESUBMIT:
> + case OFPACT_GOTO_TABLE:
> + case OFPACT_WRITE_METADATA:
> + case OFPACT_SET_TUNNEL:
> + case OFPACT_REG_MOVE:
> + case OFPACT_STACK_PUSH:
> + case OFPACT_STACK_POP:
> + case OFPACT_LEARN:
> + case OFPACT_ENQUEUE:
> + case OFPACT_SET_QUEUE:
> + case OFPACT_POP_QUEUE:
> + case OFPACT_MULTIPATH:
> + case OFPACT_BUNDLE:
> + case OFPACT_EXIT:
> + case OFPACT_UNROLL_XLATE:
> + case OFPACT_FIN_TIMEOUT:
> + case OFPACT_CLEAR_ACTIONS:
> + case OFPACT_WRITE_ACTIONS:
> + case OFPACT_METER:
> + case OFPACT_SAMPLE:
> + case OFPACT_CLONE:
> + case OFPACT_DEBUG_RECIRC:
> + case OFPACT_DEBUG_SLOW:
> + case OFPACT_CT:
> + case OFPACT_CT_CLEAR:
> + case OFPACT_CHECK_PKT_LARGER:
> + case OFPACT_DELETE_FIELD:
> + case OFPACT_NOTE:
> + case OFPACT_CONJUNCTION:
> + return;
> + }
> + OVS_NOT_REACHED();
> +}
> +
> static void
> do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
> struct xlate_ctx *ctx, bool is_last_action,
> @@ -7030,6 +7114,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> break;
> }
>
> + reset_mirror_ctx(ctx, flow, a->type);
> +
> if (OVS_UNLIKELY(ctx->xin->trace)) {
> struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map);
>
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 6824ce0bb..f242f77f3 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5349,7 +5349,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>
> flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
> AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> - [Datapath actions: 3,push_vlan(vid=17,pcp=0),2
> + [Datapath actions: 3,push_vlan(vid=17,pcp=0),2,3
> ])
>
>
> flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
> @@ -5388,7 +5388,7 @@
> flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x080
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
> actual=`tail -1 stdout | sed 's/Datapath actions: //'`
>
> -expected="push_vlan(vid=17,pcp=0),1,pop_vlan,push_vlan(vid=12,pcp=0),1,2,100"
> +expected="push_vlan(vid=12,pcp=0),100,2,1,pop_vlan,push_vlan(vid=17,pcp=0),1,pop_vlan,push_vlan(vid=12,pcp=0),100,2,1"
> AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout])
> mv stdout expout
> AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout])
> @@ -5656,7 +5656,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>
> flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
> AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> - [Datapath actions: trunc(100),3,push_vlan(vid=17,pcp=0),2
> + [Datapath actions: trunc(100),3,push_vlan(vid=17,pcp=0),2,trunc(100),3
> ])
>
>
> flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
