> On Dec 9, 2015, at 6:27 PM, Daniele Di Proietto <diproiet...@vmware.com>
> wrote:
>
> During translation we need to unwildcard each member of the flow that we
> look at. When setting or moving a field, we need to look at (and
> consequently unwildcard) the field itself an all the prerequisites.
>
> The current code unwildcards the field and all the prerequisites and
> then reads them. This behavior is wrong, because if prerequisites are
> not met we might end up unwildcarding more fields than necessary,
> generating masks that don't make sense.
>
Good catch!
> The bug can be triggered with an action that sets the tcp src port and
> a fragmented IP packet. The translation will not generate a set action,
> but it will match on the target field (tcp src port), even though the
> prerequisites for that are not met.
>
> The wrong mask causes the flow to get deleted by revalidation.
>
> Fix this by introducing mf_check_prereqs_and_set_mask(), which checks
> the prerequisites while masking the field. It is based on the behavior
> of mf_are_prereqs_ok().
>
> mf_mask_field_and_prereqs() is not used anymore, so it can be removed.
> mf_are_prereqs_ok() can be rewritten using
> mf_check_prereqs_and_set_mask(), avoiding code duplication
>
> Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com>
> ---
> lib/flow.h | 38 +++++++++++++--
> lib/meta-flow.c | 113 +++++++++++++++++++------------------------
> lib/meta-flow.h | 5 +-
> lib/nx-match.c | 10 ++--
> ofproto/ofproto-dpif-xlate.c | 4 +-
> 5 files changed, 92 insertions(+), 78 deletions(-)
>
> diff --git a/lib/flow.h b/lib/flow.h
> index 5d78615..6f99297 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -999,21 +999,49 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const
> struct flow *flow)
> md->ct_label = flow->ct_label;
> }
>
> +/* Often, during translation we need to read a value from a flow and
> + * unwildcard the corresponding bits in the wildcards. */
> +
> +#define FLOW_WC_GET_AND_MASK(FLOW, WC, FIELD) \
> + (((WC) ? WC_MASK_FIELD(WC, FIELD) : NULL), (FLOW)->FIELD)
> +
> +#define FLOW_WC_GET_AND_MASK_MASKED(FLOW, WC, FIELD, MASK) \
> + (((WC) ? WC_MASK_FIELD_MASK(WC, FIELD, MASK) : 0), \
> + (((FLOW)->FIELD) & (MASK)))
> +
These would be better named as FLOW_GET_AND_MASK_WC{_MASKED}(). Now the names
see like they would return a field from the WC.
> +static inline bool check_and_mask_ip_any(const struct flow *flow,
> + struct flow_wildcards *wc)
> +{
> + return dl_type_is_ip_any(FLOW_WC_GET_AND_MASK(flow, wc, dl_type));
> +}
While correct, masking here unnecessary, as dl_type is always unwildcarded in
the beginning of xlate_wc_init(). We also rely on that everywhere, so we might
do it here as well. The reasoning for this is that dl_type is always read and
in so many places that it is better to mask it unconditionally. Given this the
callers can keep using dl_type_is_ip_any() directly.
> +
> +static inline bool check_and_mask_icmpv4(const struct flow *flow,
> + struct flow_wildcards *wc)
> +{
> + return (FLOW_WC_GET_AND_MASK(flow, wc, dl_type) == htons(ETH_TYPE_IP)
ditto.
> + && FLOW_WC_GET_AND_MASK(flow, wc, nw_proto) == IPPROTO_ICMP);
> +}
> +
> +static inline bool check_and_mask_icmpv6(const struct flow *flow,
> + struct flow_wildcards *wc)
> +{
> + return (FLOW_WC_GET_AND_MASK(flow, wc, dl_type) == htons(ETH_TYPE_IPV6)
ditto.
> + && FLOW_WC_GET_AND_MASK(flow, wc, nw_proto) == IPPROTO_ICMPV6);
> +}
> +
> static inline bool is_ip_any(const struct flow *flow)
> {
> - return dl_type_is_ip_any(flow->dl_type);
> + return check_and_mask_ip_any(flow, NULL);
Given the above this change is not needed.
> }
>
> static inline bool is_icmpv4(const struct flow *flow)
> {
> - return (flow->dl_type == htons(ETH_TYPE_IP)
> - && flow->nw_proto == IPPROTO_ICMP);
> + return check_and_mask_icmpv4(flow, NULL);
Given that there is no mask I would not change this either.
> }
>
> static inline bool is_icmpv6(const struct flow *flow)
> {
> - return (flow->dl_type == htons(ETH_TYPE_IPV6)
> - && flow->nw_proto == IPPROTO_ICMPV6);
> + return check_and_mask_icmpv6(flow, NULL);
Nor this.
> }
>
> static inline bool is_igmp(const struct flow *flow)
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index 6bd0b99..398b2f2 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -356,94 +356,79 @@ mf_is_mask_valid(const struct mf_field *mf, const union
> mf_value *mask)
> bool
> mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow)
> {
> + return mf_check_prereqs_and_set_mask(mf, flow, NULL);
> +}
> +
> +/* Returns true if 'flow' meets the prerequisites for 'mf', false otherwise.
> + * If an attribute is read from 'flow', the corresponding attribute in 'wc'
> + * will be unwildcarded. */
> +bool
> +mf_check_prereqs_and_set_mask(const struct mf_field *mf,
> + const struct flow *flow,
> + struct flow_wildcards *wc)
> +{
> switch (mf->prereqs) {
> case MFP_NONE:
> return true;
>
> case MFP_ARP:
> - return (flow->dl_type == htons(ETH_TYPE_ARP) ||
> - flow->dl_type == htons(ETH_TYPE_RARP));
> + return FLOW_WC_GET_AND_MASK(flow, wc, dl_type) == htons(ETH_TYPE_ARP)
> + || FLOW_WC_GET_AND_MASK(flow, wc, dl_type) ==
> htons(ETH_TYPE_RARP);
No change needed.
> case MFP_IPV4:
> - return flow->dl_type == htons(ETH_TYPE_IP);
> + return FLOW_WC_GET_AND_MASK(flow, wc, dl_type) == htons(ETH_TYPE_IP);
No change needed.
> case MFP_IPV6:
> - return flow->dl_type == htons(ETH_TYPE_IPV6);
> + return FLOW_WC_GET_AND_MASK(flow, wc, dl_type) ==
> htons(ETH_TYPE_IPV6);
No change needed.
> case MFP_VLAN_VID:
> - return (flow->vlan_tci & htons(VLAN_CFI)) != 0;
> + return FLOW_WC_GET_AND_MASK_MASKED(flow, wc,
> + vlan_tci, htons(VLAN_CFI)) != 0;
> case MFP_MPLS:
> - return eth_type_mpls(flow->dl_type);
> + return eth_type_mpls(FLOW_WC_GET_AND_MASK(flow, wc, dl_type));
No change needed.
> case MFP_IP_ANY:
> - return is_ip_any(flow);
> -
> + return check_and_mask_ip_any(flow, wc);
No change needed here.
> case MFP_TCP:
> - return is_ip_any(flow) && flow->nw_proto == IPPROTO_TCP
> - && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
> + return check_and_mask_ip_any(flow, wc)
This need not be changed.
> + && FLOW_WC_GET_AND_MASK(flow, wc, nw_proto) == IPPROTO_TCP
> + && !FLOW_WC_GET_AND_MASK_MASKED(flow, wc, nw_frag,
> + FLOW_NW_FRAG_LATER);
Is the order between these now significant?
> case MFP_UDP:
> - return is_ip_any(flow) && flow->nw_proto == IPPROTO_UDP
> - && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
> + return check_and_mask_ip_any(flow, wc)
> + && FLOW_WC_GET_AND_MASK(flow, wc, nw_proto) == IPPROTO_UDP
> + && !FLOW_WC_GET_AND_MASK_MASKED(flow, wc, nw_frag,
> + FLOW_NW_FRAG_LATER);
> case MFP_SCTP:
> - return is_ip_any(flow) && flow->nw_proto == IPPROTO_SCTP
> - && !(flow->nw_frag & FLOW_NW_FRAG_LATER);
> + return check_and_mask_ip_any(flow, wc)
> + && FLOW_WC_GET_AND_MASK(flow, wc, nw_proto) == IPPROTO_SCTP
> + && !FLOW_WC_GET_AND_MASK_MASKED(flow, wc, nw_frag,
> + FLOW_NW_FRAG_LATER);
> case MFP_ICMPV4:
> - return is_icmpv4(flow);
> + return check_and_mask_icmpv4(flow, wc);
> case MFP_ICMPV6:
> - return is_icmpv6(flow);
> + return check_and_mask_icmpv6(flow, wc);
>
> case MFP_ND:
> - return (is_icmpv6(flow)
> - && flow->tp_dst == htons(0)
> - && (flow->tp_src == htons(ND_NEIGHBOR_SOLICIT) ||
> - flow->tp_src == htons(ND_NEIGHBOR_ADVERT)));
> + return check_and_mask_icmpv6(flow, wc)
> + && FLOW_WC_GET_AND_MASK(flow, wc, tp_dst) == htons(0)
> + && (FLOW_WC_GET_AND_MASK(flow, wc, tp_src)
> + == htons(ND_NEIGHBOR_SOLICIT)
> + || FLOW_WC_GET_AND_MASK(flow, wc, tp_src)
> + == htons(ND_NEIGHBOR_ADVERT));
Same here, is the order for checking and masking tp_dst and tp_src significant?
> case MFP_ND_SOLICIT:
> - return (is_icmpv6(flow)
> - && flow->tp_dst == htons(0)
> - && (flow->tp_src == htons(ND_NEIGHBOR_SOLICIT)));
> + return check_and_mask_icmpv6(flow, wc)
> + && FLOW_WC_GET_AND_MASK(flow, wc, tp_dst)
> + == htons(0)
> + && FLOW_WC_GET_AND_MASK(flow, wc, tp_src)
> + == htons(ND_NEIGHBOR_SOLICIT);
> case MFP_ND_ADVERT:
> - return (is_icmpv6(flow)
> - && flow->tp_dst == htons(0)
> - && (flow->tp_src == htons(ND_NEIGHBOR_ADVERT)));
> + return check_and_mask_icmpv6(flow, wc)
> + && FLOW_WC_GET_AND_MASK(flow, wc, tp_dst)
> + == htons(0)
> + && FLOW_WC_GET_AND_MASK(flow, wc, tp_src)
> + == htons(ND_NEIGHBOR_ADVERT);
> }
>
> OVS_NOT_REACHED();
> }
>
> -/* Set field and it's prerequisities in the mask.
> - * This is only ever called for writeable 'mf's, but we do not make the
> - * distinction here. */
> -void
> -mf_mask_field_and_prereqs(const struct mf_field *mf, struct flow_wildcards
> *wc)
> -{
> - mf_set_flow_value(mf, &exact_match_mask, &wc->masks);
> -
> - switch (mf->prereqs) {
> - case MFP_ND:
> - case MFP_ND_SOLICIT:
> - case MFP_ND_ADVERT:
> - WC_MASK_FIELD(wc, tp_src);
> - WC_MASK_FIELD(wc, tp_dst);
> - /* Fall through. */
> - case MFP_TCP:
> - case MFP_UDP:
> - case MFP_SCTP:
> - case MFP_ICMPV4:
> - case MFP_ICMPV6:
> - /* nw_frag always unwildcarded. */
> - WC_MASK_FIELD(wc, nw_proto);
> - /* Fall through. */
> - case MFP_ARP:
> - case MFP_IPV4:
> - case MFP_IPV6:
> - case MFP_MPLS:
> - case MFP_IP_ANY:
> - /* dl_type always unwildcarded. */
> - break;
> - case MFP_VLAN_VID:
> - WC_MASK_FIELD_MASK(wc, vlan_tci, htons(VLAN_CFI));
> - break;
> - case MFP_NONE:
> - break;
> - }
> -}
> -
> /* Set bits of 'bm' corresponding to the field 'mf' and it's prerequisities.
> */
> void
> mf_bitmap_set_field_and_prereqs(const struct mf_field *mf, struct mf_bitmap
> *bm)
> diff --git a/lib/meta-flow.h b/lib/meta-flow.h
> index 71c238d..1e6055b 100644
> --- a/lib/meta-flow.h
> +++ b/lib/meta-flow.h
> @@ -1967,8 +1967,9 @@ void mf_get_mask(const struct mf_field *, const struct
> flow_wildcards *,
>
> /* Prerequisites. */
> bool mf_are_prereqs_ok(const struct mf_field *, const struct flow *);
> -void mf_mask_field_and_prereqs(const struct mf_field *,
> - struct flow_wildcards *);
> +bool mf_check_prereqs_and_set_mask(const struct mf_field *mf,
> + const struct flow *flow,
> + struct flow_wildcards *wc);
> void mf_bitmap_set_field_and_prereqs(const struct mf_field *mf, struct
> mf_bitmap *bm);
>
> diff --git a/lib/nx-match.c b/lib/nx-match.c
> index 11bcd95..f56c9e6 100644
> --- a/lib/nx-match.c
> +++ b/lib/nx-match.c
> @@ -1605,13 +1605,13 @@ nxm_execute_reg_move(const struct ofpact_reg_move
> *move,
> union mf_value src_value;
> union mf_value dst_value;
>
> - mf_mask_field_and_prereqs(move->dst.field, wc);
> - mf_mask_field_and_prereqs(move->src.field, wc);
> -
> /* A flow may wildcard nw_frag. Do nothing if setting a transport
> * header field on a packet that does not have them. */
> - if (mf_are_prereqs_ok(move->dst.field, flow)
> - && mf_are_prereqs_ok(move->src.field, flow)) {
> + if (mf_check_prereqs_and_set_mask(move->dst.field, flow, wc)
> + && mf_check_prereqs_and_set_mask(move->src.field, flow, wc)) {
Would checking and masking in the opposite order make a difference?
> +
> + 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);
> mf_get_value(move->src.field, flow, &src_value);
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index dab64b9..08807bc 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4594,8 +4594,8 @@ 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. */
> - mf_mask_field_and_prereqs(mf, wc);
> - if (mf_are_prereqs_ok(mf, flow)) {
> + if (mf_check_prereqs_and_set_mask(mf, flow, wc)) {
> + 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
