[0/3] bridge: Do not send multicast queries by default

[0/3] bridge: Do not send multicast queries by default

[Herbert Xu]

Hi:

This series of patches is aimed to change the default multicast
snooping behaviour to one that is safer to deploy in the wild.

There have been numerous reports of switches misbehaving with
our current behaviour of sending general queries, presumably
because we're using a zero source IP address which is unavoidable
as using anything else would interfere with multicast querier
elections (incidentally, I noticed that our IPv6 code has been
"fixed" to not use zero source addresses, which is wrong as we
may end up being THE MLD querier in a network).

Since our queries aren't actually required for multicast snooping
to function, but is merely an optimisation mostly for faster
start-up convergence, I think we should disable this by default.

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

[PATCH 2/3] bridge: Restart queries when last querier expires

[Herbert Xu]

bridge: Restart queries when last querier expires

As it stands when we discover that a real querier (one that queries
with a non-zero source address) we stop querying.  However, even
after said querier has fallen off the edge of the earth, we will
never restart querying (unless the bridge itself is restarted).

This patch fixes this by kicking our own querier into gear when
the timer for other queriers expire.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 net/bridge/br_multicast.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index dca0c66..b0796d7 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -36,6 +36,8 @@
 #define mlock_dereference(X, br) \
 	rcu_dereference_protected(X, lockdep_is_held(&br->multicast_lock))
 
+static void br_multicast_start_querier(struct net_bridge *br);
+
 #if IS_ENABLED(CONFIG_IPV6)
 static inline int ipv6_is_transient_multicast(const struct in6_addr *addr)
 {
@@ -740,6 +742,21 @@ static void br_multicast_local_router_expired(unsigned long data)
 {
 }
 
+static void br_multicast_querier_expired(unsigned long data)
+{
+	struct net_bridge_port *port = (void *)data;
+	struct net_bridge *br = port->br;
+
+	spin_lock(&br->multicast_lock);
+	if (!netif_running(br->dev) || br->multicast_disabled)
+		goto out;
+
+	br_multicast_start_querier(br);
+
+out:
+	spin_unlock(&br->multicast_lock);
+}
+
 static void __br_multicast_send_query(struct net_bridge *br,
 				      struct net_bridge_port *port,
 				      struct br_ip *ip)
@@ -1562,7 +1579,7 @@ void br_multicast_init(struct net_bridge *br)
 	setup_timer(&br->multicast_router_timer,
 		    br_multicast_local_router_expired, 0);
 	setup_timer(&br->multicast_querier_timer,
-		    br_multicast_local_router_expired, 0);
+		    br_multicast_querier_expired, 0);
 	setup_timer(&br->multicast_query_timer, br_multicast_query_expired,
 		    (unsigned long)br);
 }

[PATCH 1/3] bridge: Add br_multicast_start_querier

[Herbert Xu]

bridge: Add br_multicast_start_querier

This patch adds the helper br_multicast_start_querier so that
the code which starts the queriers in br_multicast_toggle can
be reused elsewhere.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 net/bridge/br_multicast.c |   25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 27ca25e..dca0c66 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1689,9 +1689,23 @@ unlock:
 	return err;
 }
 
-int br_multicast_toggle(struct net_bridge *br, unsigned long val)
+static void br_multicast_start_querier(struct net_bridge *br)
 {
 	struct net_bridge_port *port;
+
+	br_multicast_open(br);
+
+	list_for_each_entry(port, &br->port_list, list) {
+		if (port->state == BR_STATE_DISABLED ||
+		    port->state == BR_STATE_BLOCKING)
+			continue;
+
+		__br_multicast_enable_port(port);
+	}
+}
+
+int br_multicast_toggle(struct net_bridge *br, unsigned long val)
+{
 	int err = 0;
 	struct net_bridge_mdb_htable *mdb;
 
@@ -1721,14 +1735,7 @@ rollback:
 			goto rollback;
 	}
 
-	br_multicast_open(br);
-	list_for_each_entry(port, &br->port_list, list) {
-		if (port->state == BR_STATE_DISABLED ||
-		    port->state == BR_STATE_BLOCKING)
-			continue;
-
-		__br_multicast_enable_port(port);
-	}
+	br_multicast_start_querier(br);
 
 unlock:
 	spin_unlock_bh(&br->multicast_lock);

[PATCH 3/3] bridge: Add multicast_querier toggle and disable queries by default

[Herbert Xu]

bridge: Add multicast_querier toggle and disable queries by default

Sending general queries was implemented as an optimisation to speed
up convergence on start-up.  In order to prevent interference with
multicast routers a zero source address has to be used.

Unfortunately these packets appear to cause some multicast-aware
switches to misbehave, e.g., by disrupting multicast packets to us.

Since the multicast snooping feature still functions without sending
our own queries, this patch will change the default to not send
queries.

