Multicast snooping fixes and suggestions

Multicast snooping fixes and suggestions

[Linus Lüssing]

Hello everyone,

While testing the (very awesome!) bridge igmp/mld snooping support I came across
two issues which are breaking IPv6 multicast snooping and IPv6
non-link-local multicast on bridges with multicast snooping support enabled
in general. The first two patches shall fix these issues.

The third one addresses a potential bug on little endian machines which I noticed
during this little code reviewing. This patch is untested though, feedback welcome.

The fourth and fifth patch are a suggestion to also permit using the bridge multicast
snooping feature for link local multimedia multicast traffic. Therefore
using the transient multicast flag instead of the non-link-local scope criteria
seems to be a suitable solution at least for IPv6, in my opinion. Let me know what
you think about it.

Thanks for reviewing these patches.

Cheers,
Linus

[PATCH 1/5] bridge: Fix IPv6 multicast snooping by storing correct protocol type

[Linus Lüssing]

The protocol type for IPv6 entries in the hash table for multicast
bridge snooping is falsely set to ETH_P_IP, marking it as an IPv4
address, instead of setting it to ETH_P_IPV6, which results in negative
look-ups in the hash table later.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 net/bridge/br_multicast.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index f701a21..135d929 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -785,7 +785,7 @@ static int br_ip6_multicast_add_group(struct net_bridge *br,
 		return 0;
 
 	ipv6_addr_copy(&br_group.u.ip6, group);
-	br_group.proto = htons(ETH_P_IP);
+	br_group.proto = htons(ETH_P_IPV6);
 
 	return br_multicast_add_group(br, port, &br_group);
 }
-- 
1.7.2.3

[PATCH 2/5] bridge: Fix IPv6 multicast snooping by correcting offset in MLDv2 report

[Linus Lüssing]

We actually want a pointer to the grec_nsrcr and not the following
field. Otherwise we can get very high values for *nsrcs as the first two
bytes of the IPv6 multicast address are being used instead, leading to
a failing pskb_may_pull() which results in MLDv2 reports not being
parsed.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 net/bridge/br_multicast.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 135d929..45dcf10 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1014,7 +1014,7 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
 
 		nsrcs = skb_header_pointer(skb,
 					   len + offsetof(struct mld2_grec,
-							  grec_mca),
+							  grec_nsrcs),
 					   sizeof(_nsrcs), &_nsrcs);
 		if (!nsrcs)
 			return -EINVAL;
-- 
1.7.2.3

[PATCH 3/5] bridge: Add missing ntohs()s for MLDv2 report parsing

[Linus Lüssing]

The nsrcs number is 2 Byte wide, therefore we need to call ntohs()
before using it.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 net/bridge/br_multicast.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 45dcf10..e8fdaab 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1021,11 +1021,12 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
 
 		if (!pskb_may_pull(skb,
 				   len + sizeof(*grec) +
-				   sizeof(struct in6_addr) * (*nsrcs)))
+				   sizeof(struct in6_addr) * ntohs(*nsrcs)))
 			return -EINVAL;
 
 		grec = (struct mld2_grec *)(skb->data + len);
