| Message ID | 20240312173716.835716-1-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif-upcall: Don't mirror packets that aren't 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 |
On 3/12/24 18:37, Mike Pattrick wrote: > Previously OVS reset the mirror contents when a packet is modified in > such a way that the packets contents changes. However, this change > incorrectly reset that mirror context when only metadata changes as > well. > > Now we check for all metadata fields, instead of just tunnel metadata, > before resetting the mirror context. > > Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.") > Reported-by: Zhangweiwei <zhang.weiwei@h3c.com> > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > include/openvswitch/meta-flow.h | 1 + > lib/meta-flow.c | 109 ++++++++++++++++++++++++++++++++ > ofproto/ofproto-dpif-xlate.c | 2 +- > tests/ofproto-dpif.at | 5 +- > 4 files changed, 114 insertions(+), 3 deletions(-) Thanks, Mike! The change makes sense to me. See some comments inline. Best regards, Ilya Maixmets. > > diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h > index 3b0220aaa..96aad3933 100644 > --- a/include/openvswitch/meta-flow.h > +++ b/include/openvswitch/meta-flow.h > @@ -2305,6 +2305,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 *); Nit: maybe call it mf_is_any_metadata ? > 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 aa7cf1fcb..7ecec334e 100644 > --- a/lib/meta-flow.c > +++ b/lib/meta-flow.c > @@ -1788,6 +1788,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_TUN_METADATA: > + case MFF_METADATA: > + case MFF_IN_PORT: > + case MFF_IN_PORT_OXM: > + CASE_MFF_REGS: > + CASE_MFF_XREGS: > + CASE_MFF_XXREGS: > + case MFF_PACKET_TYPE: > + case MFF_DP_HASH: > + case MFF_RECIRC_ID: > + case MFF_CONJ_ID: > + case MFF_ACTSET_OUTPUT: > + case MFF_SKB_PRIORITY: > + case MFF_PKT_MARK: > + case MFF_CT_STATE: > + case MFF_CT_ZONE: > + case MFF_CT_MARK: > + case MFF_CT_LABEL: > + case MFF_CT_NW_PROTO: > + case MFF_CT_NW_SRC: > + case MFF_CT_NW_DST: > + case MFF_CT_IPV6_SRC: > + case MFF_CT_IPV6_DST: > + case MFF_CT_TP_SRC: > + case MFF_CT_TP_DST: > + case MFF_N_IDS: The N_IDS should not be here. It probably goes to the 'default' case. > + return true; > + > + case MFF_TUN_ID: > + case MFF_TUN_SRC: > + case MFF_TUN_DST: > + case MFF_TUN_IPV6_SRC: > + case MFF_TUN_IPV6_DST: > + case MFF_TUN_FLAGS: > + case MFF_TUN_GBP_ID: > + case MFF_TUN_GBP_FLAGS: > + case MFF_TUN_ERSPAN_VER: > + case MFF_TUN_ERSPAN_IDX: > + case MFF_TUN_ERSPAN_DIR: > + case MFF_TUN_ERSPAN_HWID: > + case MFF_TUN_GTPU_FLAGS: > + case MFF_TUN_GTPU_MSGTYPE: > + case MFF_TUN_TTL: > + case MFF_TUN_TOS: > + case MFF_ETH_SRC: > + case MFF_ETH_DST: > + case MFF_ETH_TYPE: > + case MFF_VLAN_TCI: > + case MFF_DL_VLAN: > + case MFF_VLAN_VID: > + case MFF_DL_VLAN_PCP: > + case MFF_VLAN_PCP: > + case MFF_MPLS_LABEL: > + case MFF_MPLS_TC: > + case MFF_MPLS_BOS: > + case MFF_MPLS_TTL: > + case MFF_IPV4_SRC: > + case MFF_IPV4_DST: > + case MFF_IPV6_SRC: > + case MFF_IPV6_DST: > + case MFF_IPV6_LABEL: > + case MFF_IP_PROTO: > + case MFF_IP_DSCP: > + case MFF_IP_DSCP_SHIFTED: > + case MFF_IP_ECN: > + case MFF_IP_TTL: > + case MFF_IP_FRAG: > + case MFF_ARP_OP: > + case MFF_ARP_SPA: > + case MFF_ARP_TPA: > + case MFF_ARP_SHA: > + case MFF_ARP_THA: > + case MFF_TCP_SRC: > + case MFF_TCP_DST: > + case MFF_TCP_FLAGS: > + case MFF_UDP_SRC: > + case MFF_UDP_DST: > + case MFF_SCTP_SRC: > + case MFF_SCTP_DST: > + case MFF_ICMPV4_TYPE: > + case MFF_ICMPV4_CODE: > + case MFF_ICMPV6_TYPE: > + case MFF_ICMPV6_CODE: > + case MFF_ND_TARGET: > + case MFF_ND_SLL: > + case MFF_ND_TLL: > + case MFF_ND_RESERVED: > + case MFF_ND_OPTIONS_TYPE: > + case MFF_NSH_FLAGS: > + case MFF_NSH_TTL: > + case MFF_NSH_MDTYPE: > + case MFF_NSH_NP: > + case MFF_NSH_SPI: > + case MFF_NSH_SI: > + case MFF_NSH_C1: > + case MFF_NSH_C2: > + case MFF_NSH_C3: > + case MFF_NSH_C4: > + return false; In general, functions like this are trying to maintain a specific order of the cases. It's either alphabetical or the order in which they are defined in the enum. I think, most of the mf functions are trying to follwo the enum order. Order in this fucntion doesn't seem to match any other function. Could you re-order the cases in the enum order, please? > + > + 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 89f183182..faa364ec8 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -7141,7 +7141,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 a1393f7f8..245e209c3 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -5443,7 +5443,8 @@ AT_CLEANUP > # This test verifies that mirror state is preserved across recirculation. > # > # Otherwise, post-recirculation the ingress and the output to port 4 > -# would cause the packet to be mirrored to port 3 a second time. > +# would cause the packet to be mirrored to port 3 a second time. A register > +# is also modified to verify that this doesn't reset the mirror context. I don't think we should mix the tests like that. Original test is checking a particular functionlity related to recirculation. We should have a separate test for metadata changes. > AT_SETUP([ofproto-dpif - mirroring with recirculation]) > AT_KEYWORDS([mirror mirrors mirroring]) > OVS_VSWITCHD_START > @@ -5454,7 +5455,7 @@ ovs-vsctl \ > --id=@m create Mirror name=mymirror select_all=true output_port=@p3 > > AT_DATA([flows.txt], [dnl > -in_port=1 actions=2,debug_recirc,4 > +in_port=1 actions=2,debug_recirc,set_field:1->reg4,4 > ]) > AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >
diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h index 3b0220aaa..96aad3933 100644 --- a/include/openvswitch/meta-flow.h +++ b/include/openvswitch/meta-flow.h @@ -2305,6 +2305,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 aa7cf1fcb..7ecec334e 100644 --- a/lib/meta-flow.c +++ b/lib/meta-flow.c @@ -1788,6 +1788,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_TUN_METADATA: + case MFF_METADATA: + case MFF_IN_PORT: + case MFF_IN_PORT_OXM: + CASE_MFF_REGS: + CASE_MFF_XREGS: + CASE_MFF_XXREGS: + case MFF_PACKET_TYPE: + case MFF_DP_HASH: + case MFF_RECIRC_ID: + case MFF_CONJ_ID: + case MFF_ACTSET_OUTPUT: + case MFF_SKB_PRIORITY: + case MFF_PKT_MARK: + case MFF_CT_STATE: + case MFF_CT_ZONE: + case MFF_CT_MARK: + case MFF_CT_LABEL: + case MFF_CT_NW_PROTO: + case MFF_CT_NW_SRC: + case MFF_CT_NW_DST: + case MFF_CT_IPV6_SRC: + case MFF_CT_IPV6_DST: + case MFF_CT_TP_SRC: + case MFF_CT_TP_DST: + case MFF_N_IDS: + return true; + + case MFF_TUN_ID: + case MFF_TUN_SRC: + case MFF_TUN_DST: + case MFF_TUN_IPV6_SRC: + case MFF_TUN_IPV6_DST: + case MFF_TUN_FLAGS: + case MFF_TUN_GBP_ID: + case MFF_TUN_GBP_FLAGS: + case MFF_TUN_ERSPAN_VER: + case MFF_TUN_ERSPAN_IDX: + case MFF_TUN_ERSPAN_DIR: + case MFF_TUN_ERSPAN_HWID: + case MFF_TUN_GTPU_FLAGS: + case MFF_TUN_GTPU_MSGTYPE: + case MFF_TUN_TTL: + case MFF_TUN_TOS: + case MFF_ETH_SRC: + case MFF_ETH_DST: + case MFF_ETH_TYPE: + case MFF_VLAN_TCI: + case MFF_DL_VLAN: + case MFF_VLAN_VID: + case MFF_DL_VLAN_PCP: + case MFF_VLAN_PCP: + case MFF_MPLS_LABEL: + case MFF_MPLS_TC: + case MFF_MPLS_BOS: + case MFF_MPLS_TTL: + case MFF_IPV4_SRC: + case MFF_IPV4_DST: + case MFF_IPV6_SRC: + case MFF_IPV6_DST: + case MFF_IPV6_LABEL: + case MFF_IP_PROTO: + case MFF_IP_DSCP: + case MFF_IP_DSCP_SHIFTED: + case MFF_IP_ECN: + case MFF_IP_TTL: + case MFF_IP_FRAG: + case MFF_ARP_OP: + case MFF_ARP_SPA: + case MFF_ARP_TPA: + case MFF_ARP_SHA: + case MFF_ARP_THA: + case MFF_TCP_SRC: + case MFF_TCP_DST: + case MFF_TCP_FLAGS: + case MFF_UDP_SRC: + case MFF_UDP_DST: + case MFF_SCTP_SRC: + case MFF_SCTP_DST: + case MFF_ICMPV4_TYPE: + case MFF_ICMPV4_CODE: + case MFF_ICMPV6_TYPE: + case MFF_ICMPV6_CODE: + case MFF_ND_TARGET: + case MFF_ND_SLL: + case MFF_ND_TLL: + case MFF_ND_RESERVED: + case MFF_ND_OPTIONS_TYPE: + case MFF_NSH_FLAGS: + case MFF_NSH_TTL: + case MFF_NSH_MDTYPE: + case MFF_NSH_NP: + case MFF_NSH_SPI: + case MFF_NSH_SI: + case MFF_NSH_C1: + case MFF_NSH_C2: + case MFF_NSH_C3: + case MFF_NSH_C4: + return false; + + 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 89f183182..faa364ec8 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -7141,7 +7141,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 a1393f7f8..245e209c3 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -5443,7 +5443,8 @@ AT_CLEANUP # This test verifies that mirror state is preserved across recirculation. # # Otherwise, post-recirculation the ingress and the output to port 4 -# would cause the packet to be mirrored to port 3 a second time. +# would cause the packet to be mirrored to port 3 a second time. A register +# is also modified to verify that this doesn't reset the mirror context. AT_SETUP([ofproto-dpif - mirroring with recirculation]) AT_KEYWORDS([mirror mirrors mirroring]) OVS_VSWITCHD_START @@ -5454,7 +5455,7 @@ ovs-vsctl \ --id=@m create Mirror name=mymirror select_all=true output_port=@p3 AT_DATA([flows.txt], [dnl -in_port=1 actions=2,debug_recirc,4 +in_port=1 actions=2,debug_recirc,set_field:1->reg4,4 ]) AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
Previously OVS reset the mirror contents when a packet is modified in such a way that the packets contents changes. However, this change incorrectly reset that mirror context when only metadata changes as well. Now we check for all metadata fields, instead of just tunnel metadata, before resetting the mirror context. Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.") Reported-by: Zhangweiwei <zhang.weiwei@h3c.com> Signed-off-by: Mike Pattrick <mkp@redhat.com> --- include/openvswitch/meta-flow.h | 1 + lib/meta-flow.c | 109 ++++++++++++++++++++++++++++++++ ofproto/ofproto-dpif-xlate.c | 2 +- tests/ofproto-dpif.at | 5 +- 4 files changed, 114 insertions(+), 3 deletions(-)