[ovs-dev] [PATCH] ofproto-dpif-xlate: Fix IGMP megaflow matching.

Thadeu Lima de Souza Cascardo cascardo at redhat.com
Thu May 19 12:59:57 UTC 2016


On Wed, May 18, 2016 at 05:56:02PM -0700, Ben Pfaff wrote:
> Jarno, I think that you'd be a good person to review this.  Would you
> mind taking a look?
> 
> Thanks,
> 
> Ben.

Hi, Ben.

I still need to find some time to try to reproduce the scenarios this solves,
but I have at least one test that is fixed by the section replaced by
is_ip_local_multicast.

I was planning on writing more tests to reproduce the other cases in order to
send a patch, but here it is as a starting point.

Cascardo.

---
diff --git a/tests/automake.mk b/tests/automake.mk
index a5c6074..ae9a436 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -51,6 +51,7 @@ TESTSUITE_AT = \
 	tests/tunnel.at \
 	tests/tunnel-push-pop.at \
 	tests/tunnel-push-pop-ipv6.at \
+	tests/mcast-snooping.at \
 	tests/lockfile.at \
 	tests/reconnect.at \
 	tests/ovs-vswitchd.at \
diff --git a/tests/mcast-snooping.at b/tests/mcast-snooping.at
new file mode 100644
index 0000000..7369e2b
--- /dev/null
+++ b/tests/mcast-snooping.at
@@ -0,0 +1,76 @@
+AT_BANNER([mcast snooping])
+
+AT_SETUP([mcast snooping])
+
+OVS_VSWITCHD_START([])
+AT_CHECK([ovs-vsctl set bridge br0 datapath_type=dummy mcast_snooping_enable=true other-config:mcast-snooping-disable-flood-unregistered=true], [0])
+AT_CHECK([ovs-vsctl add-port br0 p0 -- set Interface p0 type=dummy \
+                       other-config:hwaddr=aa:55:aa:55:00:00 ofport_request=1 \
+                    -- add-port br0 p1 -- set Interface p1 type=dummy \
+                       other-config:hwaddr=aa:55:aa:55:00:01 ofport_request=2 \
+                       ], [0])
+
+AT_CHECK([ovs-appctl dpif/show], [0], [dnl
+dummy at ovs-dummy: hit:0 missed:0
+	br0:
+		br0 65534/100: (dummy)
+		p0 1/1: (dummy)
+		p1 2/2: (dummy)
+])
+
+AT_CHECK([ovs-ofctl add-flow br0 action=normal])
+
+dnl Verify this will not output to the other port
+AT_CHECK([ovs-vsctl set Interface p1 options:tx_pcap=p1.pcap])
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 'in_port(1),eth(src=aa:55:aa:55:00:00,dst=a1:55:aa:55:00:00),eth_type(0x0800),ipv4(src=1.1.3.93,dst=225.0.0.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)'])
+AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1])
+AT_CHECK([cat p1.pcap.txt], [0], [dnl
+])
+
+dnl Verify local multicast will output to the other port
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 'in_port(1),eth(src=aa:55:aa:55:00:00,dst=a1:55:aa:55:00:00),eth_type(0x0800),ipv4(src=1.1.3.92,dst=224.0.0.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)'])
+AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1])
+AT_CHECK([cat p1.pcap.txt], [0], [dnl
+a155aa550000aa55aa55000008004500001c00000000401196730101035ce000000100001f4000000000000000000000000000000000000000000000
+])
+
+dnl Send IGMP join and verify mdb
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 '01005e000016525400aabbcc080046c00028000040000102fc04e00000160000000094040000220017010000000104000000efff0101'])
+AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl
+ port  VLAN  GROUP                Age
+    2     0  239.255.1.1           0
+])
+
+dnl Verify packet to joined address will be output
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 'in_port(1),eth(src=aa:55:aa:55:00:00,dst=a1:55:aa:55:00:00),eth_type(0x0800),ipv4(src=1.1.3.92,dst=239.255.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)'])
+AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1])
+AT_CHECK([cat p1.pcap.txt], [0], [dnl
+a155aa550000aa55aa55000008004500001c00000000401196730101035ce000000100001f4000000000000000000000000000000000000000000000
+a155aa550000aa55aa55000008004500001c00000000401185740101035cefff010100001f4000000000000000000000000000000000000000000000
+])
+
+dnl Send IGMP leave and verify mdb
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 '01005e000016525400aabbcc080046c00028000040000102fc04e00000160000000094040000220014010000000101000000efff0101'])
+AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl
+ port  VLAN  GROUP                Age
+])
+
+dnl Send IGMP join and verify mdb
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 '01005e000016525400aabbcc080046c00028000040000102fc04e00000160000000094040000220017010000000104000000efff0101'])
+AT_CHECK([ovs-appctl mdb/show br0], [0], [dnl
+ port  VLAN  GROUP                Age
+    2     0  239.255.1.1           0
+])
+
+dnl Verify packet to joined address will be output
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 'in_port(1),eth(src=aa:55:aa:55:00:00,dst=a1:55:aa:55:00:00),eth_type(0x0800),ipv4(src=1.1.3.92,dst=239.255.1.1,proto=17,tos=0,ttl=64,frag=no),udp(src=0,dst=8000)'])
+AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1])
+AT_CHECK([cat p1.pcap.txt], [0], [dnl
+a155aa550000aa55aa55000008004500001c00000000401196730101035ce000000100001f4000000000000000000000000000000000000000000000
+a155aa550000aa55aa55000008004500001c00000000401185740101035cefff010100001f4000000000000000000000000000000000000000000000
+a155aa550000aa55aa55000008004500001c00000000401185740101035cefff010100001f4000000000000000000000000000000000000000000000
+])
+
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
diff --git a/tests/testsuite.at b/tests/testsuite.at
index 7ac74df..02502d0 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -49,6 +49,7 @@ m4_include([tests/jsonrpc-py.at])
 m4_include([tests/tunnel.at])
 m4_include([tests/tunnel-push-pop.at])
 m4_include([tests/tunnel-push-pop-ipv6.at])
+m4_include([tests/mcast-snooping.at])
 m4_include([tests/lockfile.at])
 m4_include([tests/reconnect.at])
 m4_include([tests/ovs-vswitchd.at])
---



> 
> 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 at hpe.com>
> > Reported-at: http://openvswitch.org/pipermail/discuss/2016-April/021036.html
> > Signed-off-by: Ben Pfaff <blp at 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 at gmail.com
> >  Christopher Paggen      cpaggen at cisco.com
> >  Chunhe Li               lichunhe at huawei.com
> >  Daniel Badea            daniel.badea at windriver.com
> > +Darragh O'Reilly        darragh.oreilly at hpe.com
> >  Dave Walker             DaveWalker at ubuntu.com
> >  David Evans             davidjoshuaevans at gmail.com
> >  David Palma             palma at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list