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

[Patch net-next v5 1/3] bridge: use the bridge IP addr as source addr for 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."

And introduce a new troggle for it, as suggested by Herbert.

Suggested-by: Adam Baker <linux@baker-net.org.uk>
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>
---
v5: change the sysfs file name to multicast_query_use_ifaddr
v4: no change
v3: no change
v2: introduce a new troggle

 net/bridge/br_multicast.c |    5 ++++-
 net/bridge/br_private.h   |    1 +
 net/bridge/br_sysfs_br.c  |   26 ++++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 81f2389..2475147 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,8 @@ 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 = br->multicast_query_use_ifaddr ?
+		     inet_select_addr(br->dev, 0, RT_SCOPE_LINK) : 0;
 	iph->daddr = htonl(INADDR_ALLHOSTS_GROUP);
 	((u8 *)&iph[1])[0] = IPOPT_RA;
 	((u8 *)&iph[1])[1] = 4;
@@ -1618,6 +1620,7 @@ void br_multicast_init(struct net_bridge *br)
 
 	br->multicast_router = 1;
 	br->multicast_querier = 0;
+	br->multicast_query_use_ifaddr = 0;
 	br->multicast_last_member_count = 2;
 	br->multicast_startup_query_count = 2;
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index d2c043a..e260710 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -249,6 +249,7 @@ struct net_bridge
 
 	u8				multicast_disabled:1;
 	u8				multicast_querier:1;
+	u8				multicast_query_use_ifaddr:1;
 
 	u32				hash_elasticity;
 	u32				hash_max;
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 8baa9c0..394bb96 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -375,6 +375,31 @@ 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_query_use_ifaddr(struct device *d,
+				      struct device_attribute *attr,
+				      char *buf)
+{
+	struct net_bridge *br = to_bridge(d);
+	return sprintf(buf, "%d\n", br->multicast_query_use_ifaddr);
+}
+
+static int set_query_use_ifaddr(struct net_bridge *br, unsigned long val)
+{
+	br->multicast_query_use_ifaddr = !!val;
+	return 0;
+}
+
+static ssize_t
+store_multicast_query_use_ifaddr(struct device *d,
+				 struct device_attribute *attr,
+				 const char *buf, size_t len)
+{
+	return store_bridge_parm(d, buf, len, set_query_use_ifaddr);
+}
+static DEVICE_ATTR(multicast_query_use_ifaddr, S_IRUGO | S_IWUSR,
+		   show_multicast_query_use_ifaddr,
+		   store_multicast_query_use_ifaddr);
+
 static ssize_t show_multicast_querier(struct device *d,
 				      struct device_attribute *attr,
 				      char *buf)
@@ -734,6 +759,7 @@ static struct attribute *bridge_attrs[] = {
 	&dev_attr_multicast_router.attr,
 	&dev_attr_multicast_snooping.attr,
 	&dev_attr_multicast_querier.attr,
+	&dev_attr_multicast_query_use_ifaddr.attr,
 	&dev_attr_hash_elasticity.attr,
 	&dev_attr_hash_max.attr,
 	&dev_attr_multicast_last_member_count.attr,
-- 
1.7.7.6

[Patch net-next v5 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.

And he also mentioned "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."

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>
---
v5: no change
v4: remove some useless timer code
v3: fix the code in br_multicast_leave_group()
v2: add a flag to check if timer is fired or not

 net/bridge/br_multicast.c |   39 ++++++++++++---------------------------
 net/bridge/br_private.h   |    1 +
 2 files changed, 13 insertions(+), 27 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2475147..40bda80 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -617,8 +617,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++;
@@ -656,7 +654,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);
@@ -671,7 +668,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;
 	}
 
@@ -679,7 +675,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;
 	}
@@ -690,8 +686,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;
 
