Jarno, I think that you'd be a good person to review this. Would you mind taking a look?
Thanks, Ben. On Sun, May 08, 2016 at 10:34:10AM -0700, Ben Pfaff wrote: > IGMP translations wasn't setting enough bits in the wildcards to ensure > different packets were handled differently. > > Reported-by: "O'Reilly, Darragh" <darragh.orei...@hpe.com> > Reported-at: http://openvswitch.org/pipermail/discuss/2016-April/021036.html > Signed-off-by: Ben Pfaff <b...@ovn.org> > --- > AUTHORS | 1 + > lib/flow.h | 69 > ++++++++++++++++++++++++++++++++------------ > lib/meta-flow.c | 10 +++---- > lib/nx-match.c | 4 +-- > lib/odp-util.c | 4 +-- > ofproto/ofproto-dpif-xlate.c | 28 +++++++++++++----- > 6 files changed, 80 insertions(+), 36 deletions(-) > > diff --git a/AUTHORS b/AUTHORS > index cc82388..22f527c 100644 > --- a/AUTHORS > +++ b/AUTHORS > @@ -292,6 +292,7 @@ Christian Stigen Larsen cslar...@gmail.com > Christopher Paggen cpag...@cisco.com > Chunhe Li lichu...@huawei.com > Daniel Badea daniel.ba...@windriver.com > +Darragh O'Reilly darragh.orei...@hpe.com > Dave Walker davewal...@ubuntu.com > David Evans davidjoshuaev...@gmail.com > David Palma pa...@onesource.pt > diff --git a/lib/flow.h b/lib/flow.h > index 0196ee7..22b245c 100644 > --- a/lib/flow.h > +++ b/lib/flow.h > @@ -832,41 +832,72 @@ static inline bool is_ip_any(const struct flow *flow) > return dl_type_is_ip_any(flow->dl_type); > } > > -static inline bool is_icmpv4(const struct flow *flow) > +static inline bool is_icmpv4(const struct flow *flow, > + struct flow_wildcards *wc) > { > - return (flow->dl_type == htons(ETH_TYPE_IP) > - && flow->nw_proto == IPPROTO_ICMP); > + if (flow->dl_type == htons(ETH_TYPE_IP)) { > + if (wc) { > + memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); > + } > + return flow->nw_proto == IPPROTO_ICMP; > + } > + return false; > } > > -static inline bool is_icmpv6(const struct flow *flow) > +static inline bool is_icmpv6(const struct flow *flow, > + struct flow_wildcards *wc) > { > - return (flow->dl_type == htons(ETH_TYPE_IPV6) > - && flow->nw_proto == IPPROTO_ICMPV6); > + if (flow->dl_type == htons(ETH_TYPE_IPV6)) { > + if (wc) { > + memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); > + } > + return flow->nw_proto == IPPROTO_ICMPV6; > + } > + return false; > } > > -static inline bool is_igmp(const struct flow *flow) > +static inline bool is_igmp(const struct flow *flow, struct flow_wildcards > *wc) > { > - return (flow->dl_type == htons(ETH_TYPE_IP) > - && flow->nw_proto == IPPROTO_IGMP); > + if (flow->dl_type == htons(ETH_TYPE_IP)) { > + if (wc) { > + memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); > + } > + return flow->nw_proto == IPPROTO_IGMP; > + } > + return false; > } > > -static inline bool is_mld(const struct flow *flow) > +static inline bool is_mld(const struct flow *flow, > + struct flow_wildcards *wc) > { > - return is_icmpv6(flow) > - && (flow->tp_src == htons(MLD_QUERY) > - || flow->tp_src == htons(MLD_REPORT) > - || flow->tp_src == htons(MLD_DONE) > - || flow->tp_src == htons(MLD2_REPORT)); > + if (is_icmpv6(flow, wc)) { > + if (wc) { > + memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); > + } > + return (flow->tp_src == htons(MLD_QUERY) > + || flow->tp_src == htons(MLD_REPORT) > + || flow->tp_src == htons(MLD_DONE) > + || flow->tp_src == htons(MLD2_REPORT)); > + } > + return false; > } > > -static inline bool is_mld_query(const struct flow *flow) > +static inline bool is_mld_query(const struct flow *flow, > + struct flow_wildcards *wc) > { > - return is_icmpv6(flow) && flow->tp_src == htons(MLD_QUERY); > + if (is_icmpv6(flow, wc)) { > + if (wc) { > + memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); > + } > + return flow->tp_src == htons(MLD_QUERY); > + } > + return false; > } > > -static inline bool is_mld_report(const struct flow *flow) > +static inline bool is_mld_report(const struct flow *flow, > + struct flow_wildcards *wc) > { > - return is_mld(flow) && !is_mld_query(flow); > + return is_mld(flow, wc) && !is_mld_query(flow, wc); > } > > static inline bool is_stp(const struct flow *flow) > diff --git a/lib/meta-flow.c b/lib/meta-flow.c > index ce60f22..a23c01f 100644 > --- a/lib/meta-flow.c > +++ b/lib/meta-flow.c > @@ -393,21 +393,21 @@ mf_are_prereqs_ok(const struct mf_field *mf, const > struct flow *flow) > return is_ip_any(flow) && flow->nw_proto == IPPROTO_SCTP > && !(flow->nw_frag & FLOW_NW_FRAG_LATER); > case MFP_ICMPV4: > - return is_icmpv4(flow); > + return is_icmpv4(flow, NULL); > case MFP_ICMPV6: > - return is_icmpv6(flow); > + return is_icmpv6(flow, NULL); > > case MFP_ND: > - return (is_icmpv6(flow) > + return (is_icmpv6(flow, NULL) > && flow->tp_dst == htons(0) > && (flow->tp_src == htons(ND_NEIGHBOR_SOLICIT) || > flow->tp_src == htons(ND_NEIGHBOR_ADVERT))); > case MFP_ND_SOLICIT: > - return (is_icmpv6(flow) > + return (is_icmpv6(flow, NULL) > && flow->tp_dst == htons(0) > && (flow->tp_src == htons(ND_NEIGHBOR_SOLICIT))); > case MFP_ND_ADVERT: > - return (is_icmpv6(flow) > + return (is_icmpv6(flow, NULL) > && flow->tp_dst == htons(0) > && (flow->tp_src == htons(ND_NEIGHBOR_ADVERT))); > } > diff --git a/lib/nx-match.c b/lib/nx-match.c > index 3c48570..aad6e02 100644 > --- a/lib/nx-match.c > +++ b/lib/nx-match.c > @@ -861,7 +861,7 @@ nxm_put_ip(struct ofpbuf *b, const struct match *match, > enum ofp_version oxm) > match->wc.masks.tp_src); > nxm_put_16m(b, MFF_SCTP_DST, oxm, flow->tp_dst, > match->wc.masks.tp_dst); > - } else if (is_icmpv4(flow)) { > + } else if (is_icmpv4(flow, NULL)) { > if (match->wc.masks.tp_src) { > nxm_put_8(b, MFF_ICMPV4_TYPE, oxm, > ntohs(flow->tp_src)); > @@ -870,7 +870,7 @@ nxm_put_ip(struct ofpbuf *b, const struct match *match, > enum ofp_version oxm) > nxm_put_8(b, MFF_ICMPV4_CODE, oxm, > ntohs(flow->tp_dst)); > } > - } else if (is_icmpv6(flow)) { > + } else if (is_icmpv6(flow, NULL)) { > if (match->wc.masks.tp_src) { > nxm_put_8(b, MFF_ICMPV6_TYPE, oxm, > ntohs(flow->tp_src)); > diff --git a/lib/odp-util.c b/lib/odp-util.c > index e0a1ad4..9144e56 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -5719,9 +5719,9 @@ commit_set_icmp_action(const struct flow *flow, struct > flow *base_flow, > struct ovs_key_icmp key, mask, base; > enum ovs_key_attr attr; > > - if (is_icmpv4(flow)) { > + if (is_icmpv4(flow, NULL)) { > attr = OVS_KEY_ATTR_ICMP; > - } else if (is_icmpv6(flow)) { > + } else if (is_icmpv6(flow, NULL)) { > attr = OVS_KEY_ATTR_ICMPV6; > } else { > return 0; > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index cdf5a83..08eabf5 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -2359,6 +2359,20 @@ xlate_normal_flood(struct xlate_ctx *ctx, struct > xbundle *in_xbundle, > ctx->nf_output_iface = NF_OUT_FLOOD; > } > > +static bool > +is_ip_local_multicast(const struct flow *flow, struct flow_wildcards *wc) > +{ > + if (flow->dl_type == htons(ETH_TYPE_IP)) { > + memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst); > + return ip_is_local_multicast(flow->nw_dst); > + } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) { > + memset(&wc->masks.ipv6_dst, 0xff, sizeof wc->masks.ipv6_dst); > + return ipv6_is_all_hosts(&flow->ipv6_dst); > + } else { > + return false; > + } > +} > + > static void > xlate_normal(struct xlate_ctx *ctx) > { > @@ -2442,7 +2456,8 @@ xlate_normal(struct xlate_ctx *ctx) > struct mcast_snooping *ms = ctx->xbridge->ms; > struct mcast_group *grp = NULL; > > - if (is_igmp(flow)) { > + if (is_igmp(flow, wc)) { > + memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src); > if (mcast_snooping_is_membership(flow->tp_src) || > mcast_snooping_is_query(flow->tp_src)) { > if (ctx->xin->may_learn && ctx->xin->packet) { > @@ -2475,13 +2490,13 @@ xlate_normal(struct xlate_ctx *ctx) > xlate_normal_flood(ctx, in_xbundle, vlan); > } > return; > - } else if (is_mld(flow)) { > + } else if (is_mld(flow, wc)) { > ctx->xout->slow |= SLOW_ACTION; > if (ctx->xin->may_learn && ctx->xin->packet) { > update_mcast_snooping_table(ctx->xbridge, flow, vlan, > in_xbundle, ctx->xin->packet); > } > - if (is_mld_report(flow)) { > + if (is_mld_report(flow, wc)) { > ovs_rwlock_rdlock(&ms->rwlock); > xlate_normal_mcast_send_mrouters(ctx, ms, in_xbundle, vlan); > xlate_normal_mcast_send_rports(ctx, ms, in_xbundle, vlan); > @@ -2491,10 +2506,7 @@ xlate_normal(struct xlate_ctx *ctx) > xlate_normal_flood(ctx, in_xbundle, vlan); > } > } else { > - if ((flow->dl_type == htons(ETH_TYPE_IP) > - && ip_is_local_multicast(flow->nw_dst)) > - || (flow->dl_type == htons(ETH_TYPE_IPV6) > - && ipv6_is_all_hosts(&flow->ipv6_dst))) { > + if (is_ip_local_multicast(flow, wc)) { > /* RFC4541: section 2.1.2, item 2: Packets with a dst IP > * address in the 224.0.0.x range which are not IGMP must > * be forwarded on all ports */ > @@ -5030,7 +5042,7 @@ xlate_wc_finish(struct xlate_ctx *ctx) > * Avoid the problem here by making sure that only the low 8 bits of > * either field can be unwildcarded for ICMP. > */ > - if (is_icmpv4(&ctx->xin->flow) || is_icmpv6(&ctx->xin->flow)) { > + if (is_icmpv4(&ctx->xin->flow, NULL) || is_icmpv6(&ctx->xin->flow, > NULL)) { > ctx->wc->masks.tp_src &= htons(UINT8_MAX); > ctx->wc->masks.tp_dst &= htons(UINT8_MAX); > } > -- > 2.1.3 > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
