When translating a set action we also unwildcard the field in question.
This is done to correctly translate set actions with the value identical
to the ingress flow, like in the following example:
flow table:
tcp,actions=set_field:80->tcp_dst,output:5
ingress packet:
...,tcp,tcp_dst=80
datapath flow
...,tcp(dst=80) actions:5
The datapath flow must exact match the target field, because the actions
do not include a set field. (Otherwise a packet coming in with a
different tcp_dst would be matched, and its port wouldn't be altered).
Tunnel attributes behave differently: at the datapath layer, before
action processing they're cleared (we do the same at the ofproto layer
in xlate_actions()). Therefore there's no need to unwildcard them,
because a set action would always be detected (since we zero them at the
beginning of xlate_ations()).
This fixes a problem related to the handling of Geneve options.
Unwildcarding non existing Geneve options (as done by a
set_field:X->tun_metadata<n> on a packet coming from a non-tunnel
interface) would be problematic for the datapaths: the ODP translation
functions cannot express a match on non existing Geneve options (unlike
on other attributes), and the userspace datapath wouldn't be able to
translate them to "userspace datapath format". In both cases the
installed flow would be deleted by revalidation at the first
opportunity.
Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com>
---
include/openvswitch/meta-flow.h | 1 +
lib/meta-flow.c | 95 ++++++++++++++++++++++++++++++++++++++++-
ofproto/ofproto-dpif-xlate.c | 4 +-
3 files changed, 98 insertions(+), 2 deletions(-)
diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 23f9916..f21df4b 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -2094,6 +2094,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_set_must_mask(const struct mf_field *);
bool mf_is_set(const struct mf_field *, const struct flow *);
void mf_mask_field(const struct mf_field *, struct flow_wildcards *);
void mf_mask_field_masked(const struct mf_field *, const union mf_value *mask,
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 3dc2770..86a0f03 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -1407,6 +1407,97 @@ mf_is_tun_metadata(const struct mf_field *mf)
mf->id < MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS;
}
+/* Returns true if a field 'mf' should be exact matched before being set
+ * by the action translation, false otherwise. Most of the fields need
+ * an exact match.*/
+bool
+mf_set_must_mask(const struct mf_field *mf)
+{
+ /* Tunnel attributes don't need an exact match, because they are
+ * cleared by the datapath between ingress and egress. Also, an
+ * exact match on tunnel metadata might be problematic, because
+ * it is not possible to express it if the metadata didn't exist
+ * on ingress. */
+ switch (mf->id) {
+ 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_TOS:
+ case MFF_TUN_TTL:
+ CASE_MFF_TUN_METADATA:
+ return false;
+
+ case MFF_DP_HASH:
+ case MFF_RECIRC_ID:
+ case MFF_CONJ_ID:
+ 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_REGS:
+ CASE_MFF_XREGS:
+ CASE_MFF_XXREGS:
+ 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_ARP_SPA:
+ case MFF_IPV4_DST:
+ case MFF_ARP_TPA:
+ 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_SHA:
+ case MFF_ND_SLL:
+ case MFF_ARP_THA:
+ case MFF_ND_TLL:
+ case MFF_TCP_SRC:
+ case MFF_UDP_SRC:
+ case MFF_SCTP_SRC:
+ case MFF_ICMPV4_TYPE:
+ case MFF_ICMPV6_TYPE:
+ case MFF_TCP_DST:
+ case MFF_UDP_DST:
+ case MFF_SCTP_DST:
+ case MFF_ICMPV4_CODE:
+ case MFF_ICMPV6_CODE:
+ case MFF_TCP_FLAGS:
+ case MFF_ND_TARGET:
+ return true;
+
+ case MFF_N_IDS:
+ default:
+ OVS_NOT_REACHED();
+ }
+}
+
/* Returns true if 'mf' has previously been set in 'flow', false if
* it contains a non-default value.
*
@@ -1988,7 +2079,9 @@ mf_subfield_copy(const struct mf_subfield *src,
if (mf_are_prereqs_ok(dst->field, flow, wc)
&& mf_are_prereqs_ok(src->field, flow, wc)) {
unwildcard_subfield(src, wc);
- unwildcard_subfield(dst, wc);
+ if (mf_set_must_mask(dst->field)) {
+ unwildcard_subfield(dst, wc);
+ }
union mf_value src_value;
union mf_value dst_value;
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 893c033..0118d01 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4924,7 +4924,9 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
/* Set the field only if the packet actually has it. */
if (mf_are_prereqs_ok(mf, flow, wc)) {
- mf_mask_field_masked(mf, &set_field->mask, wc);
+ if (mf_set_must_mask(mf)) {
+ mf_mask_field_masked(mf, &set_field->mask, wc);
+ }
mf_set_flow_value_masked(mf, &set_field->value,
&set_field->mask, flow);
}
--
2.9.3
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
