[PATCH] bridge: disable snooping if there is no querier

[PATCH] bridge: disable snooping if there is no querier

[Linus Lüssing]

If there is no querier on a link then we won't get periodic reports and
therefore won't be able to learn about multicast listeners behind ports,
potentially leading to lost multicast packets, especially for multicast
listeners that joined before the creation of the bridge.

These lost multicast packets can appear since c5c23260594
("bridge: Add multicast_querier toggle and disable queries by default")
in particular.

With this patch we are flooding multicast packets if our querier is
disabled and if we didn't detect any other querier.

A grace period of the Maximum Response Delay of the querier is added to
give multicast responses enough time to arrive and to be learned from
before disabling the flooding behaviour again.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 net/bridge/br_device.c    |    3 ++-
 net/bridge/br_input.c     |    3 ++-
 net/bridge/br_multicast.c |   41 ++++++++++++++++++++++++++++++++---------
 net/bridge/br_private.h   |   11 +++++++++++
 4 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 2ef6678..69363bd 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -70,7 +70,8 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 
 		mdst = br_mdb_get(br, skb, vid);
-		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb))
+		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
+		    br_multicast_querier_exists(br))
 			br_multicast_deliver(mdst, skb);
 		else
 			br_flood_deliver(br, skb, false);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 1b8b8b8..8c561c0 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -101,7 +101,8 @@ int br_handle_frame_finish(struct sk_buff *skb)
 		unicast = false;
 	} else if (is_multicast_ether_addr(dest)) {
 		mdst = br_mdb_get(br, skb, vid);
-		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) {
+		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
+		    br_multicast_querier_exists(br)) {
 			if ((mdst && mdst->mglist) ||
 			    br_multicast_is_router(br))
 				skb2 = skb;
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 69af490..5b5b9c0 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1011,6 +1011,17 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
 }
 #endif
 
