Linus Lüssing
2013-Jul-25 12:57 UTC
[Bridge] [PATCH] bridge: disable snooping if there is no querier
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 at 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
Linus Lüssing
2013-Jul-25 13:56 UTC
[Bridge] [PATCHv2] bridge: disable snooping if there is no querier
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 at 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
Stephen Hemminger
2013-Jul-25 16:01 UTC
[Bridge] [PATCHv2] bridge: disable snooping if there is no querier
On Thu, 25 Jul 2013 15:56:20 +0200 Linus L?ssing <linus.luessing at 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); }
Adam Baker
2013-Jul-26 22:19 UTC
Re: [PATCHv2] bridge: disable snooping if there is no querier
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
Adam Baker
2013-Jul-26 22:19 UTC
[Bridge] [PATCHv2] bridge: disable snooping if there is no querier
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 at 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
David Miller
2013-Jul-30 23:10 UTC
[Bridge] [PATCHv2] bridge: disable snooping if there is no querier
From: Linus L?ssing <linus.luessing at 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.