On Wed, Oct 2, 2024 at 7:40 AM Ilya Maximets <[email protected]> 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 <[email protected]>
> > ---
> > 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;
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
