Skip to content

Commit

Permalink
ofproto-dpif-xlate: Fix IGMP megaflow matching.
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@blp
blp committed May 20, 2016
1 parent 36d8de1 commit a75636c
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 36 deletions.
69 changes: 50 additions & 19 deletions lib/flow.h
View file
Expand Up @@ -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)
Expand Down
10 changes: 5 additions & 5 deletions lib/meta-flow.c
View file
Expand Up @@ -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)));
}
Expand Down
4 changes: 2 additions & 2 deletions lib/nx-match.c
View file
Expand Up @@ -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));
Expand All @@ -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));
Expand Down
4 changes: 2 additions & 2 deletions lib/odp-util.c
View file
Expand Up @@ -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;
Expand Down
28 changes: 20 additions & 8 deletions ofproto/ofproto-dpif-xlate.c
View file
Expand Up @@ -2388,6 +2388,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)
{
Expand Down Expand Up @@ -2471,7 +2485,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) {
Expand Down Expand Up @@ -2504,13 +2519,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);
Expand All @@ -2520,10 +2535,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 */
Expand Down Expand Up @@ -5066,7 +5078,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);
}
Expand Down

0 comments on commit a75636c

Please sign in to comment.