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@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.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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