+static void br_multicast_update_querier_timer(struct net_bridge *br,
+					      unsigned long max_delay)
+{
+	if (!timer_pending(&br->multicast_querier_timer))
+		atomic64_set(&br->multicast_querier_delay_time,
+			     jiffies + max_delay);
+
+	mod_timer(&br->multicast_querier_timer,
+		  jiffies + br->multicast_querier_interval);
+}
+
 /*
  * Add port to router_list
  *  list is maintained ordered by pointer value
@@ -1061,11 +1072,11 @@ timer:
 
 static void br_multicast_query_received(struct net_bridge *br,
 					struct net_bridge_port *port,
-					int saddr)
+					int saddr,
+					unsigned long max_delay)
 {
 	if (saddr)
-		mod_timer(&br->multicast_querier_timer,
-			  jiffies + br->multicast_querier_interval);
+		br_multicast_update_querier_timer(br, max_delay);
 	else if (timer_pending(&br->multicast_querier_timer))
 		return;
 
@@ -1093,8 +1104,6 @@ static int br_ip4_multicast_query(struct net_bridge *br,
 	    (port && port->state == BR_STATE_DISABLED))
 		goto out;
 
-	br_multicast_query_received(br, port, !!iph->saddr);
-
 	group = ih->group;
 
 	if (skb->len == sizeof(*ih)) {
@@ -1118,6 +1127,8 @@ static int br_ip4_multicast_query(struct net_bridge *br,
 			    IGMPV3_MRC(ih3->code) * (HZ / IGMP_TIMER_SCALE) : 1;
 	}
 
+	br_multicast_query_received(br, port, !!iph->saddr, max_delay);
+
 	if (!group)
 		goto out;
 
@@ -1174,8 +1185,6 @@ static int br_ip6_multicast_query(struct net_bridge *br,
 	    (port && port->state == BR_STATE_DISABLED))
 		goto out;
 
-	br_multicast_query_received(br, port, !ipv6_addr_any(&ip6h->saddr));
-
 	if (skb->len == sizeof(*mld)) {
 		if (!pskb_may_pull(skb, sizeof(*mld))) {
 			err = -EINVAL;
@@ -1196,6 +1205,9 @@ static int br_ip6_multicast_query(struct net_bridge *br,
 		max_delay = mld2q->mld2q_mrc ? MLDV2_MRC(ntohs(mld2q->mld2q_mrc)) : 1;
 	}
 
+	br_multicast_query_received(br, port, !ipv6_addr_any(&ip6h->saddr),
+				    max_delay);
+
 	if (!group)
 		goto out;
 
@@ -1642,6 +1654,8 @@ void br_multicast_init(struct net_bridge *br)
 	br->multicast_querier_interval = 255 * HZ;
 	br->multicast_membership_interval = 260 * HZ;
 
+	atomic64_set(&br->multicast_querier_delay_time, 0);
+
 	spin_lock_init(&br->multicast_lock);
 	setup_timer(&br->multicast_router_timer,
 		    br_multicast_local_router_expired, 0);
@@ -1830,6 +1844,8 @@ unlock:
 
 int br_multicast_set_querier(struct net_bridge *br, unsigned long val)
 {
+	unsigned long max_delay;
+
 	val = !!val;
 
 	spin_lock_bh(&br->multicast_lock);
@@ -1837,8 +1853,15 @@ int br_multicast_set_querier(struct net_bridge *br, unsigned long val)
 		goto unlock;
 
 	br->multicast_querier = val;
-	if (val)
-		br_multicast_start_querier(br);
+	if (!val)
+		goto unlock;
+
+	max_delay = br->multicast_query_response_interval;
+	if (!timer_pending(&br->multicast_querier_timer))
+		atomic64_set(&br->multicast_querier_delay_time,
+			     jiffies + max_delay);
+
+	br_multicast_start_querier(br);
 
 unlock:
 	spin_unlock_bh(&br->multicast_lock);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 3be89b3..bbf5173 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -275,6 +275,7 @@ struct net_bridge
 	struct timer_list		multicast_router_timer;
 	struct timer_list		multicast_querier_timer;
 	struct timer_list		multicast_query_timer;
+	atomic64_t			multicast_querier_delay_time;
 #endif
 
 	struct timer_list		hello_timer;
@@ -501,6 +502,16 @@ static inline bool br_multicast_is_router(struct net_bridge *br)
 	       (br->multicast_router == 1 &&
 		timer_pending(&br->multicast_router_timer));
 }
+
+static inline bool br_multicast_querier_exists(struct net_bridge *br)
+{
+	unsigned long delay_time =
+			atomic64_read(&br->multicast_querier_delay_time);
+
+	return time_is_before_jiffies(delay_time) &&
+	       (br->multicast_querier ||
+		timer_pending(&br->multicast_querier_timer));
+}
 #else
 static inline int br_multicast_rcv(struct net_bridge *br,
 				   struct net_bridge_port *port,
-- 
1.7.10.4

[PATCHv2] bridge: disable snooping if there is no querier

[Linus Lüssing]

If there is no querier on a link then we won't get periodic reports and
therefore won't be able to learn about multicast listeners behind ports,
potentially leading to lost multicast packets, especially for multicast
listeners that joined before the creation of the bridge.

These lost multicast packets can appear since c5c23260594
("bridge: Add multicast_querier toggle and disable queries by default")
in particular.

With this patch we are flooding multicast packets if our querier is
disabled and if we didn't detect any other querier.

A grace period of the Maximum Response Delay of the querier is added to
give multicast responses enough time to arrive and to be learned from
before disabling the flooding behaviour again.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
v2: added missing, empty br_multicast_querier_exists() to avoid
    build failures if CONFIG_BRIDGE_IGMP_SNOOPING is not set


 net/bridge/br_device.c    |    3 ++-
 net/bridge/br_input.c     |    3 ++-
 net/bridge/br_multicast.c |   41 ++++++++++++++++++++++++++++++++---------
 net/bridge/br_private.h   |   15 +++++++++++++++
 4 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 2ef6678..69363bd 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -70,7 +70,8 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 
 		mdst = br_mdb_get(br, skb, vid);
-		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb))
+		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
+		    br_multicast_querier_exists(br))
 			br_multicast_deliver(mdst, skb);
 		else
 			br_flood_deliver(br, skb, false);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 1b8b8b8..8c561c0 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -101,7 +101,8 @@ int br_handle_frame_finish(struct sk_buff *skb)
 		unicast = false;
 	} else if (is_multicast_ether_addr(dest)) {
 		mdst = br_mdb_get(br, skb, vid);
-		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) {
+		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
+		    br_multicast_querier_exists(br)) {
 			if ((mdst && mdst->mglist) ||
 			    br_multicast_is_router(br))
 				skb2 = skb;
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 69af490..5b5b9c0 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1011,6 +1011,17 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
 }
 #endif
 
+static void br_multicast_update_querier_timer(struct net_bridge *br,
+					      unsigned long max_delay)
+{
+	if (!timer_pending(&br->multicast_querier_timer))
+		atomic64_set(&br->multicast_querier_delay_time,
+			     jiffies + max_delay);
+
+	mod_timer(&br->multicast_querier_timer,
+		  jiffies + br->multicast_querier_interval);
+}
+
 /*
  * Add port to router_list
  *  list is maintained ordered by pointer value
@@ -1061,11 +1072,11 @@ timer:
 
 static void br_multicast_query_received(struct net_bridge *br,
 					struct net_bridge_port *port,
-					int saddr)
+					int saddr,
+					unsigned long max_delay)
 {
 	if (saddr)
-		mod_timer(&br->multicast_querier_timer,
-			  jiffies + br->multicast_querier_interval);
+		br_multicast_update_querier_timer(br, max_delay);
 	else if (timer_pending(&br->multicast_querier_timer))
 		return;
 
@@ -1093,8 +1104,6 @@ static int br_ip4_multicast_query(struct net_bridge *br,
 	    (port && port->state == BR_STATE_DISABLED))
 		goto out;
 
-	br_multicast_query_received(br, port, !!iph->saddr);
-
 	group = ih->group;
 
 	if (skb->len == sizeof(*ih)) {
@@ -1118,6 +1127,8 @@ static int br_ip4_multicast_query(struct net_bridge *br,
 			    IGMPV3_MRC(ih3->code) * (HZ / IGMP_TIMER_SCALE) : 1;
 	}
 
+	br_multicast_query_received(br, port, !!iph->saddr, max_delay);
+
 	if (!group)
 		goto out;
 
@@ -1174,8 +1185,6 @@ static int br_ip6_multicast_query(struct net_bridge *br,
 	    (port && port->state == BR_STATE_DISABLED))
 		goto out;
 
-	br_multicast_query_received(br, port, !ipv6_addr_any(&ip6h->saddr));
-
 	if (skb->len == sizeof(*mld)) {
 		if (!pskb_may_pull(skb, sizeof(*mld))) {
 			err = -EINVAL;
@@ -1196,6 +1205,9 @@ static int br_ip6_multicast_query(struct net_bridge *br,
 		max_delay = mld2q->mld2q_mrc ? MLDV2_MRC(ntohs(mld2q->mld2q_mrc)) : 1;
 	}
 
+	br_multicast_query_received(br, port, !ipv6_addr_any(&ip6h->saddr),
+				    max_delay);
+
 	if (!group)
 		goto out;
 
@@ -1642,6 +1654,8 @@ void br_multicast_init(struct net_bridge *br)
 	br->multicast_querier_interval = 255 * HZ;
 	br->multicast_membership_interval = 260 * HZ;
 
+	atomic64_set(&br->multicast_querier_delay_time, 0);
+
 	spin_lock_init(&br->multicast_lock);
 	setup_timer(&br->multicast_router_timer,
 		    br_multicast_local_router_expired, 0);
@@ -1830,6 +1844,8 @@ unlock:
 
 int br_multicast_set_querier(struct net_bridge *br, unsigned long val)
 {
+	unsigned long max_delay;
+
 	val = !!val;
 
 	spin_lock_bh(&br->multicast_lock);
@@ -1837,8 +1853,15 @@ int br_multicast_set_querier(struct net_bridge *br, unsigned long val)
 		goto unlock;
 
 	br->multicast_querier = val;
-	if (val)
-		br_multicast_start_querier(br);
+	if (!val)
+		goto unlock;
+
+	max_delay = br->multicast_query_response_interval;
+	if (!timer_pending(&br->multicast_querier_timer))
+		atomic64_set(&br->multicast_querier_delay_time,
+			     jiffies + max_delay);
+
+	br_multicast_start_querier(br);
 
 unlock:
 	spin_unlock_bh(&br->multicast_lock);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 3be89b3..5c04658 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -275,6 +275,7 @@ struct net_bridge
 	struct timer_list		multicast_router_timer;
 	struct timer_list		multicast_querier_timer;
 	struct timer_list		multicast_query_timer;
+	atomic64_t			multicast_querier_delay_time;
 #endif
 
 	struct timer_list		hello_timer;
@@ -501,6 +502,16 @@ static inline bool br_multicast_is_router(struct net_bridge *br)
 	       (br->multicast_router == 1 &&
 		timer_pending(&br->multicast_router_timer));
 }
+
+static inline bool br_multicast_querier_exists(struct net_bridge *br)
+{
+	unsigned long delay_time =
+			atomic64_read(&br->multicast_querier_delay_time);
+
+	return time_is_before_jiffies(delay_time) &&
+	       (br->multicast_querier ||
+		timer_pending(&br->multicast_querier_timer));
+}
 #else
 static inline int br_multicast_rcv(struct net_bridge *br,
 				   struct net_bridge_port *port,
@@ -557,6 +568,10 @@ static inline bool br_multicast_is_router(struct net_bridge *br)
 {
 	return 0;
 }
+static inline bool br_multicast_querier_exists(struct net_bridge *br)
+{
+	return false;
+}
 static inline void br_mdb_init(void)
 {
 }
-- 
1.7.10.4

Re: [PATCHv2] bridge: disable snooping if there is no querier

[Stephen Hemminger]

On Thu, 25 Jul 2013 15:56:20 +0200
Linus Lüssing <linus.luessing@web.de> wrote:

>  
> +static void br_multicast_update_querier_timer(struct net_bridge *br,
> +					      unsigned long max_delay)
> +{
> +	if (!timer_pending(&br->multicast_querier_timer))
> +		atomic64_set(&br->multicast_querier_delay_time,
> +			     jiffies + max_delay);
> +
> +	mod_timer(&br->multicast_querier_timer,
> +		  jiffies + br->multicast_querier_interval);
> +}
> +

Isn't this test racing with timer expiration.

static void br_multicast_update_querier_timer(struct net_bridge *br,
					      unsigned long max_delay)
{
	if (!timer_pending(&br->multicast_querier_timer))
		atomic64_set(&br->multicast_querier_delay_time,
			     jiffies + max_delay);
What if timer completes here?

	mod_timer(&br->multicast_querier_timer,
		  jiffies + br->multicast_querier_interval);
}


And another race if timer goes off?

static void br_multicast_update_querier_timer(struct net_bridge *br,
					      unsigned long max_delay)
{
	if (!timer_pending(&br->multicast_querier_timer))
		atomic64_set(&br->multicast_querier_delay_time,
			     jiffies + max_delay);
Timer fires here...?

	mod_timer(&br->multicast_querier_timer,
		  jiffies + br->multicast_querier_interval);
}

Re: [PATCHv2] bridge: disable snooping if there is no querier

[Linus Lüssing]

On Thu, Jul 25, 2013 at 09:01:40AM -0700, Stephen Hemminger wrote:
> On Thu, 25 Jul 2013 15:56:20 +0200
> Linus Lüssing <linus.luessing@web.de> wrote:
> 
> >  
> > +static void br_multicast_update_querier_timer(struct net_bridge *br,
> > +					      unsigned long max_delay)
> > +{
> > +	if (!timer_pending(&br->multicast_querier_timer))
> > +		atomic64_set(&br->multicast_querier_delay_time,
> > +			     jiffies + max_delay);
> > +
> > +	mod_timer(&br->multicast_querier_timer,
> > +		  jiffies + br->multicast_querier_interval);
> > +}
> > +
> 
> Isn't this test racing with timer expiration.
> 
> static void br_multicast_update_querier_timer(struct net_bridge *br,
> 					      unsigned long max_delay)
> {
> 	if (!timer_pending(&br->multicast_querier_timer))
> 		atomic64_set(&br->multicast_querier_delay_time,
> 			     jiffies + max_delay);
> What if timer completes here?

If the timer completes here, then for one thing this means that
the query message is very late (we were supposed to have heard
at least two query messages by now, query messages should by
default arrive every 125 seconds, we are at 255 seconds now).

Which in most cases would have the reason of the original querier
having left.

Not resetting the newly introduced
br->multicast_querier_delay_time means that we won't switch back
to flooding for a grace period (which we would have done if the
timer had completed three lines earlier).

So the question is, does refraining from switching back to
flooding for the grace period result in any packet loss in this
scenario?

Yes and no. Our current records from the previous multicast
listener reports are still valid until
br->multicast_membership_interval, so for another 5 seconds.
So in the worst case we can have lost multicast packets for
up to five seconds for some listeners.

However, normal multicast routers would have the same issue for
this five seconds period. So to me it looks like this is
actually a bug in RFC2710, section 7.4 - Multicast Listener
Interval: We and multicast routers wouldn't have that problem if
it were 'plus (one _and a half_ Query Response Interval)' instead.

So maybe we could just increase br->multicast_membership_interval
from 260 to 265 with another patch?


Despite from that I don't see which other issues could arise from
the race you pointed out here.

> 
> 	mod_timer(&br->multicast_querier_timer,
> 		  jiffies + br->multicast_querier_interval);
> }
> 
> 
> And another race if timer goes off?
> 
> static void br_multicast_update_querier_timer(struct net_bridge *br,
> 					      unsigned long max_delay)
> {
> 	if (!timer_pending(&br->multicast_querier_timer))
> 		atomic64_set(&br->multicast_querier_delay_time,
> 			     jiffies + max_delay);
> Timer fires here...?
> 
> 	mod_timer(&br->multicast_querier_timer,
> 		  jiffies + br->multicast_querier_interval);
> }

Hm? Sorry, I don't quite see how this race differs from the one
you pointed out before.


Thanks for looking at this patch so far!

Cheers, Linus

Re: [PATCHv2] bridge: disable snooping if there is no querier

[Adam Baker]

On 25/07/13 14:56, Linus Lüssing wrote:
> If there is no querier on a link then we won't get periodic reports and
> therefore won't be able to learn about multicast listeners behind ports,
> potentially leading to lost multicast packets, especially for multicast
> listeners that joined before the creation of the bridge.
>
> These lost multicast packets can appear since c5c23260594
> ("bridge: Add multicast_querier toggle and disable queries by default")
> in particular.
>
> With this patch we are flooding multicast packets if our querier is
> disabled and if we didn't detect any other querier.
>
> A grace period of the Maximum Response Delay of the querier is added to
> give multicast responses enough time to arrive and to be learned from
> before disabling the flooding behaviour again.
>
> Signed-off-by: Linus Lüssing<linus.luessing@web.de>

If the lack of queries if there is no other querier is unacceptable to 
the majority of users (and I believe it is) then surely the sensible 
option is to have the multicast querier toggle enabled by default.

The toggle was added in the first place because the queries were 
reported to be generating issues with certain other equipment. This may 
have been because the queries by default have an invalid IP address 
(although I have been unable to identify what equipment they caused 
problems with so can't verify this).

If the only reason to turn the querier off is because it interferes with 
other equipment then the solution to it being off by default isn't to 
generate queries in some instances even if it is off but rather to turn 
it on by default and only turn it off if it causes problems. If 
multicast_query_use_ifaddr was also enabled by default the the 
likelihood of the querier causing problems elsewhere should be reduced.

Regards

Adam

Re: [PATCHv2] bridge: disable snooping if there is no querier

[Linus Lüssing]

On Fri, Jul 26, 2013 at 11:19:00PM +0100, Adam Baker wrote:
> On 25/07/13 14:56, Linus Lüssing wrote:
> >If there is no querier on a link then we won't get periodic reports and
> >therefore won't be able to learn about multicast listeners behind ports,
> >potentially leading to lost multicast packets, especially for multicast
> >listeners that joined before the creation of the bridge.
> >
> >These lost multicast packets can appear since c5c23260594
> >("bridge: Add multicast_querier toggle and disable queries by default")
> >in particular.
> >
> >With this patch we are flooding multicast packets if our querier is
> >disabled and if we didn't detect any other querier.
> >
> >A grace period of the Maximum Response Delay of the querier is added to
> >give multicast responses enough time to arrive and to be learned from
> >before disabling the flooding behaviour again.
> >
> >Signed-off-by: Linus Lüssing<linus.luessing@web.de>
> 
> If the lack of queries if there is no other querier is unacceptable
> to the majority of users (and I believe it is) then surely the
> sensible option is to have the multicast querier toggle enabled by
> default.
> 
> The toggle was added in the first place because the queries were
> reported to be generating issues with certain other equipment. This
> may have been because the queries by default have an invalid IP
> address (although I have been unable to identify what equipment they
> caused problems with so can't verify this).
> 
> If the only reason to turn the querier off is because it interferes
> with other equipment then the solution to it being off by default
> isn't to generate queries in some instances even if it is off but
> rather to turn it on by default and only turn it off if it causes
> problems. If multicast_query_use_ifaddr was also enabled by default
> the the likelihood of the querier causing problems elsewhere should
> be reduced.
> 
> Regards
> 
> Adam

One more, general disadvantage I could see is, that in a network
with multiple bridges basically a random one would become the querier
and the according network segment would get hit by all the
according multicast traffic. If the available bandwidth of links
on your network varies, then you however usually want to have the
querier in a "good" position of your network. Which might be a
little harder to control if the querier is on by default.

Also this specific, current querier implementation has two more
disadvantages: 
* It's doing MLDv1/IGMPv2 queries, so it downgrades our whole
  network to MLDv1/IGMPv2, no MLDv2/IGMPv3 and source specific
  multicast could be used.
* The querier selection is not RFC compliant (we should refrain
  from sending queries if our address is higher, not if we hear
  any query)

Cheers, Linus

Re: [PATCHv2] bridge: disable snooping if there is no querier

[David Miller]

From: Linus Lüssing <linus.luessing@web.de>
Date: Thu, 25 Jul 2013 15:56:20 +0200

> +	atomic64_t			multicast_querier_delay_time;

Please don't use an atomic64_t here, it's pointless.

You're only doing set and read operations on it, there's absolutely
nothing atomic about that.

You have to make sure that the top-level operations that use this
new value use an appropriate amount of locking on the higher level
objects.

[PATCHv3] bridge: disable snooping if there is no querier

[Linus Lüssing]

If there is no querier on a link then we won't get periodic reports and
therefore won't be able to learn about multicast listeners behind ports,
potentially leading to lost multicast packets, especially for multicast
listeners that joined before the creation of the bridge.

These lost multicast packets can appear since c5c23260594
("bridge: Add multicast_querier toggle and disable queries by default")
in particular.

With this patch we are flooding multicast packets if our querier is
disabled and if we didn't detect any other querier.

A grace period of the Maximum Response Delay of the querier is added to
give multicast responses enough time to arrive and to be learned from
before disabling the flooding behaviour again.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
v3: changed type of multicast_querier_delay_time from
    atomic64_t to unsigned long
v2: added missing, empty br_multicast_querier_exists() to avoid
    build failures if CONFIG_BRIDGE_IGMP_SNOOPING is not set

 net/bridge/br_device.c    |    3 ++-
 net/bridge/br_input.c     |    3 ++-
 net/bridge/br_multicast.c |   39 ++++++++++++++++++++++++++++++---------
 net/bridge/br_private.h   |   12 ++++++++++++
 4 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 2ef6678..69363bd 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -70,7 +70,8 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 
 		mdst = br_mdb_get(br, skb, vid);
-		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb))
+		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
+		    br_multicast_querier_exists(br))
 			br_multicast_deliver(mdst, skb);
 		else
 			br_flood_deliver(br, skb, false);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 1b8b8b8..8c561c0 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -101,7 +101,8 @@ int br_handle_frame_finish(struct sk_buff *skb)
 		unicast = false;
 	} else if (is_multicast_ether_addr(dest)) {
 		mdst = br_mdb_get(br, skb, vid);
-		if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) {
+		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
+		    br_multicast_querier_exists(br)) {
 			if ((mdst && mdst->mglist) ||
 			    br_multicast_is_router(br))
 				skb2 = skb;
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 4b99c9a..61c5e81 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1014,6 +1014,16 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
 }
 #endif
 
+static void br_multicast_update_querier_timer(struct net_bridge *br,
+					      unsigned long max_delay)
+{
+	if (!timer_pending(&br->multicast_querier_timer))
+		br->multicast_querier_delay_time = jiffies + max_delay;
+
+	mod_timer(&br->multicast_querier_timer,
+		  jiffies + br->multicast_querier_interval);
+}
+
 /*
  * Add port to router_list
  *  list is maintained ordered by pointer value
@@ -1064,11 +1074,11 @@ timer:
 
 static void br_multicast_query_received(struct net_bridge *br,
 					struct net_bridge_port *port,
-					int saddr)
+					int saddr,
+					unsigned long max_delay)
 {
 	if (saddr)
-		mod_timer(&br->multicast_querier_timer,
-			  jiffies + br->multicast_querier_interval);
+		br_multicast_update_querier_timer(br, max_delay);
 	else if (timer_pending(&br->multicast_querier_timer))
 		return;
 
@@ -1096,8 +1106,6 @@ static int br_ip4_multicast_query(struct net_bridge *br,
 	    (port && port->state == BR_STATE_DISABLED))
 		goto out;
 
-	br_multicast_query_received(br, port, !!iph->saddr);
-
 	group = ih->group;
 
 	if (skb->len == sizeof(*ih)) {
@@ -1121,6 +1129,8 @@ static int br_ip4_multicast_query(struct net_bridge *br,
 			    IGMPV3_MRC(ih3->code) * (HZ / IGMP_TIMER_SCALE) : 1;
 	}
 
+	br_multicast_query_received(br, port, !!iph->saddr, max_delay);
+
 	if (!group)
 		goto out;
 
@@ -1176,8 +1186,6 @@ static int br_ip6_multicast_query(struct net_bridge *br,
 	    (port && port->state == BR_STATE_DISABLED))
 		goto out;
 
-	br_multicast_query_received(br, port, !ipv6_addr_any(&ip6h->saddr));
-
 	if (skb->len == sizeof(*mld)) {
 		if (!pskb_may_pull(skb, sizeof(*mld))) {
 			err = -EINVAL;
@@ -1198,6 +1206,9 @@ static int br_ip6_multicast_query(struct net_bridge *br,
 		max_delay = mld2q->mld2q_mrc ? MLDV2_MRC(ntohs(mld2q->mld2q_mrc)) : 1;
 	}
 
+	br_multicast_query_received(br, port, !ipv6_addr_any(&ip6h->saddr),
+				    max_delay);
+
 	if (!group)
 		goto out;
 
@@ -1643,6 +1654,8 @@ void br_multicast_init(struct net_bridge *br)
 	br->multicast_querier_interval = 255 * HZ;
 	br->multicast_membership_interval = 260 * HZ;
 
+	br->multicast_querier_delay_time = 0;
+
 	spin_lock_init(&br->multicast_lock);
 	setup_timer(&br->multicast_router_timer,
 		    br_multicast_local_router_expired, 0);
@@ -1831,6 +1844,8 @@ unlock:
 
 int br_multicast_set_querier(struct net_bridge *br, unsigned long val)
 {
+	unsigned long max_delay;
+
 	val = !!val;
 
 	spin_lock_bh(&br->multicast_lock);
@@ -1838,8 +1853,14 @@ int br_multicast_set_querier(struct net_bridge *br, unsigned long val)
 		goto unlock;
 
 	br->multicast_querier = val;
-	if (val)
-		br_multicast_start_querier(br);
+	if (!val)
+		goto unlock;
+
+	max_delay = br->multicast_query_response_interval;
+	if (!timer_pending(&br->multicast_querier_timer))
+		br->multicast_querier_delay_time = jiffies + max_delay;
+
+	br_multicast_start_querier(br);
 
 unlock:
 	spin_unlock_bh(&br->multicast_lock);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 3be89b3..2f7da41 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -267,6 +267,7 @@ struct net_bridge
 	unsigned long			multicast_query_interval;
 	unsigned long			multicast_query_response_interval;
 	unsigned long			multicast_startup_query_interval;
+	unsigned long			multicast_querier_delay_time;
 
 	spinlock_t			multicast_lock;
 	struct net_bridge_mdb_htable __rcu *mdb;
@@ -501,6 +502,13 @@ static inline bool br_multicast_is_router(struct net_bridge *br)
 	       (br->multicast_router == 1 &&
 		timer_pending(&br->multicast_router_timer));
 }
+
+static inline bool br_multicast_querier_exists(struct net_bridge *br)
+{
+	return time_is_before_jiffies(br->multicast_querier_delay_time) &&
+	       (br->multicast_querier ||
+		timer_pending(&br->multicast_querier_timer));
+}
 #else
 static inline int br_multicast_rcv(struct net_bridge *br,
 				   struct net_bridge_port *port,
@@ -557,6 +565,10 @@ static inline bool br_multicast_is_router(struct net_bridge *br)
 {
 	return 0;
 }
+static inline bool br_multicast_querier_exists(struct net_bridge *br)
+{
+	return false;
+}
 static inline void br_mdb_init(void)
 {
 }
-- 
1.7.10.4

Re: [PATCHv3] bridge: disable snooping if there is no querier

[David Miller]

From: Linus Lüssing <linus.luessing@web.de>
Date: Thu,  1 Aug 2013 01:06:20 +0200

> If there is no querier on a link then we won't get periodic reports and
> therefore won't be able to learn about multicast listeners behind ports,
> potentially leading to lost multicast packets, especially for multicast
> listeners that joined before the creation of the bridge.
> 
> These lost multicast packets can appear since c5c23260594
> ("bridge: Add multicast_querier toggle and disable queries by default")
> in particular.
> 
> With this patch we are flooding multicast packets if our querier is
> disabled and if we didn't detect any other querier.
> 
> A grace period of the Maximum Response Delay of the querier is added to
> give multicast responses enough time to arrive and to be learned from
> before disabling the flooding behaviour again.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>

Looks good, applied, thanks Linus.

Re: [PATCHv3] bridge: disable snooping if there is no querier

[Paul Bolle]

On Wed, 2013-07-31 at 17:40 -0700, David Miller wrote:
> > If there is no querier on a link then we won't get periodic reports and
> > therefore won't be able to learn about multicast listeners behind ports,
> > potentially leading to lost multicast packets, especially for multicast
> > listeners that joined before the creation of the bridge.
> > 
> > These lost multicast packets can appear since c5c23260594
> > ("bridge: Add multicast_querier toggle and disable queries by default")
> > in particular.
> > 
> > With this patch we are flooding multicast packets if our querier is
> > disabled and if we didn't detect any other querier.
> > 
> > A grace period of the Maximum Response Delay of the querier is added to
> > give multicast responses enough time to arrive and to be learned from
> > before disabling the flooding behaviour again.
> > 
> > Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> 
> Looks good, applied, thanks Linus.

0) This patch is part of v3.11-rc4 as commit b00589af3b0. It introduced
a GCC warning:
    net/bridge/br_multicast.c: In function ‘br_multicast_rcv’:
    net/bridge/br_multicast.c:1081:36: warning: ‘max_delay’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    net/bridge/br_multicast.c:1178:16: note: ‘max_delay’ was declared here

1) Summarized, the code reads:

        unsigned long max_delay;

        if (skb->len == sizeof(*mld))
                max_delay = msecs_to_jiffies(ntohs(mld->mld_maxdelay));
        else if (skb->len >= sizeof(*mld2q))
                max_delay = mld2q->mld2q_mrc ? MLDV2_MRC(ntohs(mld2q->mld2q_mrc)) : 1;

        br_multicast_query_received(br, port, !ipv6_addr_any(&ip6h->saddr),
                                    max_delay);
 
So GCC notices that max_delay is still uninitialized if skb->len is
neither equal to sizeof(*mld) nor equal or bigger than sizeof(*mld2q).
To me it looks GCC is right here. At least, it is not obvious that
max_delay will actually not be used in br_multicast_query_received() if
it still is uninitialized.

2) I'm entirely unfamiliar to this code. So I can't say how this warning
might be silenced.


Paul Bolle

Re: [PATCHv3] bridge: disable snooping if there is no querier

[Linus Lüssing]

Thanks for the very quick reporting! Looks like my gcc was and still
isn't as supportive... You and the lists should have received a patch
to fix that.

Cheers, Linus


On Mon, Aug 05, 2013 at 10:41:06AM +0200, Paul Bolle wrote:
> On Wed, 2013-07-31 at 17:40 -0700, David Miller wrote:
> > > If there is no querier on a link then we won't get periodic reports and
> > > therefore won't be able to learn about multicast listeners behind ports,
> > > potentially leading to lost multicast packets, especially for multicast
> > > listeners that joined before the creation of the bridge.
> > > 
> > > These lost multicast packets can appear since c5c23260594
> > > ("bridge: Add multicast_querier toggle and disable queries by default")
> > > in particular.
> > > 
> > > With this patch we are flooding multicast packets if our querier is
> > > disabled and if we didn't detect any other querier.
> > > 
> > > A grace period of the Maximum Response Delay of the querier is added to
> > > give multicast responses enough time to arrive and to be learned from
> > > before disabling the flooding behaviour again.
> > > 
> > > Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> > 
> > Looks good, applied, thanks Linus.
> 
> 0) This patch is part of v3.11-rc4 as commit b00589af3b0. It introduced
> a GCC warning:
>     net/bridge/br_multicast.c: In function ‘br_multicast_rcv’:
>     net/bridge/br_multicast.c:1081:36: warning: ‘max_delay’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     net/bridge/br_multicast.c:1178:16: note: ‘max_delay’ was declared here
> 
> 1) Summarized, the code reads:
> 
>         unsigned long max_delay;
> 
>         if (skb->len == sizeof(*mld))
>                 max_delay = msecs_to_jiffies(ntohs(mld->mld_maxdelay));
>         else if (skb->len >= sizeof(*mld2q))
>                 max_delay = mld2q->mld2q_mrc ? MLDV2_MRC(ntohs(mld2q->mld2q_mrc)) : 1;
> 
>         br_multicast_query_received(br, port, !ipv6_addr_any(&ip6h->saddr),
>                                     max_delay);
>  
> So GCC notices that max_delay is still uninitialized if skb->len is
> neither equal to sizeof(*mld) nor equal or bigger than sizeof(*mld2q).
> To me it looks GCC is right here. At least, it is not obvious that
> max_delay will actually not be used in br_multicast_query_received() if
> it still is uninitialized.
> 
> 2) I'm entirely unfamiliar to this code. So I can't say how this warning
> might be silenced.
> 
> 
> Paul Bolle
>