For those that need queries in order to speed up convergence on start-up,
a toggle is provided to restore the previous behaviour.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 net/bridge/br_multicast.c |   20 ++++++++++++++++++++
 net/bridge/br_private.h   |    2 ++
 net/bridge/br_sysfs_br.c  |   18 ++++++++++++++++++
 3 files changed, 40 insertions(+)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index b0796d7..26df642 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -783,6 +783,7 @@ static void br_multicast_send_query(struct net_bridge *br,
 	struct br_ip br_group;
 
 	if (!netif_running(br->dev) || br->multicast_disabled ||
+	    !br->multicast_querier ||
 	    timer_pending(&br->multicast_querier_timer))
 		return;
 
@@ -1565,6 +1566,7 @@ void br_multicast_init(struct net_bridge *br)
 	br->hash_max = 512;
 
 	br->multicast_router = 1;
+	br->multicast_querier = 0;
 	br->multicast_last_member_count = 2;
 	br->multicast_startup_query_count = 2;
 
@@ -1760,6 +1762,24 @@ unlock:
 	return err;
 }
 
+int br_multicast_set_querier(struct net_bridge *br, unsigned long val)
+{
+	val = !!val;
+
+	spin_lock_bh(&br->multicast_lock);
+	if (br->multicast_querier == val)
+		goto unlock;
+
+	br->multicast_querier = val;
+	if (val)
+		br_multicast_start_querier(br);
+
+unlock:
+	spin_unlock_bh(&br->multicast_lock);
+
+	return 0;
+}
+
 int br_multicast_set_hash_max(struct net_bridge *br, unsigned long val)
 {
 	int err = -ENOENT;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index e1d8822..f8ffd8c 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -224,6 +224,7 @@ struct net_bridge
 	unsigned char			multicast_router;
 
 	u8				multicast_disabled:1;
+	u8				multicast_querier:1;
 
 	u32				hash_elasticity;
 	u32				hash_max;
@@ -417,6 +418,7 @@ extern int br_multicast_set_router(struct net_bridge *br, unsigned long val);
 extern int br_multicast_set_port_router(struct net_bridge_port *p,
 					unsigned long val);
 extern int br_multicast_toggle(struct net_bridge *br, unsigned long val);
+extern int br_multicast_set_querier(struct net_bridge *br, unsigned long val);
 extern int br_multicast_set_hash_max(struct net_bridge *br, unsigned long val);
 
 static inline bool br_multicast_is_router(struct net_bridge *br)
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index c236c0e..a27243e 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -379,6 +379,23 @@ static ssize_t store_multicast_snooping(struct device *d,
 static DEVICE_ATTR(multicast_snooping, S_IRUGO | S_IWUSR,
 		   show_multicast_snooping, store_multicast_snooping);
 
+static ssize_t show_multicast_querier(struct device *d,
+				      struct device_attribute *attr,
+				      char *buf)
+{
+	struct net_bridge *br = to_bridge(d);
+	return sprintf(buf, "%d\n", br->multicast_querier);
+}
+
+static ssize_t store_multicast_querier(struct device *d,
+				       struct device_attribute *attr,
+				       const char *buf, size_t len)
+{
+	return store_bridge_parm(d, buf, len, br_multicast_set_querier);
+}
+static DEVICE_ATTR(multicast_querier, S_IRUGO | S_IWUSR,
+		   show_multicast_querier, store_multicast_querier);
+
 static ssize_t show_hash_elasticity(struct device *d,
 				    struct device_attribute *attr, char *buf)
 {
@@ -702,6 +719,7 @@ static struct attribute *bridge_attrs[] = {
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	&dev_attr_multicast_router.attr,
 	&dev_attr_multicast_snooping.attr,
+	&dev_attr_multicast_querier.attr,
 	&dev_attr_hash_elasticity.attr,
 	&dev_attr_hash_max.attr,
 	&dev_attr_multicast_last_member_count.attr,

Re: [0/3] bridge: Do not send multicast queries by default

[David Miller]

From: Herbert Xu <herbert@gondor.hengli.com.au>
Date: Fri, 13 Apr 2012 20:36:41 +0800

> (incidentally, I noticed that our IPv6 code has been "fixed" to not
> use zero source addresses, which is wrong as we may end up being THE
> MLD querier in a network).

I seem to recall it was explicitly changed to be this way and that
there was a good reason for this, see the history.

Re: [0/3] bridge: Do not send multicast queries by default

[Herbert Xu]

On Fri, Apr 13, 2012 at 10:53:45AM -0400, David Miller wrote:
> From: Herbert Xu <herbert@gondor.hengli.com.au>
> Date: Fri, 13 Apr 2012 20:36:41 +0800
> 
> > (incidentally, I noticed that our IPv6 code has been "fixed" to not
> > use zero source addresses, which is wrong as we may end up being THE
> > MLD querier in a network).
> 
> I seem to recall it was explicitly changed to be this way and that
> there was a good reason for this, see the history.

Right, the reason given is that RFC2710 (for MLD) requires the
source address to be a link-local address.

However, we're not implementing an RFC2710 node here.  What we're
doing is better described by RFC4541 (IGMP/MLD snooping), which calls
for the use of a zero source address for both IPv4 and IPv6.

The reason is precisely because it's invalid for normal querier
nodes and as such they would ignore us (rather than elect us
and potentially disrupt things).

Now granted we may also end up having other nodes ignoring our
queries where we'd rather that they answered us with reports.
However, this isn't as bad because the whole querying mechanism
in the snooping code is merely an optimisation to speed up
convergence primarily during start-up.  So if we don't see the
reports straight away it's not a deal-breaker.

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: [0/3] bridge: Do not send multicast queries by default

[David Miller]

From: Herbert Xu <herbert@gondor.hengli.com.au>
Date: Sun, 15 Apr 2012 19:13:00 +0800

> However, we're not implementing an RFC2710 node here.  What we're
> doing is better described by RFC4541 (IGMP/MLD snooping), which calls
> for the use of a zero source address for both IPv4 and IPv6.
> 
> The reason is precisely because it's invalid for normal querier
> nodes and as such they would ignore us (rather than elect us
> and potentially disrupt things).

Ok, make sense.

And in fact, I welcome changing this back to using a zero saddr, as
the ipv6 source address setting code brought in a lot of ugly
dependencies. :-)

Re: [0/3] bridge: Do not send multicast queries by default

[David Miller]

From: Herbert Xu <herbert@gondor.hengli.com.au>
Date: Fri, 13 Apr 2012 20:36:41 +0800

> This series of patches is aimed to change the default multicast
> snooping behaviour to one that is safer to deploy in the wild.
> 
> There have been numerous reports of switches misbehaving with
> our current behaviour of sending general queries, presumably
> because we're using a zero source IP address which is unavoidable
> as using anything else would interfere with multicast querier
> elections (incidentally, I noticed that our IPv6 code has been
> "fixed" to not use zero source addresses, which is wrong as we
> may end up being THE MLD querier in a network).
> 
> Since our queries aren't actually required for multicast snooping
> to function, but is merely an optimisation mostly for faster
> start-up convergence, I think we should disable this by default.

All applied to net-next, thanks Herbert.

Re: [0/3] bridge: Do not send multicast queries by default

[Linus Lüssing]

Herbert Xu <herbert <at> gondor.apana.org.au> writes:

> 
> On Fri, Apr 13, 2012 at 10:53:45AM -0400, David Miller wrote:
> > From: Herbert Xu <herbert <at> gondor.hengli.com.au>
> > Date: Fri, 13 Apr 2012 20:36:41 +0800
> > 
> > > (incidentally, I noticed that our IPv6 code has been "fixed" to not
> > > use zero source addresses, which is wrong as we may end up being THE
> > > MLD querier in a network).
> > 
> > I seem to recall it was explicitly changed to be this way and that
> > there was a good reason for this, see the history.
> 
> Right, the reason given is that RFC2710 (for MLD) requires the
> source address to be a link-local address.
> 
> However, we're not implementing an RFC2710 node here.  What we're
> doing is better described by RFC4541 (IGMP/MLD snooping), which calls
> for the use of a zero source address for both IPv4 and IPv6.

Hm, yes, you're right, RFC4541 mentions a zero-source address in a short
paragraph.

> 
> The reason is precisely because it's invalid for normal querier
> nodes and as such they would ignore us (rather than elect us
> and potentially disrupt things).
> 

Okay, right, RFC2710, section 6, "query received from a router with a lower IP
address" prevents that. But aren't RFC2710, section 5, "query received" and
RFC3810, section 5.1.14 preventing multicast listeners from processing our
query, too?

I'm not sure, but the _informational_ RFC4541 is not supposed to update the
general MLDv1/2 node (especially multicast listener) behaviour from the
proposed standards RFC2710/RFC3810, is it?

> Now granted we may also end up having other nodes ignoring our
> queries where we'd rather that they answered us with reports.
> However, this isn't as bad because the whole querying mechanism
> in the snooping code is merely an optimisation to speed up
> convergence primarily during start-up.  So if we don't see the
> reports straight away it's not a deal-breaker.

I'm not sure about that either whether it's just an optimization. Changing the
default setting to a disabled querier in the snooping code actually broke things
for me in a setup where no multicast router is present:

The thing is, with no querier on the link there are no periodic MLDv1 Reports /
MLDv2 Current State Reports (see RFC2710,  section 5. for instance, there's no
timer for the "Idle Listener" state; for RFC3810/MLDv2 I didn't find such a
timer either).

If creating a bridge interface after a multicast listener has sent all its
initial reports, then such a multicast listener will never be seen by our
bridge. Which either results in flooding the according multicast traffic on all
ports (because of another bug in the multicast snooping code, a bug which you've
actually fixed for IPv4 already, but not for IPv6, I think) if no other listener
joins after the bridge creation. Or results in this multicast listener not
receiving its traffic from the according IPv6 multicast address with a transient
flag if another listener joins on another port after creating the bridge.


Unfortunately if we were changing things back to sending multicast queries by
default, then this breaks things when another multicast router is present, one
which performs a proper querier protocol, urgh...

Maybe we'd need to implement a proper querier behaviour in the snooping code,
one as specified in RFC3810 for multicast routers, for instance?

And I'm also wondering what if yes the least harmful default settings would be
for the bridge multicast snooping for now.

> 
> Cheers,

Cheers, Linus