| Message ID | 20241001184713.192690-1-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v1] 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 Tue, Oct 01, 2024 at 02:47:13PM -0400, 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> Acked-by: Simon Horman <horms@ovn.org>
On 1 Oct 2024, at 20:47, 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> Small nits, which I think we could apply during the commit. With nits fixed Acked-by: Eelco Chaudron <echaudro@redhat.com> //Eelco > --- > include/openvswitch/meta-flow.h | 1 + > lib/meta-flow.c | 107 ++++++++++++++++++++++++++++++++ > ofproto/ofproto-dpif-xlate.c | 2 +- > 3 files changed, 109 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..c0cc5c164 100644 > --- a/lib/meta-flow.c > +++ b/lib/meta-flow.c > @@ -1790,6 +1790,113 @@ 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; Add a new line, to be inline with mf_is_pipeline_field() > + 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_N_IDS: > + case MFF_PKT_MARK: > + case MFF_RECIRC_ID: > + case MFF_SKB_PRIORITY: > + case MFF_TUN_ERSPAN_DIR: > + return true; Add new line > + default: > + OVS_NOT_REACHED(); This should be 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; > -- > 2.43.5 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 10/1/24 20:47, 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> > --- > include/openvswitch/meta-flow.h | 1 + > lib/meta-flow.c | 107 ++++++++++++++++++++++++++++++++ > ofproto/ofproto-dpif-xlate.c | 2 +- > 3 files changed, 109 insertions(+), 1 deletion(-) Hi, Mike. Thanks for the fix! Could you, please, add a unit test for this issue? We have a few tests that check the datapath flows generated for mirrors, this can be similar. > > 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..c0cc5c164 100644 > --- a/lib/meta-flow.c > +++ b/lib/meta-flow.c > @@ -1790,6 +1790,113 @@ 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_N_IDS: > + case MFF_PKT_MARK: Are we sure about packet mark? While it's not part of the packet per se, but it becomes part of skb in the kernel. Is skb mark being set when we mirror packets? > + case MFF_RECIRC_ID: > + case MFF_SKB_PRIORITY: > + case MFF_TUN_ERSPAN_DIR: > + return true; > + 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;
On Wed, Oct 2, 2024 at 7:40 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 10/1/24 20:47, 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> > > --- > > include/openvswitch/meta-flow.h | 1 + > > lib/meta-flow.c | 107 ++++++++++++++++++++++++++++++++ > > ofproto/ofproto-dpif-xlate.c | 2 +- > > 3 files changed, 109 insertions(+), 1 deletion(-) > > Hi, Mike. Thanks for the fix! > > Could you, please, add a unit test for this issue? > We have a few tests that check the datapath flows generated > for mirrors, this can be similar. Will do! > > > > > 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..c0cc5c164 100644 > > --- a/lib/meta-flow.c > > +++ b/lib/meta-flow.c > > @@ -1790,6 +1790,113 @@ 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_N_IDS: > > + case MFF_PKT_MARK: > > Are we sure about packet mark? While it's not part of the packet per se, > but it becomes part of skb in the kernel. Is skb mark being set when we > mirror packets? Someone could have the following actions: load:0x00->NXM_NX_PKT_MARK[],output:1 and trigger a packet duplication. I'm not sure why anyone would do that, but it seems reasonable to handle that here. I guess something else in the kernel could check the skb_mark and affect the packet some way, but I don't believe mirroring would capture that modification either. -M > > > + case MFF_RECIRC_ID: > > + case MFF_SKB_PRIORITY: > > + case MFF_TUN_ERSPAN_DIR: > > + return true; > > + 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/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..c0cc5c164 100644 --- a/lib/meta-flow.c +++ b/lib/meta-flow.c @@ -1790,6 +1790,113 @@ 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_N_IDS: + case MFF_PKT_MARK: + case MFF_RECIRC_ID: + case MFF_SKB_PRIORITY: + case MFF_TUN_ERSPAN_DIR: + return true; + 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;
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> --- include/openvswitch/meta-flow.h | 1 + lib/meta-flow.c | 107 ++++++++++++++++++++++++++++++++ ofproto/ofproto-dpif-xlate.c | 2 +- 3 files changed, 109 insertions(+), 1 deletion(-)