Skip to content

Commit

Permalink
ofproto-dpif-upcall: Fix redundant mirror on metadata modification.
Browse files Browse the repository at this point in the history 
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: feed7f6 ("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]>
Signed-off-by: Ilya Maximets <[email protected]>
  • Loading branch information
@mkp-rh @igsilya
mkp-rh authored and igsilya committed Oct 30, 2024
1 parent 06b8b9e commit 49a249f
Show file tree
Hide file tree
Showing 4 changed files with 138 additions and 1 deletion.
1 change: 1 addition & 0 deletions include/openvswitch/meta-flow.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_any_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 *);
Expand Down
109 changes: 109 additions & 0 deletions lib/meta-flow.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_any_metadata(const struct mf_field *mf)
{
switch (mf->id) {
case MFF_DP_HASH:
case MFF_RECIRC_ID:
case MFF_PACKET_TYPE:
case MFF_CONJ_ID:
case MFF_TUN_ERSPAN_DIR:
CASE_MFF_TUN_METADATA:
case MFF_METADATA:
case MFF_IN_PORT:
case MFF_IN_PORT_OXM:
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_REGS:
CASE_MFF_XREGS:
CASE_MFF_XXREGS:
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_TTL:
case MFF_TUN_TOS:
case MFF_TUN_GBP_ID:
case MFF_TUN_GBP_FLAGS:
case MFF_TUN_ERSPAN_IDX:
case MFF_TUN_ERSPAN_VER:
case MFF_TUN_ERSPAN_HWID:
case MFF_TUN_GTPU_FLAGS:
case MFF_TUN_GTPU_MSGTYPE:
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_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:
case MFF_NSH_TTL:
return false;

case MFF_N_IDS:
default:
OVS_NOT_REACHED();
}
}

bool
mf_is_frozen_metadata(const struct mf_field *mf)
{
Expand Down
2 changes: 1 addition & 1 deletion ofproto/ofproto-dpif-xlate.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_any_metadata(mf)) {
ctx->mirrors = 0;
}
return;
Expand Down
27 changes: 27 additions & 0 deletions tests/ofproto-dpif.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5568,6 +5568,33 @@ 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=@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, duplicate packet shouldn't be delivered to mirror.
m4_define([ICMP_FLOW], [m4_join([,],
[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 "ICMP_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
Expand Down

0 comments on commit 49a249f

Please sign in to comment.