@@ -1131,6 +1125,10 @@ 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);
+	mp->timer_armed = true;
+
 	max_delay *= br->multicast_last_member_count;
 
 	if (mp->mglist &&
@@ -1205,6 +1203,10 @@ 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);
+	mp->timer_armed = true;
+
 	max_delay *= br->multicast_last_member_count;
 	if (mp->mglist &&
 	    (timer_pending(&mp->timer) ?
@@ -1263,7 +1265,7 @@ static void br_multicast_leave_group(struct net_bridge *br,
 			call_rcu_bh(&p->rcu, br_multicast_free_pg);
 			br_mdb_notify(br->dev, port, group, RTM_DELMDB);
 
-			if (!mp->ports && !mp->mglist &&
+			if (!mp->ports && !mp->mglist && mp->timer_armed &&
 			    netif_running(br->dev))
 				mod_timer(&mp->timer, jiffies);
 		}
@@ -1275,30 +1277,12 @@ static void br_multicast_leave_group(struct net_bridge *br,
 		     br->multicast_last_member_interval;
 
 	if (!port) {
-		if (mp->mglist &&
+		if (mp->mglist && mp->timer_armed &&
 		    (timer_pending(&mp->timer) ?
 		     time_after(mp->timer.expires, time) :
 		     try_to_del_timer_sync(&mp->timer) >= 0)) {
 			mod_timer(&mp->timer, time);
 		}
-
-		goto out;
-	}
-
-	for (p = mlock_dereference(mp->ports, br);
-	     p != NULL;
-	     p = mlock_dereference(p->next, br)) {
-		if (p->port != port)
-			continue;
-
-		if (!hlist_unhashed(&p->mglist) &&
-		    (timer_pending(&p->timer) ?
-		     time_after(p->timer.expires, time) :
-		     try_to_del_timer_sync(&p->timer) >= 0)) {
-			mod_timer(&p->timer, time);
-		}
-
-		break;
 	}
 
 out:
@@ -1674,6 +1658,7 @@ void br_multicast_stop(struct net_bridge *br)
 		hlist_for_each_entry_safe(mp, n, &mdb->mhash[i],
 					  hlist[ver]) {
 			del_timer(&mp->timer);
+			mp->timer_armed = false;
 			call_rcu_bh(&mp->rcu, br_multicast_free_group);
 		}
 	}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index e260710..1b0ac95 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -112,6 +112,7 @@ struct net_bridge_mdb_entry
 	struct timer_list		timer;
 	struct br_ip			addr;
 	bool				mglist;
+	bool				timer_armed;
 };
 
 struct net_bridge_mdb_htable
-- 
1.7.7.6

[Patch net-next v5 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>
---
v5: no change
v4: use the right timeout value
    re-arm timers here
v3: use non-general query
v2: only send query to the port received leave

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

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 40bda80..37a4676 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1250,6 +1250,32 @@ static void br_multicast_leave_group(struct net_bridge *br,
 	if (!mp)
 		goto out;
 
+	if (br->multicast_querier &&
+	    !timer_pending(&br->multicast_querier_timer)) {
+		__br_multicast_send_query(br, port, &mp->addr);
+
+		time = jiffies + br->multicast_last_member_count *
+				 br->multicast_last_member_interval;
+		mod_timer(port ? &port->multicast_query_timer :
+				 &br->multicast_query_timer, time);
+
+		for (p = mlock_dereference(mp->ports, br);
+		     p != NULL;
+		     p = mlock_dereference(p->next, br)) {
+			if (p->port != port)
+				continue;
+
+			if (!hlist_unhashed(&p->mglist) &&
+			    (timer_pending(&p->timer) ?
+			     time_after(p->timer.expires, time) :
+			     try_to_del_timer_sync(&p->timer) >= 0)) {
+				mod_timer(&p->timer, time);
+			}
+
+			break;
+		}
+	}
+
 	if (port && (port->flags & BR_MULTICAST_FAST_LEAVE)) {
 		struct net_bridge_port_group __rcu **pp;
 
-- 
1.7.7.6

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

[Herbert Xu]

On Wed, May 22, 2013 at 03:52:54PM +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."
> 
> And introduce a new troggle for it, as suggested by Herbert.
> 
> Suggested-by: Adam Baker <linux@baker-net.org.uk>
> 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>

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

to all three patches.

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 v5 1/3] bridge: use the bridge IP addr as source addr for querier

[Adam Baker]

On 22/05/13 08:52, 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."
>
> And introduce a new troggle for it, as suggested by Herbert.

I've now tested this series applied to a 3.9.4 kernel

Using wireshark I can see that if the multicast_querier and 
multicast_query_use_ifaddr flags are set then queries do get the correct 
IP address in them and if multicast_querier is set and 
multicast_query_use_ifaddr isn't we get queries with the address set to 
0.0.0.0

I next tested with 2 bridges configured on different nodes (this is my 
normal network configuration with the 2 bridge devices acting as 
wireless routers with different coverage areas with a wired network 
between them). If multicast_query_use_ifaddr is set whichever device 
starts querying first will act as the querier and the other will shut 
up. According to RFC 2236 it should be the device with the lower IP 
address that ends up as the querier in that scenario but I can't imagine 
a situation where that exact behaviour matters

If multicast_query_use_ifaddr is not set but multicast_querier is then 
both bridges end up generating queries with a source address of 0.0.0.0. 
Whilst this results in a small amount of unnecessary network traffic it 
does provide a functional setup.

In all of these cases I also verified that multicast UPnP AV 
applications on different network segments remain able to talk to each 
other.

I would therefore suggest that making multicast_query_use_ifaddr the 
default and making the querier only shut up if it sees a query from a 
lower non zero address rather than any non zero address would constitute 
minor improvements to this patch but as it stands it is still an 
improvement on the current behaviour.

Tested-By: Adam Baker <linux@baker-net.org.uk>

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

[Linus Lüssing]

Hi Amerigo,

I just tested these three patches on top of 3.10-rc6 (thanks for
looking into these issues so far!) but unfortunately I'm still
having problems if there is no querier on the link:

We will never discover a multicast listener which sends its
startup MLD reports before being attached to the bridge port.

I had reported that issue two months ago to Herbert's patch which
changed the querier default [1].

I guess in such a scenario where neither our querier is activated
nor could we detect any querier, then the only safe thing we can
do is to disable the bridge multicast snooping, isn't it?

Cheers, Linus


[1]: http://permalink.gmane.org/gmane.linux.network/264442

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

[Herbert Xu]

On Tue, Jun 18, 2013 at 02:03:38PM +0200, Linus Lüssing wrote:
> Hi Amerigo,
> 
> I just tested these three patches on top of 3.10-rc6 (thanks for
> looking into these issues so far!) but unfortunately I'm still
> having problems if there is no querier on the link:
> 
> We will never discover a multicast listener which sends its
> startup MLD reports before being attached to the bridge port.
> 
> I had reported that issue two months ago to Herbert's patch which
> changed the querier default [1].
> 
> I guess in such a scenario where neither our querier is activated
> nor could we detect any querier, then the only safe thing we can
> do is to disable the bridge multicast snooping, isn't it?

If we have seen no queries then we should simply flood.  The fact
that we don't currently is a bug.

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

[Cong Wang]

On Wed, 2013-06-19 at 12:44 +0800, Herbert Xu wrote:
> On Tue, Jun 18, 2013 at 02:03:38PM +0200, Linus Lüssing wrote:
> > Hi Amerigo,
> > 
> > I just tested these three patches on top of 3.10-rc6 (thanks for
> > looking into these issues so far!) but unfortunately I'm still
> > having problems if there is no querier on the link:
> > 
> > We will never discover a multicast listener which sends its
> > startup MLD reports before being attached to the bridge port.
> > 
> > I had reported that issue two months ago to Herbert's patch which
> > changed the querier default [1].
> > 
> > I guess in such a scenario where neither our querier is activated
> > nor could we detect any querier, then the only safe thing we can
> > do is to disable the bridge multicast snooping, isn't it?
> 
> If we have seen no queries then we should simply flood.  The fact
> that we don't currently is a bug.

Something like this?

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 1b8b8b8..df49d14 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -105,6 +105,10 @@ int br_handle_frame_finish(struct sk_buff *skb)
                        if ((mdst && mdst->mglist) ||
                            br_multicast_is_router(br))
                                skb2 = skb;
+                       else if (hlist_empty(&br->router_list)) {
+                               unicast = false;
+                               goto flood;
+                       }
                        br_multicast_forward(mdst, skb, skb2);
                        skb = NULL;
                        if (!skb2)
@@ -121,6 +125,7 @@ int br_handle_frame_finish(struct sk_buff *skb)
                skb = NULL;
        }
 
+flood:
        if (skb) {
                if (dst) {
                        dst->used = jiffies;

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

[Herbert Xu]

On Fri, Jun 21, 2013 at 03:31:41PM +0800, Cong Wang wrote:
> On Wed, 2013-06-19 at 12:44 +0800, Herbert Xu wrote:
> > On Tue, Jun 18, 2013 at 02:03:38PM +0200, Linus Lüssing wrote:
> > > Hi Amerigo,
> > > 
> > > I just tested these three patches on top of 3.10-rc6 (thanks for
> > > looking into these issues so far!) but unfortunately I'm still
> > > having problems if there is no querier on the link:
> > > 
> > > We will never discover a multicast listener which sends its
> > > startup MLD reports before being attached to the bridge port.
> > > 
> > > I had reported that issue two months ago to Herbert's patch which
> > > changed the querier default [1].
> > > 
> > > I guess in such a scenario where neither our querier is activated
> > > nor could we detect any querier, then the only safe thing we can
> > > do is to disable the bridge multicast snooping, isn't it?
> > 
> > If we have seen no queries then we should simply flood.  The fact
> > that we don't currently is a bug.
> 
> Something like this?

Close, ideally we should allow for at least one timeout interval
after seeing the first query.

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