-		len += sizeof(*grec) + sizeof(struct in6_addr) * (*nsrcs);
+		len += sizeof(*grec) +
+		       sizeof(struct in6_addr) * ntohs(*nsrcs);
 
 		/* We treat these as MLDv1 reports for now. */
 		switch (grec->grec_type) {
-- 
1.7.2.3

[PATCH 4/5] ipv6: Add IPv6 multicast address flag defines

[Linus Lüssing]

This commit adds the missing IPv6 multicast address flag defines to
complement the already existing multicast address scope defines and to
be able to check these flags nicely in the future.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 include/net/ipv6.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 4a3cd2c..96e50e0 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -89,6 +89,18 @@
 #define IPV6_ADDR_SCOPE_GLOBAL		0x0e
 
 /*
+ *	Addr flags
+ */
+#ifdef __KERNEL__
+#define IPV6_ADDR_MC_FLAG_TRANSIENT(a)	\
+	((a)->s6_addr[1] & 0x10)
+#define IPV6_ADDR_MC_FLAG_PREFIX(a)	\
+	((a)->s6_addr[1] & 0x20)
+#define IPV6_ADDR_MC_FLAG_RENDEZVOUS(a)	\
+	((a)->s6_addr[1] & 0x40)
+#endif
+
+/*
  *	fragmentation header
  */
 
-- 
1.7.2.3

[PATCH 5/5] bridge: Allow mcast snooping for transient link local addresses too

[Linus Lüssing]

Currently the multicast bridge snooping support is not active for
link local multicast. I assume this has been done to leave
important multicast data untouched, like IPv6 Neighborhood Discovery.

In larger, bridged, local networks it could however be desirable to
optimize for instance local multicast audio/video streaming too.

With the transient flag in IPv6 multicast addresses we have an easy
way to optimize such multimedia traffic without tempering with the
high priority multicast data from well-known addresses.

This patch alters the multicast bridge snooping for IPv6, to take
effect for transient multicast addresses instead of non-link-local
addresses.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 net/bridge/br_multicast.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index e8fdaab..b5eb28a 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -37,10 +37,9 @@
 	rcu_dereference_protected(X, lockdep_is_held(&br->multicast_lock))
 
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
-static inline int ipv6_is_local_multicast(const struct in6_addr *addr)
+static inline int ipv6_is_transient_multicast(const struct in6_addr *addr)
 {
-	if (ipv6_addr_is_multicast(addr) &&
-	    IPV6_ADDR_MC_SCOPE(addr) <= IPV6_ADDR_SCOPE_LINKLOCAL)
+	if (ipv6_addr_is_multicast(addr) && IPV6_ADDR_MC_FLAG_TRANSIENT(addr))
 		return 1;
 	return 0;
 }
@@ -781,7 +780,7 @@ static int br_ip6_multicast_add_group(struct net_bridge *br,
 {
 	struct br_ip br_group;
 
-	if (ipv6_is_local_multicast(group))
+	if (!ipv6_is_transient_multicast(group))
 		return 0;
 
 	ipv6_addr_copy(&br_group.u.ip6, group);
@@ -1342,7 +1341,7 @@ static void br_ip6_multicast_leave_group(struct net_bridge *br,
 {
 	struct br_ip br_group;
 
-	if (ipv6_is_local_multicast(group))
+	if (!ipv6_is_transient_multicast(group))
 		return;
 
 	ipv6_addr_copy(&br_group.u.ip6, group);
-- 
1.7.2.3

Re: Multicast snooping fixes and suggestions

[Stephen Hemminger]

On Wed, 16 Feb 2011 00:19:16 +0100
Linus Lüssing <linus.luessing@web.de> wrote:

> Hello everyone,
> 
> While testing the (very awesome!) bridge igmp/mld snooping support I came across
> two issues which are breaking IPv6 multicast snooping and IPv6
> non-link-local multicast on bridges with multicast snooping support enabled
> in general. The first two patches shall fix these issues.
> 
> The third one addresses a potential bug on little endian machines which I noticed
> during this little code reviewing. This patch is untested though, feedback welcome.
> 
> The fourth and fifth patch are a suggestion to also permit using the bridge multicast
> snooping feature for link local multimedia multicast traffic. Therefore
> using the transient multicast flag instead of the non-link-local scope criteria
> seems to be a suitable solution at least for IPv6, in my opinion. Let me know what
> you think about it.
> 
> Thanks for reviewing these patches.
> 
> Cheers,
> Linus

These look correct. Did you test them with real traffic?

Re: Multicast snooping fixes and suggestions

[Linus Lüssing]

> These look correct. Did you test them with real traffic?

Yes, I did. With these patches the hashlist and linked lists per port
are being filled correctly for IPv6 - initially. Verified that with
both some printk()s for the per port mglists as well as with vlc. With
patch 5/5 this also worked fine with transient link local addresses,
verified that with 'vlc -vvv "udp://@[ff12::123%eth1]"' on a device
connected to the other one with the bridge and could stream
a video as expected with no multicast traffic on any other bridge port.

However, the MLD queries are/were still broken, the queries initiated
by the bridge device do not get a response from the multicast listeners.
The following additional, attached patches fix this issue.


Last but not least, there are still a couple of bugs I could observe:
- I have attached a laptop with two interfaces with a multicast listener
  each to another PC playing with the bridge device. With the fixes
  below, the laptop sends a multicast listener report to the other PC
  on each interface, however these reports' IPv6 header's source addresses
  seem to be a random one from any of the laptop's two interfaces'
  link local addresses (which has to be a bug in the IPv6 code, as
  this one is generating the reports and not the bridge code) as long
  as it matches the selected multicast address (which was ff12::123 in
  this case).
- If there is no multicast listener present, then the multicast packets
  get flooded on all bridge ports.

And two issues with a little lower priority, I suppose:
- Packets do not get delivered to the bridge interface itself when
  a multicast listener has been started on this bridge interface
  (might be related to http://www.spinics.net/lists/linux-net/msg17556.html,
  so possibly a bug in the IPv6 code again).
~ Quitting of a multicast listener with a MLDv2 message is interpreted as
  a join, resulting in relatively long timeouts - but this MLDv1
  interpretation of MLDv2 messages seems to be intended so far due to its
  simplicity according to the comment in the code.

Cheers, Linus

[PATCH 1/2] bridge: Fix MLD queries' ethernet source address

[Linus Lüssing]

Map the IPv6 header's destination multicast address to an ethernet
source address instead of the MLD queries multicast address.

For instance for a general MLD query (multicast address in the MLD query
set to ::), this would wrongly be mapped to 33:33:00:00:00:00, although
an MLD queries destination MAC should always be 33:33:00:00:00:01 which
matches the IPv6 header's multicast destination ff02::1.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 net/bridge/br_multicast.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index b5eb28a..f904a2e 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -435,7 +435,6 @@ static struct sk_buff *br_ip6_multicast_alloc_query(struct net_bridge *br,
 	eth = eth_hdr(skb);
 
 	memcpy(eth->h_source, br->dev->dev_addr, 6);
-	ipv6_eth_mc_map(group, eth->h_dest);
 	eth->h_proto = htons(ETH_P_IPV6);
 	skb_put(skb, sizeof(*eth));
 
@@ -449,6 +448,7 @@ static struct sk_buff *br_ip6_multicast_alloc_query(struct net_bridge *br,
 	ip6h->hop_limit = 1;
 	ipv6_addr_set(&ip6h->saddr, 0, 0, 0, 0);
 	ipv6_addr_set(&ip6h->daddr, htonl(0xff020000), 0, 0, htonl(1));
+	ipv6_eth_mc_map(&ip6h->daddr, eth->h_dest);
 
 	hopopt = (u8 *)(ip6h + 1);
 	hopopt[0] = IPPROTO_ICMPV6;		/* next hdr */
-- 
1.7.2.3

[PATCH 2/2] bridge: Use IPv6 link-local address for multicast listener queries

[Linus Lüssing]

Currently the bridge multicast snooping feature periodically issues
IPv6 general multicast listener queries to sense the absence of a
listener.

For this, it uses :: as its source address - however RFC 2710 requires:
"To be valid, the Query message MUST come from a link-local IPv6 Source
Address". Current Linux kernel versions seem to follow this requirement
and ignore our bogus MLD queries.

With this commit a link local address from the bridge interface is being
used to issue the MLD query, resulting in other Linux devices which are
multicast listeners in the network to respond with a MLD response (which
was not the case before).

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 net/bridge/br_multicast.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index f904a2e..2d88861 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -446,7 +446,8 @@ static struct sk_buff *br_ip6_multicast_alloc_query(struct net_bridge *br,
 	ip6h->payload_len = htons(8 + sizeof(*mldq));
 	ip6h->nexthdr = IPPROTO_HOPOPTS;
 	ip6h->hop_limit = 1;
-	ipv6_addr_set(&ip6h->saddr, 0, 0, 0, 0);
+	ipv6_dev_get_saddr(dev_net(br->dev), br->dev, &ip6h->daddr, 0,
+			   &ip6h->saddr);
 	ipv6_addr_set(&ip6h->daddr, htonl(0xff020000), 0, 0, htonl(1));
 	ipv6_eth_mc_map(&ip6h->daddr, eth->h_dest);
 
-- 
1.7.2.3

Re: Multicast snooping fixes and suggestions

[David Miller]

From: Linus Lüssing <linus.luessing@web.de>
Date: Wed, 16 Feb 2011 00:19:16 +0100

> While testing the (very awesome!) bridge igmp/mld snooping support I came across
> two issues which are breaking IPv6 multicast snooping and IPv6
> non-link-local multicast on bridges with multicast snooping support enabled
> in general. The first two patches shall fix these issues.
> 
> The third one addresses a potential bug on little endian machines which I noticed
> during this little code reviewing. This patch is untested though, feedback welcome.
> 
> The fourth and fifth patch are a suggestion to also permit using the bridge multicast
> snooping feature for link local multimedia multicast traffic. Therefore
> using the transient multicast flag instead of the non-link-local scope criteria
> seems to be a suitable solution at least for IPv6, in my opinion. Let me know what
> you think about it.
> 
> Thanks for reviewing these patches.

Nice work, all applied to net-2.6, thanks!

Re: [PATCH 1/2] bridge: Fix MLD queries' ethernet source address

[David Miller]

From: Linus Lüssing <linus.luessing@web.de>
Date: Thu, 17 Feb 2011 19:17:51 +0100

> Map the IPv6 header's destination multicast address to an ethernet
> source address instead of the MLD queries multicast address.
> 
> For instance for a general MLD query (multicast address in the MLD query
> set to ::), this would wrongly be mapped to 33:33:00:00:00:00, although
> an MLD queries destination MAC should always be 33:33:00:00:00:01 which
> matches the IPv6 header's multicast destination ff02::1.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>

Applied.

Re: [PATCH 2/2] bridge: Use IPv6 link-local address for multicast listener queries

[David Miller]

From: Linus Lüssing <linus.luessing@web.de>
Date: Thu, 17 Feb 2011 19:17:52 +0100

> Currently the bridge multicast snooping feature periodically issues
> IPv6 general multicast listener queries to sense the absence of a
> listener.
> 
> For this, it uses :: as its source address - however RFC 2710 requires:
> "To be valid, the Query message MUST come from a link-local IPv6 Source
> Address". Current Linux kernel versions seem to follow this requirement
> and ignore our bogus MLD queries.
> 
> With this commit a link local address from the bridge interface is being
> used to issue the MLD query, resulting in other Linux devices which are
> multicast listeners in the network to respond with a MLD response (which
> was not the case before).
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>

Applied.

Re: Multicast snooping fixes and suggestions

[H. Peter Anvin]

On 02/15/2011 03:19 PM, Linus Lüssing wrote:
> Hello everyone,
> 
> While testing the (very awesome!) bridge igmp/mld snooping support I came across
> two issues which are breaking IPv6 multicast snooping and IPv6
> non-link-local multicast on bridges with multicast snooping support enabled
> in general. The first two patches shall fix these issues.
> 
> The third one addresses a potential bug on little endian machines which I noticed
> during this little code reviewing. This patch is untested though, feedback welcome.
> 
> The fourth and fifth patch are a suggestion to also permit using the bridge multicast
> snooping feature for link local multimedia multicast traffic. Therefore
> using the transient multicast flag instead of the non-link-local scope criteria
> seems to be a suitable solution at least for IPv6, in my opinion. Let me know what
> you think about it.
> 

Hello,

I have just noticed that when using a Linux bridge, IPv6 often fails to
configure until some considerable time has passed, presumably some kind
of retry timer.  The dmesg shows:

[178292.449300] br0: port 1(eth0) entering learning state
[178292.449304] br0: port 1(eth0) entering learning state
[178302.536098] br0: no IPv6 routers present
[178307.416139] br0: port 1(eth0) entering forwarding state

... even though there is a configured and active IPv6 router on the network.

I have also seen some serious delays with DHCPv4 which presumably is due
to lost packets during bridge learning.

Are these packets likely to address that situation (or am I just plain
doing something stupid)?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Multicast snooping fixes and suggestions

[Herbert Xu]

On Wed, Feb 23, 2011 at 07:16:17PM -0800, H. Peter Anvin wrote:
>
> Are these packets likely to address that situation (or am I just plain
> doing something stupid)?

No these patches deal with IPv6 multicast support in the bridge and
are not related to your problem.

If you're using the bridge for virtualisation and you know that
you don't have a loop in your setup, then one option would be to
disable STP on your bridge.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Multicast snooping fixes and suggestions

[H. Peter Anvin]

On 02/23/2011 08:04 PM, Herbert Xu wrote:
> On Wed, Feb 23, 2011 at 07:16:17PM -0800, H. Peter Anvin wrote:
>>
>> Are these packets likely to address that situation (or am I just plain
>> doing something stupid)?
> 
> No these patches deal with IPv6 multicast support in the bridge and
> are not related to your problem.
> 
> If you're using the bridge for virtualisation and you know that
> you don't have a loop in your setup, then one option would be to
> disable STP on your bridge.

Hi,

I have disabled STP on the bridge; it doesn't change the behavior.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Multicast snooping fixes and suggestions

[Herbert Xu]

On Wed, Feb 23, 2011 at 08:46:44PM -0800, H. Peter Anvin wrote:
>
> I have disabled STP on the bridge; it doesn't change the behavior.

Once you have disabled STP, you can set the forward delay to 0
which should do the trick.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Multicast snooping fixes and suggestions

[H. Peter Anvin]

[-- Attachment #1.1: Type: text/plain, Size: 699 bytes --]

Ok, so stupid question... how do hardware switches deal with this? It would seem to me that if everyone behind say a Cisco switch had these kind of issues they would have limited appeal...
-- 
Sent from my mobile phone. Please pardon any lack of formatting.

Herbert Xu <herbert@gondor.hengli.com.au> wrote:

On Wed, Feb 23, 2011 at 08:46:44PM -0800, H. Peter Anvin wrote: > > I have disabled STP on the bridge; it doesn't change the behavior. Once you have disabled STP, you can set the forward delay to 0 which should do the trick. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt 


[-- Attachment #1.2: Type: text/html, Size: 1121 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Bridge mailing list
Bridge@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/bridge

Re: Multicast snooping fixes and suggestions

[Stephen Hemminger]

On Wed, 23 Feb 2011 21:57:32 -0800
"H. Peter Anvin" <hpa@zytor.com> wrote:

> Ok, so stupid question... how do hardware switches deal with this? It would seem to me that if everyone behind say a Cisco switch had these kind of issues they would have limited appeal...

Real bridges run current newwer Spanning Tree Protocol that converges faster.
The current Linux STP code is on older standard (around 2001). The current STP
standard use RSTP which converges much faster.
  http://en.wikipedia.org/wiki/Spanning_Tree_Protocol

There is a userspace RSTP daemon that almost nobody uses.

There are a number of other STP enhancements that are needed like
STP protection and MSTP, as welll as the Cisco non-standard STP VLAN stuff.

Fixing STP and testing it is a fairly project, too big for a spare time
effort and currently not something high enough on the project chart for me to
be able to dedicate much company time on. Contributions welcome.