bridge: separate querier and query timer into IGMP/IPv4 and MLD/IPv6 ones

PATCH Superseded Linus Lüssing 2 participants 4 messages Aug 26, 2013 – Aug 30, 2013

Subsystems (3)

Networking Network drivers Ethernet bridge

lore

Share & tools

lore

Linus Lüssing

author

2013-08-26 10:01 UTC #0 lore raw

PATCH 1/1 bridge: separate querier and query timer into IGMP/IPv4 and MLD/IPv6 ones

Currently we would still potentially suffer multicast packet loss if there is just either an IGMP or an MLD querier: For the former case, we would possibly drop IPv6 multicast packets, for the latter IPv4 ones. This is because we are currently assuming that if either an IGMP or MLD querier is present that the other one is present, too.

This patch makes the behaviour and fix added in
"bridge: disable snooping if there is no querier" (b00589af3b04) to also work if there is either just an IGMP or an MLD querier on the
link: It refines the deactivation of the snooping to be protocol specific by using separate timers for the snooped IGMP and MLD queries as well as separate timers for our internal IGMP and MLD queriers.

Signed-off-by Linus Lüssing <[email protected]>

---
 net/bridge/br_device.c    |    2 +-
 net/bridge/br_input.c     |    2 +-
 net/bridge/br_mdb.c       |   14 +-
 net/bridge/br_multicast.c |  338 +++++++++++++++++++++++++++++++++++----------
 net/bridge/br_private.h   |   47 +++++--
 5 files changed, 313 insertions(+), 90 deletions(-)

net/bridge/br_device.c +1 -1

index 69363bd..89659d4 100644
@@ -71,7 +71,7 @@ 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)) &&
-		    br_multicast_querier_exists(br))
+		    br_multicast_querier_exists(br, eth_hdr(skb)))
 			br_multicast_deliver(mdst, skb);
 		else
 			br_flood_deliver(br, skb, false);

net/bridge/br_input.c +1 -1

index 8c561c0..a2fd37e 100644
@@ -102,7 +102,7 @@ int br_handle_frame_finish(struct sk_buff *skb)
 	} else if (is_multicast_ether_addr(dest)) {
 		mdst = br_mdb_get(br, skb, vid);
 		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
-		    br_multicast_querier_exists(br)) {
+		    br_multicast_querier_exists(br, eth_hdr(skb))) {
 			if ((mdst && mdst->mglist) ||
 			    br_multicast_is_router(br))
 				skb2 = skb;

net/bridge/br_mdb.c +9 -5

index 0daae3e..6053b96 100644
@@ -414,16 +414,20 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
 	if (!netif_running(br->dev) || br->multicast_disabled)
 		return -EINVAL;
 
-	if (timer_pending(&br->multicast_querier_timer))
-		return -EBUSY;
-
 	ip.proto = entry->addr.proto;
-	if (ip.proto == htons(ETH_P_IP))
+	if (ip.proto == htons(ETH_P_IP)) {
+		if (timer_pending(&br->ip4_multicast_querier_timer))
+			return -EBUSY;
+
 		ip.u.ip4 = entry->addr.u.ip4;
 #if IS_ENABLED(CONFIG_IPV6)
-	else
+	} else {
+		if (timer_pending(&br->ip6_multicast_querier_timer))
+			return -EBUSY;
+
 		ip.u.ip6 = entry->addr.u.ip6;
 #endif
+	}
 
 	spin_lock_bh(&br->multicast_lock);
 	mdb = mlock_dereference(br->mdb, br);

net/bridge/br_multicast.c +265 ~~-73~~

573 lines not on this page · raw message

net/bridge/br_private.h +37 ~~-10~~

87 lines not on this page · raw message

Signature · 1 line

1.7.10.4

David Miller

maintainer

2013-08-29 00:08 UTC #1 lore raw

From: Linus Lüssing <[email protected]>
Date: Mon, 26 Aug 2013 12:01:06 +0200

Quoted 6 lines

Currently we would still potentially suffer multicast packet loss if there is just either an IGMP or an MLD querier: For the former case, we would possibly drop IPv6 multicast packets, for the latter IPv4 ones. This is because we are currently assuming that if either an IGMP or MLD querier is present that the other one is present, too.

Quoted 8 lines

This patch makes the behaviour and fix added in
"bridge: disable snooping if there is no querier" (b00589af3b04) to also work if there is either just an IGMP or an MLD querier on the
link: It refines the deactivation of the snooping to be protocol specific by using separate timers for the snooped IGMP and MLD queries as well as separate timers for our internal IGMP and MLD queriers.
Signed-off-by Linus Lüssing <[email protected]>

I think the logic of the fix is fine, but theres a lot of duplication here.

Why not make a:

struct bridge_mcast_timer {
	struct	timer_list	timer;
	u32			queries_sent;
	u32			startup_queries_sent;

};

Then in the port structure you have:

	struct bridge_mcast_timer	ip4;
	struct bridge_mcast_timer	ip6;

And pass a reference to the right one into generic helper functions.

That sounds a lot better right?

Please make this improvement and resubmit, thanks.

Linus Lüssing

author

2013-08-30 15:27 UTC #2 lore raw

PATCH v2 1/1 bridge: separate querier and query timer into IGMP/IPv4 and MLD/IPv6 ones

Signed-off-by Linus Lüssing <[email protected]>

---
 net/bridge/br_device.c    |    2 +-
 net/bridge/br_input.c     |    2 +-
 net/bridge/br_mdb.c       |   14 ++-
 net/bridge/br_multicast.c |  258 ++++++++++++++++++++++++++++++++-------------
 net/bridge/br_private.h   |   57 ++++++++--
 5 files changed, 240 insertions(+), 93 deletions(-)

net/bridge/br_device.c +1 -1

index 69363bd..89659d4 100644
@@ -71,7 +71,7 @@ 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)) &&
-		    br_multicast_querier_exists(br))
+		    br_multicast_querier_exists(br, eth_hdr(skb)))
 			br_multicast_deliver(mdst, skb);
 		else
 			br_flood_deliver(br, skb, false);

