On Fri, Oct 4, 2024 at 10:06 AM Eelco Chaudron <[email protected]> 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 <[email protected]> > > Acked-by: Simon Horman <[email protected]> > > Signed-off-by: Mike Pattrick <[email protected]> > > 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
