| Message ID | 20230216200508.784202-1-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif-upcall: Mirror packets that are modified | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
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 <mkp@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 <mkp@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Thu, Feb 16, 2023 at 03:05:08PM -0500, 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. nit: I think 'received' at the end of the paragraph is a bit confusing. Received by who? (Yes, I know who :) > 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 <mkp@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; The pattern of adding some housekeeping to many actions to support a specific case reminds me of MPLS. My recollection is that although I used such an approach when adding MPLS support it was subsequently factored out into (what is now) recirc_for_mpls(). As I recall the main concern was about maintainability. And I think that concern applies here. F.e. if a new action that modifies packets is added, will ctx->mirrors = 0 get forgotten? So I wonder if we can try to handle this a different way. One idea I had - but have not thought through very thoroughly - is to make use of the fact that xlate_commit_actions() is called to append ODP actions corresponding to any packet modifications. Such an approach may also ensure that the logic only executes when packets are really modified - though there may be duplication if there is a modify-output-restore-output sequence. ... > 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 > ]) Test results look nice :) ...
On Tue, Feb 21, 2023 at 5:35 AM Simon Horman <simon.horman@corigine.com> wrote: > > On Thu, Feb 16, 2023 at 03:05:08PM -0500, 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. > > nit: I think 'received' at the end of the paragraph is a bit confusing. > Received by who? (Yes, I know who :) > > > 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 <mkp@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; > > The pattern of adding some housekeeping to many actions to support a > specific case reminds me of MPLS. My recollection is that although > I used such an approach when adding MPLS support it was subsequently > factored out into (what is now) recirc_for_mpls(). > > As I recall the main concern was about maintainability. And I think that > concern applies here. F.e. if a new action that modifies packets > is added, will ctx->mirrors = 0 get forgotten? That's a good point. I can wrap this in a switch statement, and will investigate if xlate_commit_actions is an appropriate location. Thanks M > > So I wonder if we can try to handle this a different way. > One idea I had - but have not thought through very thoroughly - > is to make use of the fact that xlate_commit_actions() is called > to append ODP actions corresponding to any packet modifications. > > Such an approach may also ensure that the logic only executes > when packets are really modified - though there may be duplication if > there is a modify-output-restore-output sequence. > > ... > > > 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 > > ]) > > Test results look nice :) > > ... >
On Tue, Feb 21, 2023 at 10:11:27AM -0500, Mike Pattrick wrote: > On Tue, Feb 21, 2023 at 5:35 AM Simon Horman <simon.horman@corigine.com> wrote: > > > > On Thu, Feb 16, 2023 at 03:05:08PM -0500, 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. > > > > nit: I think 'received' at the end of the paragraph is a bit confusing. > > Received by who? (Yes, I know who :) > > > > > 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 <mkp@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; > > > > The pattern of adding some housekeeping to many actions to support a > > specific case reminds me of MPLS. My recollection is that although > > I used such an approach when adding MPLS support it was subsequently > > factored out into (what is now) recirc_for_mpls(). > > > > As I recall the main concern was about maintainability. And I think that > > concern applies here. F.e. if a new action that modifies packets > > is added, will ctx->mirrors = 0 get forgotten? > > That's a good point. I can wrap this in a switch statement, and will > investigate if xlate_commit_actions is an appropriate location. Thanks. Just to clarify: my suggestion regarding xlate_commit_actions() was not just that it might be a good location. But also that you may be able to get it, or something it calls, to tell you if any packet modifications were made. > > > Thanks > M > > > > > So I wonder if we can try to handle this a different way. > > One idea I had - but have not thought through very thoroughly - > > is to make use of the fact that xlate_commit_actions() is called > > to append ODP actions corresponding to any packet modifications. > > > > Such an approach may also ensure that the logic only executes > > when packets are really modified - though there may be duplication if > > there is a modify-output-restore-output sequence. > > > > ... > > > > > 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 > > > ]) > > > > Test results look nice :) > > > > ... > > >
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)"
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 <mkp@redhat.com> --- ofproto/ofproto-dpif-xlate.c | 37 +++++++++++++++++++++++++++++++----- tests/ofproto-dpif.at | 6 +++--- 2 files changed, 35 insertions(+), 8 deletions(-)