Multicast snooping fixes and suggestions

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

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.

---
 net/bridge/br_multicast.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index f701a21..135d929 100644
@@ -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

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.

---
 net/bridge/br_multicast.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 135d929..45dcf10 100644
@@ -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

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

---
 net/bridge/br_multicast.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

index 45dcf10..e8fdaab 100644
@@ -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

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.

---
 include/net/ipv6.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

index 4a3cd2c..96e50e0 100644
@@ -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

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.

---
 net/bridge/br_multicast.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

index e8fdaab..b5eb28a 100644
@@ -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

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

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.

---
 net/bridge/br_multicast.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index b5eb28a..f904a2e 100644
@@ -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

From: Linus Lüssing <[email protected]>
Date: Thu, 17 Feb 2011 19:17:51 +0100

Applied.

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).

---
 net/bridge/br_multicast.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

index f904a2e..2d88861 100644
@@ -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

From: Linus Lüssing <[email protected]>
Date: Thu, 17 Feb 2011 19:17:52 +0100

Applied.

From: Linus Lüssing <[email protected]>
Date: Wed, 16 Feb 2011 00:19:16 +0100

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

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.

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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.

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

Cheers,

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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 <[email protected]> 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 <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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.