| Message ID | 20241004185800.290565-1-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v3] 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 4 Oct 2024, at 20:58, 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 > Signed-off-by: Mike Pattrick <mkp@redhat.com> Thanks for fixing the unit test. Changes look good to me. Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- > v2: > - Added extra whitespace > - Moved N_IDS to never reached section > - Added unit test > v3: > - Fixed unit test, added ovs-vstl to AT_CHECK and changed mirror to select_all > --- > 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..f2339b709 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 > +AT_CHECK([ovs-vsctl set Bridge br0 mirrors=@m -- \ > + --id=@p1 get Port p1 -- --id=@p3 get Port p3 -- \ > + --id=@m create Mirror name=mymirror select_all=true output_port=@p3], > + [0], [ignore]) > + > +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 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 10/4/24 20:58, 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 > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > v2: > - Added extra whitespace > - Moved N_IDS to never reached section > - Added unit test > v3: > - Fixed unit test, added ovs-vstl to AT_CHECK and changed mirror to select_all > --- Hi, Mike. Thanks for v3! However, it seems to actually be v4 as you did send this patch already back in March: https://patchwork.ozlabs.org/project/openvswitch/patch/20240312173716.835716-1-mkp@redhat.com/ I had a bit of a deja vu reviewing this, so I looked back and found it. It also had a different reporter, and not all of my comments from that original submission were addressed. I'll repeat them below + add some new ones for the test. Best regrads, Ilya Maximets. > 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 *); I had a naming comment to change the function to mf_is_any_metadata(), but I'm not sure if that is necessary. > 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; Please, list cases in the order thay are defined in the enum. Most of other fuctions here do that, so it's better to stick to this convention. > + > + 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..f2339b709 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 > +AT_CHECK([ovs-vsctl set Bridge br0 mirrors=@m -- \ > + --id=@p1 get Port p1 -- --id=@p3 get Port p3 -- \ > + --id=@m create Mirror name=mymirror select_all=true output_port=@p3], 2 spaces of extra indentation is probably engouh. Visually aligning with 'set' doesn't make this command easier to read. > + [0], [ignore]) > + > +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. The "packet shouldn't duplicate" doesn't sound right. It shouldn't be duplicated by OVS, but it doesn't duplicate anything on its own. > +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]) Moving the variable declaration out of the command doesn't really make the line shorter, but it makes the packet not show up in the testsuite log. If the idea was to save the line length, it should be constructed with m4_define + m4_join instead. This way we will also see the packet in the log. > +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
On Tue, Oct 15, 2024 at 4:05 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 10/4/24 20:58, 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 > > Signed-off-by: Mike Pattrick <mkp@redhat.com> > > --- > > v2: > > - Added extra whitespace > > - Moved N_IDS to never reached section > > - Added unit test > > v3: > > - Fixed unit test, added ovs-vstl to AT_CHECK and changed mirror to select_all > > --- > > Hi, Mike. Thanks for v3! However, it seems to actually be v4 as you did > send this patch already back in March: > https://patchwork.ozlabs.org/project/openvswitch/patch/20240312173716.835716-1-mkp@redhat.com/ > > I had a bit of a deja vu reviewing this, so I looked back and found it. > It also had a different reporter, and not all of my comments from that > original submission were addressed. I'll repeat them below + add some > new ones for the test. I had totally forgotten about that earlier submission. Sorry about that. Will send an updated revision. > > Best regrads, Ilya Maximets. > > > 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 *); > > I had a naming comment to change the function to mf_is_any_metadata(), > but I'm not sure if that is necessary. > > > 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; > > Please, list cases in the order thay are defined in the enum. > Most of other fuctions here do that, so it's better to stick > to this convention. > > > + > > + 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..f2339b709 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 > > +AT_CHECK([ovs-vsctl set Bridge br0 mirrors=@m -- \ > > + --id=@p1 get Port p1 -- --id=@p3 get Port p3 -- \ > > + --id=@m create Mirror name=mymirror select_all=true output_port=@p3], > > 2 spaces of extra indentation is probably engouh. Visually > aligning with 'set' doesn't make this command easier to read. > > > + [0], [ignore]) > > + > > +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. > > The "packet shouldn't duplicate" doesn't sound right. It shouldn't > be duplicated by OVS, but it doesn't duplicate anything on its own. > > > +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]) > > Moving the variable declaration out of the command doesn't really make > the line shorter, but it makes the packet not show up in the testsuite > log. If the idea was to save the line length, it should be constructed > with m4_define + m4_join instead. This way we will also see the packet > in the log. > > > +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 >
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..f2339b709 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 +AT_CHECK([ovs-vsctl set Bridge br0 mirrors=@m -- \ + --id=@p1 get Port p1 -- --id=@p3 get Port p3 -- \ + --id=@m create Mirror name=mymirror select_all=true output_port=@p3], + [0], [ignore]) + +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 Signed-off-by: Mike Pattrick <mkp@redhat.com> --- v2: - Added extra whitespace - Moved N_IDS to never reached section - Added unit test v3: - Fixed unit test, added ovs-vstl to AT_CHECK and changed mirror to select_all --- 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(-)