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 <[email protected]>
Thanks for fixing the unit test. Changes look good to me.
Acked-by: Eelco Chaudron <[email protected]>
> ---
> 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
