Linux Ethernet Bridging - Sep 2013 - [Bridge] bride: IPv6 multicast snooping enhancements

Hi,

Here are two, small feature changes I would like to submit to increase
the usefulness of the multicast snooping of the bridge code.

The first patch is an unaltered one I had submitted before, but since it
got no feedback I'm resubmitting it here for net-next. With the recently
added patch to disable snooping if there is no querier (b00589af + 248ba8ec05
+ 8d50af4fb), it should be a safe choice now (without these, patch 1/2 would
have introduced another potential for lost IPv6 multicast packets).

Both conceptually and also with some testing and fuzzing, I couldn't spot
any more causes for potential packet loss. And since the multicast snooping
code has now been tried by various people, I think it should be a safe
choice to apply the multicast snooping not only for IPv6 multicast packets
with a scope greater than link-local, but also for packets of exactly this
scope. The IPv6 standard mandates MLD reports for link-local multicast, too,
so we can safely snoop them as well (in contrast to IPv4 link-local).

Cheers, Linus

Currently if there is no listener for a certain group then IPv6 packets
for that group are flooded on all ports, even though there might be no
host and router interested in it on a port.

With this commit they are only forwarded to ports with a multicast
router.

Just like commit bd4265fe36 ("bridge: Only flood unregistered groups
to routers") did for IPv4, let''s do the same for IPv6 with the
same
reasoning.

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

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index bbcb435..662ba7b 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1547,8 +1547,14 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 	 *  - MLD has always Router Alert hop-by-hop option
 	 *  - But we do not support jumbrograms.
 	 */
-	if (ip6h->version != 6 ||
-	    ip6h->nexthdr != IPPROTO_HOPOPTS ||
+	if (ip6h->version != 6)
+		return 0;
+
+	/* Prevent flooding this packet if there is no listener present */
+	if (ipv6_is_transient_multicast(&ip6h->daddr))
+		BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
+
+	if (ip6h->nexthdr != IPPROTO_HOPOPTS ||
 	    ip6h->payload_len == 0)
 		return 0;
 
-- 
1.7.10.4

Currently if there is no listener for a certain group then IPv6 packets
for that group are flooded on all ports, even though there might be no
host and router interested in it on a port.

With this commit they are only forwarded to ports with a multicast
router.

Just like commit bd4265fe36 ("bridge: Only flood unregistered groups
to routers") did for IPv4, let's do the same for IPv6 with the same
reasoning.

Signed-off-by: Linus L?ssing <linus.luessing at web.de>
---
 net/bridge/br_multicast.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index bbcb435..662ba7b 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1547,8 +1547,14 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 	 *  - MLD has always Router Alert hop-by-hop option
 	 *  - But we do not support jumbrograms.
 	 */
-	if (ip6h->version != 6 ||
-	    ip6h->nexthdr != IPPROTO_HOPOPTS ||
+	if (ip6h->version != 6)
+		return 0;
+
+	/* Prevent flooding this packet if there is no listener present */
+	if (ipv6_is_transient_multicast(&ip6h->daddr))
+		BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
+
+	if (ip6h->nexthdr != IPPROTO_HOPOPTS ||
 	    ip6h->payload_len == 0)
 		return 0;
 
-- 
1.7.10.4

The multicast snooping code should have matured enough to be safely
applicable to IPv6 link-local multicast addresses (excluding the
link-local all nodes address, ff02::1), too.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 net/bridge/br_mdb.c       |    3 ++-
 net/bridge/br_multicast.c |    7 ++++---
 net/bridge/br_private.h   |   10 ----------
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index a7c6cd0..85a09bb 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -9,6 +9,7 @@
 #include <net/netlink.h>
 #if IS_ENABLED(CONFIG_IPV6)
 #include <net/ipv6.h>
+#include <net/addrconf.h>
 #endif
 
 #include "br_private.h"
@@ -254,7 +255,7 @@ static bool is_valid_mdb_entry(struct br_mdb_entry *entry)
 			return false;
 #if IS_ENABLED(CONFIG_IPV6)
 	} else if (entry->addr.proto == htons(ETH_P_IPV6)) {
-		if (!ipv6_is_transient_multicast(&entry->addr.u.ip6))
+		if (ipv6_addr_is_ll_all_nodes(&entry->addr.u.ip6))
 			return false;
 #endif
 	} else
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 662ba7b..3b0ed99 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -29,6 +29,7 @@
 #include <net/ipv6.h>
 #include <net/mld.h>
 #include <net/ip6_checksum.h>
+#include <net/addrconf.h>
 #endif
 
 #include "br_private.h"
@@ -724,7 +725,7 @@ static int br_ip6_multicast_add_group(struct net_bridge *br,
 {
 	struct br_ip br_group;
 
-	if (!ipv6_is_transient_multicast(group))
+	if (ipv6_addr_is_ll_all_nodes(group))
 		return 0;
 
 	br_group.u.ip6 = *group;
@@ -1410,7 +1411,7 @@ static void br_ip6_multicast_leave_group(struct net_bridge
*br,
 						  &br->ip6_query;
 
 
-	if (!ipv6_is_transient_multicast(group))
+	if (ipv6_addr_is_ll_all_nodes(group))
 		return;
 
 	br_group.u.ip6 = *group;
@@ -1551,7 +1552,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 		return 0;
 
 	/* Prevent flooding this packet if there is no listener present */
-	if (ipv6_is_transient_multicast(&ip6h->daddr))
+	if (!ipv6_addr_is_ll_all_nodes(&ip6h->daddr))
 		BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
 
 	if (ip6h->nexthdr != IPPROTO_HOPOPTS ||
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index f225fb6..598cb0b 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -494,16 +494,6 @@ extern void br_mdb_notify(struct net_device *dev, struct
net_bridge_port *port,
 #define mlock_dereference(X, br) \
 	rcu_dereference_protected(X, lockdep_is_held(&br->multicast_lock))
 
-#if IS_ENABLED(CONFIG_IPV6)
-#include <net/addrconf.h>
-static inline int ipv6_is_transient_multicast(const struct in6_addr *addr)
-{
-	if (ipv6_addr_is_multicast(addr) && IPV6_ADDR_MC_FLAG_TRANSIENT(addr))
-		return 1;
-	return 0;
-}
-#endif
-
 static inline bool br_multicast_is_router(struct net_bridge *br)
 {
 	return br->multicast_router == 2 ||
-- 
1.7.10.4

The multicast snooping code should have matured enough to be safely
applicable to IPv6 link-local multicast addresses (excluding the
link-local all nodes address, ff02::1), too.

Signed-off-by: Linus L?ssing <linus.luessing at web.de>
---
 net/bridge/br_mdb.c       |    3 ++-
 net/bridge/br_multicast.c |    7 ++++---
 net/bridge/br_private.h   |   10 ----------
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index a7c6cd0..85a09bb 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -9,6 +9,7 @@
 #include <net/netlink.h>
 #if IS_ENABLED(CONFIG_IPV6)
 #include <net/ipv6.h>
+#include <net/addrconf.h>
 #endif
 
 #include "br_private.h"
@@ -254,7 +255,7 @@ static bool is_valid_mdb_entry(struct br_mdb_entry *entry)
 			return false;
 #if IS_ENABLED(CONFIG_IPV6)
 	} else if (entry->addr.proto == htons(ETH_P_IPV6)) {
-		if (!ipv6_is_transient_multicast(&entry->addr.u.ip6))
+		if (ipv6_addr_is_ll_all_nodes(&entry->addr.u.ip6))
 			return false;
 #endif
 	} else
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 662ba7b..3b0ed99 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -29,6 +29,7 @@
 #include <net/ipv6.h>
 #include <net/mld.h>
 #include <net/ip6_checksum.h>
+#include <net/addrconf.h>
 #endif
 
 #include "br_private.h"
@@ -724,7 +725,7 @@ static int br_ip6_multicast_add_group(struct net_bridge *br,
 {
 	struct br_ip br_group;
 
-	if (!ipv6_is_transient_multicast(group))
+	if (ipv6_addr_is_ll_all_nodes(group))
 		return 0;
 
 	br_group.u.ip6 = *group;
@@ -1410,7 +1411,7 @@ static void br_ip6_multicast_leave_group(struct net_bridge
*br,
 						  &br->ip6_query;
 
 
-	if (!ipv6_is_transient_multicast(group))
+	if (ipv6_addr_is_ll_all_nodes(group))
 		return;
 
 	br_group.u.ip6 = *group;
@@ -1551,7 +1552,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 		return 0;
 
 	/* Prevent flooding this packet if there is no listener present */
-	if (ipv6_is_transient_multicast(&ip6h->daddr))
+	if (!ipv6_addr_is_ll_all_nodes(&ip6h->daddr))
 		BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
 
 	if (ip6h->nexthdr != IPPROTO_HOPOPTS ||
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index f225fb6..598cb0b 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -494,16 +494,6 @@ extern void br_mdb_notify(struct net_device *dev, struct
net_bridge_port *port,
 #define mlock_dereference(X, br) \
 	rcu_dereference_protected(X, lockdep_is_held(&br->multicast_lock))
 
-#if IS_ENABLED(CONFIG_IPV6)
-#include <net/addrconf.h>
-static inline int ipv6_is_transient_multicast(const struct in6_addr *addr)
-{
-	if (ipv6_addr_is_multicast(addr) && IPV6_ADDR_MC_FLAG_TRANSIENT(addr))
-		return 1;
-	return 0;
-}
-#endif
-
 static inline bool br_multicast_is_router(struct net_bridge *br)
 {
 	return br->multicast_router == 2 ||
-- 
1.7.10.4

From: Linus L?ssing <linus.luessing at web.de>
Date: Wed,  4 Sep 2013 02:13:37 +0200
> Here are two, small feature changes I would like to submit to increase
> the usefulness of the multicast snooping of the bridge code.
> 
> The first patch is an unaltered one I had submitted before, but since it
> got no feedback I'm resubmitting it here for net-next. With the
recently
> added patch to disable snooping if there is no querier (b00589af +
248ba8ec05
> + 8d50af4fb), it should be a safe choice now (without these, patch 1/2
would
> have introduced another potential for lost IPv6 multicast packets).
> 
> Both conceptually and also with some testing and fuzzing, I couldn't
spot
> any more causes for potential packet loss. And since the multicast snooping
> code has now been tried by various people, I think it should be a safe
> choice to apply the multicast snooping not only for IPv6 multicast packets
> with a scope greater than link-local, but also for packets of exactly this
> scope. The IPv6 standard mandates MLD reports for link-local multicast,
too,
> so we can safely snoop them as well (in contrast to IPv4 link-local).
Both patches applied, thanks!

This patch prevent forwarding of ICMPv6 in bridges,
so containers/VMs with virtual eth adapters connected in local bridge cannot
ping each other via ipv6 (but can do it via ipv4)

Could you please clarify, is it expected behavior?
Do we need to enable multicast routing or multicast_snooping on all local ports
on such bridges to enable just ICMPv6?
I believe ICMPv6 is an exception and should not be filtered by multicast
spoofing.

Thank you,
	Vasily Averin

On 04.09.2013 04:13, Linus L?ssing wrote:
> Hi,
> 
> Here are two, small feature changes I would like to submit to increase
> the usefulness of the multicast snooping of the bridge code.
> 
> The first patch is an unaltered one I had submitted before, but since it
> got no feedback I'm resubmitting it here for net-next. With the
recently
> added patch to disable snooping if there is no querier (b00589af +
248ba8ec05
> + 8d50af4fb), it should be a safe choice now (without these, patch 1/2
would
> have introduced another potential for lost IPv6 multicast packets).
> 
> Both conceptually and also with some testing and fuzzing, I couldn't
spot
> any more causes for potential packet loss. And since the multicast snooping
> code has now been tried by various people, I think it should be a safe
> choice to apply the multicast snooping not only for IPv6 multicast packets
> with a scope greater than link-local, but also for packets of exactly this
> scope. The IPv6 standard mandates MLD reports for link-local multicast,
too,
> so we can safely snoop them as well (in contrast to IPv4 link-local).
> 
> Cheers, Linus
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 
>