net/bridge/br_input.c +1 -1

index 8c561c0..a2fd37e 100644
@@ -102,7 +102,7 @@ int br_handle_frame_finish(struct sk_buff *skb)
 	} else if (is_multicast_ether_addr(dest)) {
 		mdst = br_mdb_get(br, skb, vid);
 		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
-		    br_multicast_querier_exists(br)) {
+		    br_multicast_querier_exists(br, eth_hdr(skb))) {
 			if ((mdst && mdst->mglist) ||
 			    br_multicast_is_router(br))
 				skb2 = skb;

net/bridge/br_mdb.c +9 -5

index 0daae3e..6319c43 100644
@@ -414,16 +414,20 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
 	if (!netif_running(br->dev) || br->multicast_disabled)
 		return -EINVAL;
 
-	if (timer_pending(&br->multicast_querier_timer))
-		return -EBUSY;
-
 	ip.proto = entry->addr.proto;
-	if (ip.proto == htons(ETH_P_IP))
+	if (ip.proto == htons(ETH_P_IP)) {
+		if (timer_pending(&br->ip4_querier.timer))
+			return -EBUSY;
+
 		ip.u.ip4 = entry->addr.u.ip4;
 #if IS_ENABLED(CONFIG_IPV6)
-	else
+	} else {
+		if (timer_pending(&br->ip6_querier.timer))
+			return -EBUSY;
+
 		ip.u.ip6 = entry->addr.u.ip6;
 #endif
+	}
 
 	spin_lock_bh(&br->multicast_lock);
 	mdb = mlock_dereference(br->mdb, br);

net/bridge/br_multicast.c +183 ~~-75~~

488 lines not on this page · raw message

net/bridge/br_private.h +46 ~~-11~~

102 lines not on this page · raw message

Signature · 1 line

1.7.10.4

David Miller

maintainer

2013-08-30 19:29 UTC #3 lore raw

From: Linus Lüssing <[email protected]>
Date: Fri, 30 Aug 2013 17:28:17 +0200

Quoted 6 lines

Currently we would still potentially suffer multicast packet loss if there is just either an IGMP or an MLD querier: For the former case, we would possibly drop IPv6 multicast packets, for the latter IPv4 ones. This is because we are currently assuming that if either an IGMP or MLD querier is present that the other one is present, too.

Quoted 8 lines

This patch makes the behaviour and fix added in
"bridge: disable snooping if there is no querier" (b00589af3b04) to also work if there is either just an IGMP or an MLD querier on the
link: It refines the deactivation of the snooping to be protocol specific by using separate timers for the snooped IGMP and MLD queries as well as separate timers for our internal IGMP and MLD queriers.
Signed-off-by Linus Lüssing <[email protected]>

Looks great, applied, thanks.