Hi Mike, Thanks for the patch!. I took your patch and tried running the OVN unit test for the 'both' direction packets mirroring check using the ovsmirrortest.diff attached in the BZ (2155579 <https://bugzilla.redhat.com/show_bug.cgi?id=2155579>) It passed with your patch.
Thanks & Regards, Abhiram R N On Fri, Feb 17, 2023 at 1:37 AM Mike Pattrick <m...@redhat.com> 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> > --- > ofproto/ofproto-dpif-xlate.c | 37 +++++++++++++++++++++++++++++++----- > tests/ofproto-dpif.at | 6 +++--- > 2 files changed, 35 insertions(+), 8 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index a9cf3cbee..34b4b4018 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -7078,6 +7078,7 @@ do_xlate_actions(const struct ofpact *ofpacts, > size_t ofpacts_len, > break; > > case OFPACT_SET_VLAN_VID: > + ctx->mirrors = 0; > wc->masks.vlans[0].tci |= htons(VLAN_VID_MASK | VLAN_CFI); > if (flow->vlans[0].tci & htons(VLAN_CFI) || > ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed) { > @@ -7092,6 +7093,7 @@ do_xlate_actions(const struct ofpact *ofpacts, > size_t ofpacts_len, > break; > > case OFPACT_SET_VLAN_PCP: > + ctx->mirrors = 0; > wc->masks.vlans[0].tci |= htons(VLAN_PCP_MASK | VLAN_CFI); > if (flow->vlans[0].tci & htons(VLAN_CFI) || > ofpact_get_SET_VLAN_PCP(a)->push_vlan_if_needed) { > @@ -7106,27 +7108,32 @@ do_xlate_actions(const struct ofpact *ofpacts, > size_t ofpacts_len, > break; > > case OFPACT_STRIP_VLAN: > + ctx->mirrors = 0; > flow_pop_vlan(flow, wc); > break; > > case OFPACT_PUSH_VLAN: > + ctx->mirrors = 0; > flow_push_vlan_uninit(flow, wc); > flow->vlans[0].tpid = ofpact_get_PUSH_VLAN(a)->ethertype; > flow->vlans[0].tci = htons(VLAN_CFI); > break; > > case OFPACT_SET_ETH_SRC: > + ctx->mirrors = 0; > WC_MASK_FIELD(wc, dl_src); > flow->dl_src = ofpact_get_SET_ETH_SRC(a)->mac; > break; > > case OFPACT_SET_ETH_DST: > + ctx->mirrors = 0; > WC_MASK_FIELD(wc, dl_dst); > flow->dl_dst = ofpact_get_SET_ETH_DST(a)->mac; > break; > > case OFPACT_SET_IPV4_SRC: > if (flow->dl_type == htons(ETH_TYPE_IP)) { > + ctx->mirrors = 0; > memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src); > flow->nw_src = ofpact_get_SET_IPV4_SRC(a)->ipv4; > } > @@ -7134,6 +7141,7 @@ do_xlate_actions(const struct ofpact *ofpacts, > size_t ofpacts_len, > > case OFPACT_SET_IPV4_DST: > if (flow->dl_type == htons(ETH_TYPE_IP)) { > + ctx->mirrors = 0; > memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst); > flow->nw_dst = ofpact_get_SET_IPV4_DST(a)->ipv4; > } > @@ -7141,6 +7149,7 @@ do_xlate_actions(const struct ofpact *ofpacts, > size_t ofpacts_len, > > case OFPACT_SET_IP_DSCP: > if (is_ip_any(flow)) { > + ctx->mirrors = 0; > wc->masks.nw_tos |= IP_DSCP_MASK; > flow->nw_tos &= ~IP_DSCP_MASK; > flow->nw_tos |= ofpact_get_SET_IP_DSCP(a)->dscp; > @@ -7149,6 +7158,7 @@ do_xlate_actions(const struct ofpact *ofpacts, > size_t ofpacts_len, > > case OFPACT_SET_IP_ECN: > if (is_ip_any(flow)) { > + ctx->mirrors = 0; > wc->masks.nw_tos |= IP_ECN_MASK; > flow->nw_tos &= ~IP_ECN_MASK; > flow->nw_tos |= ofpact_get_SET_IP_ECN(a)->ecn; > @@ -7157,6 +7167,7 @@ do_xlate_actions(const struct ofpact *ofpacts, > size_t ofpacts_len, > > case OFPACT_SET_IP_TTL: > if (is_ip_any(flow)) { > + ctx->mirrors = 0; > wc->masks.nw_ttl = 0xff; > flow->nw_ttl = ofpact_get_SET_IP_TTL(a)->ttl; > } > @@ -7164,6 +7175,7 @@ do_xlate_actions(const struct ofpact *ofpacts, > size_t ofpacts_len, > > case OFPACT_SET_L4_SRC_PORT: > if (is_ip_any(flow) && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) > { > + ctx->mirrors = 0; > memset(&wc->masks.nw_proto, 0xff, sizeof > wc->masks.nw_proto); > memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); > flow->tp_src = htons(ofpact_get_SET_L4_SRC_PORT(a)->port); > @@ -7172,6 +7184,7 @@ do_xlate_actions(const struct ofpact *ofpacts, > size_t ofpacts_len, > > case OFPACT_SET_L4_DST_PORT: > if (is_ip_any(flow) && !(flow->nw_frag & FLOW_NW_FRAG_LATER)) > { > + ctx->mirrors = 0; > memset(&wc->masks.nw_proto, 0xff, sizeof > wc->masks.nw_proto); > memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst); > flow->tp_dst = htons(ofpact_get_SET_L4_DST_PORT(a)->port); > @@ -7224,6 +7237,7 @@ do_xlate_actions(const struct ofpact *ofpacts, > size_t ofpacts_len, > > /* Set the field only if the packet actually has it. */ > if (mf_are_prereqs_ok(mf, flow, wc)) { > + ctx->mirrors = 0; > mf_mask_field_masked(mf, > ofpact_set_field_mask(set_field), wc); > mf_set_flow_value_masked(mf, set_field->value, > ofpact_set_field_mask(set_field), > @@ -7246,39 +7260,47 @@ do_xlate_actions(const struct ofpact *ofpacts, > size_t ofpacts_len, > break; > > case OFPACT_PUSH_MPLS: > + ctx->mirrors = 0; > compose_mpls_push_action(ctx, ofpact_get_PUSH_MPLS(a)); > break; > > case OFPACT_POP_MPLS: > + ctx->mirrors = 0; > compose_mpls_pop_action(ctx, > ofpact_get_POP_MPLS(a)->ethertype); > break; > > case OFPACT_SET_MPLS_LABEL: > + ctx->mirrors = 0; > compose_set_mpls_label_action( > ctx, ofpact_get_SET_MPLS_LABEL(a)->label); > break; > > case OFPACT_SET_MPLS_TC: > + ctx->mirrors = 0; > compose_set_mpls_tc_action(ctx, > ofpact_get_SET_MPLS_TC(a)->tc); > break; > > case OFPACT_SET_MPLS_TTL: > + ctx->mirrors = 0; > compose_set_mpls_ttl_action(ctx, > ofpact_get_SET_MPLS_TTL(a)->ttl); > break; > > case OFPACT_DEC_MPLS_TTL: > + ctx->mirrors = 0; > if (compose_dec_mpls_ttl_action(ctx)) { > return; > } > break; > > case OFPACT_DEC_NSH_TTL: > + ctx->mirrors = 0; > if (compose_dec_nsh_ttl_action(ctx)) { > return; > } > break; > > case OFPACT_DEC_TTL: > + ctx->mirrors = 0; > wc->masks.nw_ttl = 0xff; > if (compose_dec_ttl(ctx, ofpact_get_DEC_TTL(a))) { > return; > @@ -7370,17 +7392,21 @@ do_xlate_actions(const struct ofpact *ofpacts, > size_t ofpacts_len, > break; > > case OFPACT_ENCAP: > + ctx->mirrors = 0; > xlate_generic_encap_action(ctx, ofpact_get_ENCAP(a)); > break; > > case OFPACT_DECAP: { > bool recirc_needed = > xlate_generic_decap_action(ctx, ofpact_get_DECAP(a)); > - if (!ctx->error && recirc_needed) { > - /* Recirculate for parsing of inner packet. */ > - ctx_trigger_freeze(ctx); > - /* Then continue with next action. */ > - a = ofpact_next(a); > + if (!ctx->error) { > + ctx->mirrors = 0; > + if (recirc_needed) { > + /* Recirculate for parsing of inner packet. */ > + ctx_trigger_freeze(ctx); > + /* Then continue with next action. */ > + a = ofpact_next(a); > + } > } > break; > } > @@ -7397,6 +7423,7 @@ do_xlate_actions(const struct ofpact *ofpacts, > size_t ofpacts_len, > > case OFPACT_NAT: > /* This will be processed by compose_conntrack_action(). */ > + ctx->mirrors = 0; > ctx->ct_nat_action = ofpact_get_NAT(a); > break; > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 222415ac0..b027e8c2c 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 > -- Always put yourself in others' shoes. If you feel that it hurts you, it probably hurts the other person, too. Regards ABHIRAM _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
