Acked-by: Jarno Rajahalme <ja...@ovn.org>
> On Dec 9, 2015, at 6:27 PM, Daniele Di Proietto <diproiet...@vmware.com>
> wrote:
>
> 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()).
“xlate_actions()”
>
> This fixes a problem related to the handling of Geneve options.
> Unwildcarding non existing Geneve options (as done by a
“non-existing”
> 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>
> ---
> lib/meta-flow.c | 89 ++++++++++++++++++++++++++++++++++++++++++++
> lib/meta-flow.h | 1 +
> lib/nx-match.c | 4 +-
> ofproto/ofproto-dpif-xlate.c | 4 +-
> 4 files changed, 96 insertions(+), 2 deletions(-)
>
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index 398b2f2..e6287d5 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -1421,6 +1421,95 @@ 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_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_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.
> *
> diff --git a/lib/meta-flow.h b/lib/meta-flow.h
> index 1e6055b..b18cd97 100644
> --- a/lib/meta-flow.h
> +++ b/lib/meta-flow.h
> @@ -1993,6 +1993,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 *);
> int mf_field_len(const struct mf_field *, const union mf_value *value,
> diff --git a/lib/nx-match.c b/lib/nx-match.c
> index f56c9e6..9d141ae 100644
> --- a/lib/nx-match.c
> +++ b/lib/nx-match.c
> @@ -1610,7 +1610,9 @@ nxm_execute_reg_move(const struct ofpact_reg_move *move,
> if (mf_check_prereqs_and_set_mask(move->dst.field, flow, wc)
> && mf_check_prereqs_and_set_mask(move->src.field, flow, wc)) {
>
> - mf_mask_field(move->dst.field, &wc->masks);
> + if (mf_set_must_mask(move->dst.field)) {
> + mf_mask_field(move->dst.field, &wc->masks);
> + }
> mf_mask_field(move->src.field, &wc->masks);
>
> mf_get_value(move->dst.field, flow, &dst_value);
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 08807bc..eff938b 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4595,7 +4595,9 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> /* A flow may wildcard nw_frag. Do nothing if setting a transport
> * header field on a packet that does not have them. */
> if (mf_check_prereqs_and_set_mask(mf, flow, wc)) {
> - mf_mask_field(mf, &wc->masks);
> + if (mf_set_must_mask(mf)) {
> + mf_mask_field(mf, &wc->masks);
> + }
> mf_set_flow_value_masked(mf, &set_field->value,
> &set_field->mask, flow);
> }
> --
> 2.1.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
