| Message ID | 1449714462-31831-8-git-send-email-diproiettod@vmware.com |
|---|---|
| State | Changes Requested |
| Headers | show |
> On Dec 9, 2015, at 6:27 PM, Daniele Di Proietto <diproiettod@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 <diproiettod@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
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))) + +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)); +} + +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) + && 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) + && 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); } 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); } 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); } 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); 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); 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); 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)); case MFP_IP_ANY: - return is_ip_any(flow); - + return check_and_mask_ip_any(flow, wc); 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) + && 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); 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)); 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)) { + + 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); }
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. 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 <diproiettod@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(-)