| Message ID | 1449714462-31831-9-git-send-email-diproiettod@vmware.com |
|---|---|
| State | Superseded |
| Headers | show |
Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Dec 9, 2015, at 6:27 PM, Daniele Di Proietto <diproiettod@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 <diproiettod@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
Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Dec 9, 2015, at 6:27 PM, Daniele Di Proietto <diproiettod@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()). > > 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 <diproiettod@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
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); }
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 <diproiettod@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(-)