[Patch net-next 1/3] bridge: use the bridge IP addr as source addr of querier

[Patch net-next 1/3] bridge: use the bridge IP addr as source addr of querier

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

Quote from Adam:
"If it is believed that the use of 0.0.0.0
as the IP address is what is causing strange behaviour on other devices
then is there a good reason that a bridge rather than a router shouldn't
be the active querier? If not then using the bridge IP address and
having the querier enabled by default may be a reasonable solution
(provided that our querier obeys the election rules and shuts up if it
sees a query from a lower IP address that isn't 0.0.0.0). Just because a
device is the elected querier for IGMP doesn't appear to mean it is
required to perform any other routing functions."

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Adam Baker <linux@baker-net.org.uk>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 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 81f2389..6cb937c 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -23,6 +23,7 @@
 #include <linux/skbuff.h>
 #include <linux/slab.h>
 #include <linux/timer.h>
+#include <linux/inetdevice.h>
 #include <net/ip.h>
 #if IS_ENABLED(CONFIG_IPV6)
 #include <net/ipv6.h>
@@ -381,7 +382,7 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
 	iph->frag_off = htons(IP_DF);
 	iph->ttl = 1;
 	iph->protocol = IPPROTO_IGMP;
-	iph->saddr = 0;
+	iph->saddr = inet_select_addr(br->dev, 0, RT_SCOPE_LINK);
 	iph->daddr = htonl(INADDR_ALLHOSTS_GROUP);
 	((u8 *)&iph[1])[0] = IPOPT_RA;
 	((u8 *)&iph[1])[1] = 4;
-- 
1.7.7.6

[Patch net-next 2/3] bridge: only expire the mdb entry when query is received

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

Currently we arm the expire timer when the mdb entry is added,
however, this causes problem when there is no querier sent
out after that.

So we should only arm the timer when a corresponding query is
received, as suggested by Herbert.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Adam Baker <linux@baker-net.org.uk>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 net/bridge/br_multicast.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 6cb937c..a821a4e 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -616,8 +616,6 @@ rehash:
 
 	mp->br = br;
 	mp->addr = *group;
-	setup_timer(&mp->timer, br_multicast_group_expired,
-		    (unsigned long)mp);
 
 	hlist_add_head_rcu(&mp->hlist[mdb->ver], &mdb->mhash[hash]);
 	mdb->size++;
@@ -655,7 +653,6 @@ static int br_multicast_add_group(struct net_bridge *br,
 	struct net_bridge_mdb_entry *mp;
 	struct net_bridge_port_group *p;
 	struct net_bridge_port_group __rcu **pp;
-	unsigned long now = jiffies;
 	int err;
 
 	spin_lock(&br->multicast_lock);
@@ -670,7 +667,6 @@ static int br_multicast_add_group(struct net_bridge *br,
 
 	if (!port) {
 		mp->mglist = true;
-		mod_timer(&mp->timer, now + br->multicast_membership_interval);
 		goto out;
 	}
 
@@ -678,7 +674,7 @@ static int br_multicast_add_group(struct net_bridge *br,
 	     (p = mlock_dereference(*pp, br)) != NULL;
 	     pp = &p->next) {
 		if (p->port == port)
-			goto found;
+			goto out;
 		if ((unsigned long)p->port < (unsigned long)port)
 			break;
 	}
@@ -689,8 +685,6 @@ static int br_multicast_add_group(struct net_bridge *br,
 	rcu_assign_pointer(*pp, p);
 	br_mdb_notify(br->dev, port, group, RTM_NEWMDB);
 
-found:
-	mod_timer(&p->timer, now + br->multicast_membership_interval);
 out:
 	err = 0;
 
@@ -1130,6 +1124,9 @@ static int br_ip4_multicast_query(struct net_bridge *br,
 	if (!mp)
 		goto out;
 
+	setup_timer(&mp->timer, br_multicast_group_expired, (unsigned long)mp);
+	mod_timer(&mp->timer, now + br->multicast_membership_interval);
+
 	max_delay *= br->multicast_last_member_count;
 
 	if (mp->mglist &&
@@ -1204,6 +1201,8 @@ static int br_ip6_multicast_query(struct net_bridge *br,
 	if (!mp)
 		goto out;
 
+	setup_timer(&mp->timer, br_multicast_group_expired, (unsigned long)mp);
+	mod_timer(&mp->timer, now + br->multicast_membership_interval);
 	max_delay *= br->multicast_last_member_count;
 	if (mp->mglist &&
 	    (timer_pending(&mp->timer) ?
-- 
1.7.7.6

Re: [Patch net-next 2/3] bridge: only expire the mdb entry when query is received

[Cong Wang]

On Mon, 2013-04-29 at 15:26 +0800, Cong Wang wrote:
> From: Cong Wang <amwang@redhat.com>
> 
> Currently we arm the expire timer when the mdb entry is added,
> however, this causes problem when there is no querier sent
> out after that.
> 
> So we should only arm the timer when a corresponding query is
> received, as suggested by Herbert.

One problem with this solution is that the temp mdb entry will no longer
be expired automatically any more, since no query no timer.

I am wondering if we should find other way to fix it, for example,
touching the timer as long as there is multicast traffic (non-IGMP).

Thanks!

Re: [Patch net-next 2/3] bridge: only expire the mdb entry when query is received

[Herbert Xu]

On Tue, Apr 30, 2013 at 11:17:37AM +0800, Cong Wang wrote:
> 
> One problem with this solution is that the temp mdb entry will no longer
> be expired automatically any more, since no query no timer.
> 
> I am wondering if we should find other way to fix it, for example,
> touching the timer as long as there is multicast traffic (non-IGMP).

mdb entries represent group subscriptions.  So what you're saying
is that group subscriptions aren't expiring.

Which isn't a problem at all since if there is no querier then group
subscriptions shouldn't expire.  There has to be at least one querier
in the network for this thing to work.  Otherwise it just degenerates
into a non-snooping switch, which is OK.

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: [Patch net-next 2/3] bridge: only expire the mdb entry when query is received

[Cong Wang]

On Tue, 2013-04-30 at 11:22 +0800, Herbert Xu wrote:
> On Tue, Apr 30, 2013 at 11:17:37AM +0800, Cong Wang wrote:
> > 
> > One problem with this solution is that the temp mdb entry will no longer
> > be expired automatically any more, since no query no timer.
> > 
> > I am wondering if we should find other way to fix it, for example,
> > touching the timer as long as there is multicast traffic (non-IGMP).
> 
> mdb entries represent group subscriptions.  So what you're saying
> is that group subscriptions aren't expiring.
> 
> Which isn't a problem at all since if there is no querier then group
> subscriptions shouldn't expire.  There has to be at least one querier
> in the network for this thing to work.  Otherwise it just degenerates
> into a non-snooping switch, which is OK.

Makes sense. I will put this in the changelog.

Thanks.

[Patch net-next 3/3] bridge: send query as soon as leave is received

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

Continue sending queries when leave is received if the user marks
it as a querier.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Adam Baker <linux@baker-net.org.uk>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 net/bridge/br_multicast.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index a821a4e..03a8f2b 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1246,6 +1246,17 @@ static void br_multicast_leave_group(struct net_bridge *br,
 	if (!mp)
 		goto out;
 
+	if (br->multicast_querier) {
+		mod_timer(&br->multicast_query_timer, jiffies);
+		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);
+		}
+	}
+
 	if (port && (port->flags & BR_MULTICAST_FAST_LEAVE)) {
 		struct net_bridge_port_group __rcu **pp;
 
-- 
1.7.7.6

Re: [Patch net-next 3/3] bridge: send query as soon as leave is received

[Herbert Xu]

On Mon, Apr 29, 2013 at 03:26:32PM +0800, Cong Wang wrote:
> From: Cong Wang <amwang@redhat.com>
> 
> Continue sending queries when leave is received if the user marks
> it as a querier.

If we're going to send a query on leave it should be group-specific
and not general.

Thanks,
-- 
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: [Patch net-next 3/3] bridge: send query as soon as leave is received

[Cong Wang]

On Mon, 2013-04-29 at 15:43 +0800, Herbert Xu wrote:
> On Mon, Apr 29, 2013 at 03:26:32PM +0800, Cong Wang wrote:
> > From: Cong Wang <amwang@redhat.com>
> > 
> > Continue sending queries when leave is received if the user marks
> > it as a querier.
> 
> If we're going to send a query on leave it should be group-specific
> and not general.
> 

Right, I will fix it.

Re: [Patch net-next 1/3] bridge: use the bridge IP addr as source addr of querier

[Herbert Xu]

On Mon, Apr 29, 2013 at 03:26:30PM +0800, Cong Wang wrote:
> From: Cong Wang <amwang@redhat.com>
> 
> Quote from Adam:
> "If it is believed that the use of 0.0.0.0
> as the IP address is what is causing strange behaviour on other devices
> then is there a good reason that a bridge rather than a router shouldn't
> be the active querier? If not then using the bridge IP address and
> having the querier enabled by default may be a reasonable solution
> (provided that our querier obeys the election rules and shuts up if it
> sees a query from a lower IP address that isn't 0.0.0.0). Just because a
> device is the elected querier for IGMP doesn't appear to mean it is
> required to perform any other routing functions."

I don't think we should make this the default.  Having it as an
option though is fine with me.  We may also want to allow the user
to explicitly set an IP address.

Thanks,
-- 
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: [Patch net-next 1/3] bridge: use the bridge IP addr as source addr of querier

[Cong Wang]

On Mon, 2013-04-29 at 15:43 +0800, Herbert Xu wrote:
> On Mon, Apr 29, 2013 at 03:26:30PM +0800, Cong Wang wrote:
> > From: Cong Wang <amwang@redhat.com>
> > 
> > Quote from Adam:
> > "If it is believed that the use of 0.0.0.0
> > as the IP address is what is causing strange behaviour on other devices
> > then is there a good reason that a bridge rather than a router shouldn't
> > be the active querier? If not then using the bridge IP address and
> > having the querier enabled by default may be a reasonable solution
> > (provided that our querier obeys the election rules and shuts up if it
> > sees a query from a lower IP address that isn't 0.0.0.0). Just because a
> > device is the elected querier for IGMP doesn't appear to mean it is
> > required to perform any other routing functions."
> 
> I don't think we should make this the default.  Having it as an
> option though is fine with me.  We may also want to allow the user
> to explicitly set an IP address.

Good point. I think we should at least make it a boolen option, I am not
sure if allowing users to explicitly set it is overkill.

Thanks.