| Message ID | 1462728850-16787-1-git-send-email-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
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.oreilly@hpe.com> > Reported-at: http://openvswitch.org/pipermail/discuss/2016-April/021036.html > Signed-off-by: Ben Pfaff <blp@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 cslarsen@gmail.com > Christopher Paggen cpaggen@cisco.com > Chunhe Li lichunhe@huawei.com > Daniel Badea daniel.badea@windriver.com > +Darragh O'Reilly darragh.oreilly@hpe.com > Dave Walker DaveWalker@ubuntu.com > David Evans davidjoshuaevans@gmail.com > David Palma palma@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 >
diff --git a/AUTHORS b/AUTHORS index cc82388..22f527c 100644 --- a/AUTHORS +++ b/AUTHORS @@ -292,6 +292,7 @@ Christian Stigen Larsen cslarsen@gmail.com Christopher Paggen cpaggen@cisco.com Chunhe Li lichunhe@huawei.com Daniel Badea daniel.badea@windriver.com +Darragh O'Reilly darragh.oreilly@hpe.com Dave Walker DaveWalker@ubuntu.com David Evans davidjoshuaevans@gmail.com David Palma palma@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); }
IGMP translations wasn't setting enough bits in the wildcards to ensure different packets were handled differently. Reported-by: "O'Reilly, Darragh" <darragh.oreilly@hpe.com> Reported-at: http://openvswitch.org/pipermail/discuss/2016-April/021036.html Signed-off-by: Ben Pfaff <blp@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(-)