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 <[email protected]>
> Signed-off-by: Mike Pattrick <[email protected]>
> ---
> 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])
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
