On Thu, Jun 29, 2023 at 10:06 AM Eelco Chaudron <echau...@redhat.com> wrote: > > > > On 26 Jun 2023, at 15:43, 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> > > --- > > v2: Refactored all code into a single function > > v3: Cleaned up a code change that was incorrectly retained in v2 but > > not needed > > --- > > ofproto/ofproto-dpif-xlate.c | 87 ++++++++++++++++++++++++++++++++++++ > > tests/ofproto-dpif.at | 6 +-- > > 2 files changed, 90 insertions(+), 3 deletions(-) > > > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > index 29f4daa63..5c2c6c792 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -6989,6 +6989,91 @@ 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) > > +{ > > How about the change below? This will give a compile error if new actions get > defined.
Seems reasonable to me! I'll resubmit with that change. > > The rest looks good. > > > //Eelco > > > static void > -reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow *flow, uint8_t > type) > +reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow *flow, > + enum ofpact_type type) > { > switch (type) { > case OFPACT_STRIP_VLAN: > @@ -7014,26 +7015,26 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct > flow *flow, uint8_t type) > case OFPACT_DECAP: > case OFPACT_NAT: > ctx->mirrors = 0; > - break; > + return; > case OFPACT_SET_IPV4_SRC: > case OFPACT_SET_IPV4_DST: > if (flow->dl_type == htons(ETH_TYPE_IP)) { > ctx->mirrors = 0; > } > - break; > + return; > case OFPACT_SET_IP_DSCP: > case OFPACT_SET_IP_ECN: > case OFPACT_SET_IP_TTL: > if (is_ip_any(flow)) { > ctx->mirrors = 0; > } > - break; > + 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; > } > - break; > + return; > case OFPACT_OUTPUT_REG: > case OFPACT_OUTPUT_TRUNC: > case OFPACT_GROUP: > @@ -7068,10 +7069,9 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct > flow *flow, uint8_t type) > case OFPACT_DELETE_FIELD: > case OFPACT_NOTE: > case OFPACT_CONJUNCTION: > - break; > - default: > - OVS_NOT_REACHED(); > + return; > } > + OVS_NOT_REACHED(); > } > > > + 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; > > + break; > > + case OFPACT_SET_IPV4_SRC: > > + case OFPACT_SET_IPV4_DST: > > + if (flow->dl_type == htons(ETH_TYPE_IP)) { > > + ctx->mirrors = 0; > > + } > > + break; > > + case OFPACT_SET_IP_DSCP: > > + case OFPACT_SET_IP_ECN: > > + case OFPACT_SET_IP_TTL: > > + if (is_ip_any(flow)) { > > + ctx->mirrors = 0; > > + } > > + break; > > + 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; > > + } > > + break; > > + 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: > > + break; > > + default: > > + 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 +7115,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