Linux Ethernet Bridging - Feb 2011 - [Bridge] 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.

Signed-off-by: Linus L?ssing <linus.luessing at 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

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 at 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

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

Signed-off-by: Linus L?ssing <linus.luessing at 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

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 at 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

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 at 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

On Wed, 16 Feb 2011 00:19:16 +0100
Linus L?ssing <linus.luessing at 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?

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 at 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

From: Linus L?ssing <linus.luessing at 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!

From: Linus L?ssing <linus.luessing at 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 at web.de>
Applied.

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.

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