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