| Message ID | 20241002160247.223958-1-mkp@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2] ofproto-dpif-upcall: Fix redundant mirror on metadata modification. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 2 Oct 2024, at 18:02, Mike Pattrick wrote: > Previously a commit attempted to reset the mirror context when packets > were modified. However, this commit erroneously also reset the mirror > context when only a packet's metadata was modified. An intermediate > commit corrected this for tunnel metadata, but now that correction is > extended to other forms of metadata as well. > > Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.") > Reported-at: https://issues.redhat.com/browse/FDP-699 > Acked-by: Eelco Chaudron <echaudro@redhat.com> > Acked-by: Simon Horman <horms@ovn.org> > Signed-off-by: Mike Pattrick <mkp@redhat.com> Two comments below on the test case, the rest looks good to me. //Eelco > --- > v2: > - Added extra whitespace > - Moved N_IDS to never reached section > - Added unit test > --- > include/openvswitch/meta-flow.h | 1 + > lib/meta-flow.c | 109 ++++++++++++++++++++++++++++++++ > ofproto/ofproto-dpif-xlate.c | 2 +- > tests/ofproto-dpif.at | 24 +++++++ > 4 files changed, 135 insertions(+), 1 deletion(-) > > diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h > index aff917bcf..65d8b01fe 100644 > --- a/include/openvswitch/meta-flow.h > +++ b/include/openvswitch/meta-flow.h > @@ -2308,6 +2308,7 @@ void mf_set_flow_value_masked(const struct mf_field *, > const union mf_value *mask, > struct flow *); > bool mf_is_tun_metadata(const struct mf_field *); > +bool mf_is_metadata(const struct mf_field *); > bool mf_is_frozen_metadata(const struct mf_field *); > bool mf_is_pipeline_field(const struct mf_field *); > bool mf_is_set(const struct mf_field *, const struct flow *); > diff --git a/lib/meta-flow.c b/lib/meta-flow.c > index 499be04b6..e11fa67e4 100644 > --- a/lib/meta-flow.c > +++ b/lib/meta-flow.c > @@ -1790,6 +1790,115 @@ mf_is_tun_metadata(const struct mf_field *mf) > mf->id < MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS; > } > > +bool > +mf_is_metadata(const struct mf_field *mf) > +{ > + switch (mf->id) { > + case MFF_ARP_OP: > + case MFF_ARP_SHA: > + case MFF_ARP_SPA: > + case MFF_ARP_THA: > + case MFF_ARP_TPA: > + case MFF_DL_VLAN: > + case MFF_DL_VLAN_PCP: > + case MFF_ETH_DST: > + case MFF_ETH_SRC: > + case MFF_ETH_TYPE: > + case MFF_ICMPV4_CODE: > + case MFF_ICMPV4_TYPE: > + case MFF_ICMPV6_CODE: > + case MFF_ICMPV6_TYPE: > + case MFF_IPV4_DST: > + case MFF_IPV4_SRC: > + case MFF_IPV6_DST: > + case MFF_IPV6_LABEL: > + case MFF_IPV6_SRC: > + case MFF_IP_DSCP: > + case MFF_IP_DSCP_SHIFTED: > + case MFF_IP_ECN: > + case MFF_IP_FRAG: > + case MFF_IP_PROTO: > + case MFF_IP_TTL: > + case MFF_MPLS_BOS: > + case MFF_MPLS_LABEL: > + case MFF_MPLS_TC: > + case MFF_MPLS_TTL: > + case MFF_ND_OPTIONS_TYPE: > + case MFF_ND_RESERVED: > + case MFF_ND_SLL: > + case MFF_ND_TARGET: > + case MFF_ND_TLL: > + case MFF_NSH_C1: > + case MFF_NSH_C2: > + case MFF_NSH_C3: > + case MFF_NSH_C4: > + case MFF_NSH_FLAGS: > + case MFF_NSH_MDTYPE: > + case MFF_NSH_NP: > + case MFF_NSH_SI: > + case MFF_NSH_SPI: > + case MFF_NSH_TTL: > + case MFF_PACKET_TYPE: > + case MFF_SCTP_DST: > + case MFF_SCTP_SRC: > + case MFF_TCP_DST: > + case MFF_TCP_FLAGS: > + case MFF_TCP_SRC: > + case MFF_TUN_DST: > + case MFF_TUN_ERSPAN_HWID: > + case MFF_TUN_ERSPAN_IDX: > + case MFF_TUN_ERSPAN_VER: > + case MFF_TUN_FLAGS: > + case MFF_TUN_GBP_FLAGS: > + case MFF_TUN_GBP_ID: > + case MFF_TUN_GTPU_FLAGS: > + case MFF_TUN_GTPU_MSGTYPE: > + case MFF_TUN_ID: > + case MFF_TUN_IPV6_DST: > + case MFF_TUN_IPV6_SRC: > + case MFF_TUN_SRC: > + case MFF_TUN_TOS: > + case MFF_TUN_TTL: > + case MFF_UDP_DST: > + case MFF_UDP_SRC: > + case MFF_VLAN_PCP: > + case MFF_VLAN_TCI: > + case MFF_VLAN_VID: > + return false; > + > + CASE_MFF_REGS: > + CASE_MFF_TUN_METADATA: > + CASE_MFF_XREGS: > + CASE_MFF_XXREGS: > + case MFF_ACTSET_OUTPUT: > + case MFF_CONJ_ID: > + case MFF_CT_IPV6_DST: > + case MFF_CT_IPV6_SRC: > + case MFF_CT_LABEL: > + case MFF_CT_MARK: > + case MFF_CT_NW_DST: > + case MFF_CT_NW_PROTO: > + case MFF_CT_NW_SRC: > + case MFF_CT_STATE: > + case MFF_CT_TP_DST: > + case MFF_CT_TP_SRC: > + case MFF_CT_ZONE: > + case MFF_DP_HASH: > + case MFF_IN_PORT: > + case MFF_IN_PORT_OXM: > + case MFF_METADATA: > + case MFF_PKT_MARK: > + case MFF_RECIRC_ID: > + case MFF_SKB_PRIORITY: > + case MFF_TUN_ERSPAN_DIR: > + return true; > + > + case MFF_N_IDS: > + default: > + OVS_NOT_REACHED(); > + } > +} > + > bool > mf_is_frozen_metadata(const struct mf_field *mf) > { > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 7506ab537..8d668b955 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -7278,7 +7278,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow *flow, > > set_field = ofpact_get_SET_FIELD(a); > mf = set_field->field; > - if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_tun_metadata(mf)) { > + if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_metadata(mf)) { > ctx->mirrors = 0; > } > return; > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 12cb7f7a6..b1a8b8500 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -5514,6 +5514,30 @@ AT_CHECK_UNQUOTED([tail -1 stdout], [0], > OVS_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([ofproto-dpif - mirroring, metadata modification]) This test case also passes without your fix. > +AT_KEYWORDS([mirror mirrors mirroring]) > +OVS_VSWITCHD_START > +add_of_ports br0 1 2 3 > +ovs-vsctl \ > + set Bridge br0 mirrors=@m --\ > + --id=@p1 get Port p1 -- --id=@p3 get Port p3 --\ > + --id=@m create Mirror name=mymirror select_src_port=@p1 output_port=@p3 This command should be run with AT_CHECK(). > + > +AT_DATA([flows.txt], [dnl > +in_port=1 actions=load:0x00->NXM_OF_IN_PORT[[]],output:2 > +]) > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > + > +dnl Metadata modified, packet shouldn't duplicate. > +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,2 > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([ofproto-dpif - mirroring, OFPP_NONE ingress port]) > AT_KEYWORDS([mirror mirrors mirroring]) > OVS_VSWITCHD_START > -- > 2.43.5
On Fri, Oct 4, 2024 at 10:06 AM Eelco Chaudron <echaudro@redhat.com> wrote: > > > > On 2 Oct 2024, at 18:02, Mike Pattrick wrote: > > > Previously a commit attempted to reset the mirror context when packets > > were modified. However, this commit erroneously also reset the mirror > > context when only a packet's metadata was modified. An intermediate > > commit corrected this for tunnel metadata, but now that correction is > > extended to other forms of metadata as well. > > > > Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.") > > Reported-at: https://issues.redhat.com/browse/FDP-699 > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > Acked-by: Simon Horman <horms@ovn.org> > > Signed-off-by: Mike Pattrick <mkp@redhat.com> > > Two comments below on the test case, the rest looks good to me. > > //Eelco > > > > --- > > v2: > > - Added extra whitespace > > - Moved N_IDS to never reached section > > - Added unit test > > --- > > include/openvswitch/meta-flow.h | 1 + > > lib/meta-flow.c | 109 ++++++++++++++++++++++++++++++++ > > ofproto/ofproto-dpif-xlate.c | 2 +- > > tests/ofproto-dpif.at | 24 +++++++ > > 4 files changed, 135 insertions(+), 1 deletion(-) > > > > diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h > > index aff917bcf..65d8b01fe 100644 > > --- a/include/openvswitch/meta-flow.h > > +++ b/include/openvswitch/meta-flow.h > > @@ -2308,6 +2308,7 @@ void mf_set_flow_value_masked(const struct mf_field *, > > const union mf_value *mask, > > struct flow *); > > bool mf_is_tun_metadata(const struct mf_field *); > > +bool mf_is_metadata(const struct mf_field *); > > bool mf_is_frozen_metadata(const struct mf_field *); > > bool mf_is_pipeline_field(const struct mf_field *); > > bool mf_is_set(const struct mf_field *, const struct flow *); > > diff --git a/lib/meta-flow.c b/lib/meta-flow.c > > index 499be04b6..e11fa67e4 100644 > > --- a/lib/meta-flow.c > > +++ b/lib/meta-flow.c > > @@ -1790,6 +1790,115 @@ mf_is_tun_metadata(const struct mf_field *mf) > > mf->id < MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS; > > } > > > > +bool > > +mf_is_metadata(const struct mf_field *mf) > > +{ > > + switch (mf->id) { > > + case MFF_ARP_OP: > > + case MFF_ARP_SHA: > > + case MFF_ARP_SPA: > > + case MFF_ARP_THA: > > + case MFF_ARP_TPA: > > + case MFF_DL_VLAN: > > + case MFF_DL_VLAN_PCP: > > + case MFF_ETH_DST: > > + case MFF_ETH_SRC: > > + case MFF_ETH_TYPE: > > + case MFF_ICMPV4_CODE: > > + case MFF_ICMPV4_TYPE: > > + case MFF_ICMPV6_CODE: > > + case MFF_ICMPV6_TYPE: > > + case MFF_IPV4_DST: > > + case MFF_IPV4_SRC: > > + case MFF_IPV6_DST: > > + case MFF_IPV6_LABEL: > > + case MFF_IPV6_SRC: > > + case MFF_IP_DSCP: > > + case MFF_IP_DSCP_SHIFTED: > > + case MFF_IP_ECN: > > + case MFF_IP_FRAG: > > + case MFF_IP_PROTO: > > + case MFF_IP_TTL: > > + case MFF_MPLS_BOS: > > + case MFF_MPLS_LABEL: > > + case MFF_MPLS_TC: > > + case MFF_MPLS_TTL: > > + case MFF_ND_OPTIONS_TYPE: > > + case MFF_ND_RESERVED: > > + case MFF_ND_SLL: > > + case MFF_ND_TARGET: > > + case MFF_ND_TLL: > > + case MFF_NSH_C1: > > + case MFF_NSH_C2: > > + case MFF_NSH_C3: > > + case MFF_NSH_C4: > > + case MFF_NSH_FLAGS: > > + case MFF_NSH_MDTYPE: > > + case MFF_NSH_NP: > > + case MFF_NSH_SI: > > + case MFF_NSH_SPI: > > + case MFF_NSH_TTL: > > + case MFF_PACKET_TYPE: > > + case MFF_SCTP_DST: > > + case MFF_SCTP_SRC: > > + case MFF_TCP_DST: > > + case MFF_TCP_FLAGS: > > + case MFF_TCP_SRC: > > + case MFF_TUN_DST: > > + case MFF_TUN_ERSPAN_HWID: > > + case MFF_TUN_ERSPAN_IDX: > > + case MFF_TUN_ERSPAN_VER: > > + case MFF_TUN_FLAGS: > > + case MFF_TUN_GBP_FLAGS: > > + case MFF_TUN_GBP_ID: > > + case MFF_TUN_GTPU_FLAGS: > > + case MFF_TUN_GTPU_MSGTYPE: > > + case MFF_TUN_ID: > > + case MFF_TUN_IPV6_DST: > > + case MFF_TUN_IPV6_SRC: > > + case MFF_TUN_SRC: > > + case MFF_TUN_TOS: > > + case MFF_TUN_TTL: > > + case MFF_UDP_DST: > > + case MFF_UDP_SRC: > > + case MFF_VLAN_PCP: > > + case MFF_VLAN_TCI: > > + case MFF_VLAN_VID: > > + return false; > > + > > + CASE_MFF_REGS: > > + CASE_MFF_TUN_METADATA: > > + CASE_MFF_XREGS: > > + CASE_MFF_XXREGS: > > + case MFF_ACTSET_OUTPUT: > > + case MFF_CONJ_ID: > > + case MFF_CT_IPV6_DST: > > + case MFF_CT_IPV6_SRC: > > + case MFF_CT_LABEL: > > + case MFF_CT_MARK: > > + case MFF_CT_NW_DST: > > + case MFF_CT_NW_PROTO: > > + case MFF_CT_NW_SRC: > > + case MFF_CT_STATE: > > + case MFF_CT_TP_DST: > > + case MFF_CT_TP_SRC: > > + case MFF_CT_ZONE: > > + case MFF_DP_HASH: > > + case MFF_IN_PORT: > > + case MFF_IN_PORT_OXM: > > + case MFF_METADATA: > > + case MFF_PKT_MARK: > > + case MFF_RECIRC_ID: > > + case MFF_SKB_PRIORITY: > > + case MFF_TUN_ERSPAN_DIR: > > + return true; > > + > > + case MFF_N_IDS: > > + default: > > + OVS_NOT_REACHED(); > > + } > > +} > > + > > bool > > mf_is_frozen_metadata(const struct mf_field *mf) > > { > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > index 7506ab537..8d668b955 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -7278,7 +7278,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow *flow, > > > > set_field = ofpact_get_SET_FIELD(a); > > mf = set_field->field; > > - if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_tun_metadata(mf)) { > > + if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_metadata(mf)) { > > ctx->mirrors = 0; > > } > > return; > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > > index 12cb7f7a6..b1a8b8500 100644 > > --- a/tests/ofproto-dpif.at > > +++ b/tests/ofproto-dpif.at > > @@ -5514,6 +5514,30 @@ AT_CHECK_UNQUOTED([tail -1 stdout], [0], > > OVS_VSWITCHD_STOP > > AT_CLEANUP > > > > +AT_SETUP([ofproto-dpif - mirroring, metadata modification]) > > This test case also passes without your fix. > > > +AT_KEYWORDS([mirror mirrors mirroring]) > > +OVS_VSWITCHD_START > > +add_of_ports br0 1 2 3 > > +ovs-vsctl \ > > + set Bridge br0 mirrors=@m --\ > > + --id=@p1 get Port p1 -- --id=@p3 get Port p3 --\ > > + --id=@m create Mirror name=mymirror select_src_port=@p1 output_port=@p3 > > This command should be run with AT_CHECK(). Both issues are addressed in the new version, thank you. -M > > > + > > +AT_DATA([flows.txt], [dnl > > +in_port=1 actions=load:0x00->NXM_OF_IN_PORT[[]],output:2 > > +]) > > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > > + > > +dnl Metadata modified, packet shouldn't duplicate. > > +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,2 > > +]) > > + > > +OVS_VSWITCHD_STOP > > +AT_CLEANUP > > + > > AT_SETUP([ofproto-dpif - mirroring, OFPP_NONE ingress port]) > > AT_KEYWORDS([mirror mirrors mirroring]) > > OVS_VSWITCHD_START > > -- > > 2.43.5 >
diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h index aff917bcf..65d8b01fe 100644 --- a/include/openvswitch/meta-flow.h +++ b/include/openvswitch/meta-flow.h @@ -2308,6 +2308,7 @@ void mf_set_flow_value_masked(const struct mf_field *, const union mf_value *mask, struct flow *); bool mf_is_tun_metadata(const struct mf_field *); +bool mf_is_metadata(const struct mf_field *); bool mf_is_frozen_metadata(const struct mf_field *); bool mf_is_pipeline_field(const struct mf_field *); bool mf_is_set(const struct mf_field *, const struct flow *); diff --git a/lib/meta-flow.c b/lib/meta-flow.c index 499be04b6..e11fa67e4 100644 --- a/lib/meta-flow.c +++ b/lib/meta-flow.c @@ -1790,6 +1790,115 @@ mf_is_tun_metadata(const struct mf_field *mf) mf->id < MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS; } +bool +mf_is_metadata(const struct mf_field *mf) +{ + switch (mf->id) { + case MFF_ARP_OP: + case MFF_ARP_SHA: + case MFF_ARP_SPA: + case MFF_ARP_THA: + case MFF_ARP_TPA: + case MFF_DL_VLAN: + case MFF_DL_VLAN_PCP: + case MFF_ETH_DST: + case MFF_ETH_SRC: + case MFF_ETH_TYPE: + case MFF_ICMPV4_CODE: + case MFF_ICMPV4_TYPE: + case MFF_ICMPV6_CODE: + case MFF_ICMPV6_TYPE: + case MFF_IPV4_DST: + case MFF_IPV4_SRC: + case MFF_IPV6_DST: + case MFF_IPV6_LABEL: + case MFF_IPV6_SRC: + case MFF_IP_DSCP: + case MFF_IP_DSCP_SHIFTED: + case MFF_IP_ECN: + case MFF_IP_FRAG: + case MFF_IP_PROTO: + case MFF_IP_TTL: + case MFF_MPLS_BOS: + case MFF_MPLS_LABEL: + case MFF_MPLS_TC: + case MFF_MPLS_TTL: + case MFF_ND_OPTIONS_TYPE: + case MFF_ND_RESERVED: + case MFF_ND_SLL: + case MFF_ND_TARGET: + case MFF_ND_TLL: + case MFF_NSH_C1: + case MFF_NSH_C2: + case MFF_NSH_C3: + case MFF_NSH_C4: + case MFF_NSH_FLAGS: + case MFF_NSH_MDTYPE: + case MFF_NSH_NP: + case MFF_NSH_SI: + case MFF_NSH_SPI: + case MFF_NSH_TTL: + case MFF_PACKET_TYPE: + case MFF_SCTP_DST: + case MFF_SCTP_SRC: + case MFF_TCP_DST: + case MFF_TCP_FLAGS: + case MFF_TCP_SRC: + case MFF_TUN_DST: + case MFF_TUN_ERSPAN_HWID: + case MFF_TUN_ERSPAN_IDX: + case MFF_TUN_ERSPAN_VER: + case MFF_TUN_FLAGS: + case MFF_TUN_GBP_FLAGS: + case MFF_TUN_GBP_ID: + case MFF_TUN_GTPU_FLAGS: + case MFF_TUN_GTPU_MSGTYPE: + case MFF_TUN_ID: + case MFF_TUN_IPV6_DST: + case MFF_TUN_IPV6_SRC: + case MFF_TUN_SRC: + case MFF_TUN_TOS: + case MFF_TUN_TTL: + case MFF_UDP_DST: + case MFF_UDP_SRC: + case MFF_VLAN_PCP: + case MFF_VLAN_TCI: + case MFF_VLAN_VID: + return false; + + CASE_MFF_REGS: + CASE_MFF_TUN_METADATA: + CASE_MFF_XREGS: + CASE_MFF_XXREGS: + case MFF_ACTSET_OUTPUT: + case MFF_CONJ_ID: + case MFF_CT_IPV6_DST: + case MFF_CT_IPV6_SRC: + case MFF_CT_LABEL: + case MFF_CT_MARK: + case MFF_CT_NW_DST: + case MFF_CT_NW_PROTO: + case MFF_CT_NW_SRC: + case MFF_CT_STATE: + case MFF_CT_TP_DST: + case MFF_CT_TP_SRC: + case MFF_CT_ZONE: + case MFF_DP_HASH: + case MFF_IN_PORT: + case MFF_IN_PORT_OXM: + case MFF_METADATA: + case MFF_PKT_MARK: + case MFF_RECIRC_ID: + case MFF_SKB_PRIORITY: + case MFF_TUN_ERSPAN_DIR: + return true; + + case MFF_N_IDS: + default: + OVS_NOT_REACHED(); + } +} + bool mf_is_frozen_metadata(const struct mf_field *mf) { diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 7506ab537..8d668b955 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -7278,7 +7278,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct flow *flow, set_field = ofpact_get_SET_FIELD(a); mf = set_field->field; - if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_tun_metadata(mf)) { + if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_metadata(mf)) { ctx->mirrors = 0; } return; diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 12cb7f7a6..b1a8b8500 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -5514,6 +5514,30 @@ AT_CHECK_UNQUOTED([tail -1 stdout], [0], OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif - mirroring, metadata modification]) +AT_KEYWORDS([mirror mirrors mirroring]) +OVS_VSWITCHD_START +add_of_ports br0 1 2 3 +ovs-vsctl \ + set Bridge br0 mirrors=@m --\ + --id=@p1 get Port p1 -- --id=@p3 get Port p3 --\ + --id=@m create Mirror name=mymirror select_src_port=@p1 output_port=@p3 + +AT_DATA([flows.txt], [dnl +in_port=1 actions=load:0x00->NXM_OF_IN_PORT[[]],output:2 +]) +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) + +dnl Metadata modified, packet shouldn't duplicate. +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,2 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ofproto-dpif - mirroring, OFPP_NONE ingress port]) AT_KEYWORDS([mirror mirrors mirroring]) OVS_VSWITCHD_START
Previously a commit attempted to reset the mirror context when packets were modified. However, this commit erroneously also reset the mirror context when only a packet's metadata was modified. An intermediate commit corrected this for tunnel metadata, but now that correction is extended to other forms of metadata as well. Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.") Reported-at: https://issues.redhat.com/browse/FDP-699 Acked-by: Eelco Chaudron <echaudro@redhat.com> Acked-by: Simon Horman <horms@ovn.org> Signed-off-by: Mike Pattrick <mkp@redhat.com> --- v2: - Added extra whitespace - Moved N_IDS to never reached section - Added unit test --- include/openvswitch/meta-flow.h | 1 + lib/meta-flow.c | 109 ++++++++++++++++++++++++++++++++ ofproto/ofproto-dpif-xlate.c | 2 +- tests/ofproto-dpif.at | 24 +++++++ 4 files changed, 135 insertions(+), 1 deletion(-)