Linux Ethernet Bridging - Oct 2022 - [Bridge] [PATCH net-next 0/4] bridge: A few multicast cleanups

Clean up a few issues spotted while working on the bridge multicast code
and running its selftests.

Ido Schimmel (4):
  selftests: bridge_vlan_mcast: Delete qdiscs during cleanup
  selftests: bridge_igmp: Remove unnecessary address deletion
  bridge: mcast: Use spin_lock() instead of spin_lock_bh()
  bridge: mcast: Simplify MDB entry creation

 net/bridge/br_mdb.c                                   | 11 +++--------
 net/bridge/br_multicast.c                             |  8 ++++----
 tools/testing/selftests/net/forwarding/bridge_igmp.sh |  3 ---
 .../selftests/net/forwarding/bridge_vlan_mcast.sh     |  3 +++
 4 files changed, 10 insertions(+), 15 deletions(-)

-- 
2.37.3

The qdiscs are added during setup, but not deleted during cleanup,
resulting in the following error messages:

 # ./bridge_vlan_mcast.sh
 [...]
 # ./bridge_vlan_mcast.sh
 Error: Exclusivity flag on, cannot modify.
 Error: Exclusivity flag on, cannot modify.

Solve by deleting the qdiscs during cleanup.

Signed-off-by: Ido Schimmel <idosch at nvidia.com>
---
 tools/testing/selftests/net/forwarding/bridge_vlan_mcast.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/testing/selftests/net/forwarding/bridge_vlan_mcast.sh
b/tools/testing/selftests/net/forwarding/bridge_vlan_mcast.sh
index 8748d1b1d95b..72dfbeaf56b9 100755
--- a/tools/testing/selftests/net/forwarding/bridge_vlan_mcast.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_vlan_mcast.sh
@@ -59,6 +59,9 @@ switch_create()
 
 switch_destroy()
 {
+	tc qdisc del dev $swp2 clsact
+	tc qdisc del dev $swp1 clsact
+
 	ip link set dev $swp2 down
 	ip link set dev $swp1 down
 
-- 
2.37.3

The test group address is added and removed in v2reportleave_test().
There is no need to delete it again during cleanup as it results in the
following error message:

 # bash -x ./bridge_igmp.sh
 [...]
 + cleanup
 + pre_cleanup
 [...]
 + ip address del dev swp4 239.10.10.10/32
 RTNETLINK answers: Cannot assign requested address
 + h2_destroy

Solve by removing the unnecessary address deletion.

Signed-off-by: Ido Schimmel <idosch at nvidia.com>
---
 tools/testing/selftests/net/forwarding/bridge_igmp.sh | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/bridge_igmp.sh
b/tools/testing/selftests/net/forwarding/bridge_igmp.sh
index 1162836f8f32..2aa66d2a1702 100755
--- a/tools/testing/selftests/net/forwarding/bridge_igmp.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_igmp.sh
@@ -96,9 +96,6 @@ cleanup()
 
 	switch_destroy
 
-	# Always cleanup the mcast group
-	ip address del dev $h2 $TEST_GROUP/32 2>&1 1>/dev/null
-
 	h2_destroy
 	h1_destroy
 
-- 
2.37.3

IGMPv3 / MLDv2 Membership Reports are only processed from the data path
with softIRQ disabled, so there is no need to call spin_lock_bh(). Use
spin_lock() instead.

This is consistent with how other IGMP / MLD packets are processed.

Signed-off-by: Ido Schimmel <idosch at nvidia.com>
---
 net/bridge/br_multicast.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index db4f2641d1cd..09140bc8c15e 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -2669,7 +2669,7 @@ static int br_ip4_multicast_igmp3_report(struct
net_bridge_mcast *brmctx,
 		if (!pmctx || igmpv2)
 			continue;
 
-		spin_lock_bh(&brmctx->br->multicast_lock);
+		spin_lock(&brmctx->br->multicast_lock);
 		if (!br_multicast_ctx_should_use(brmctx, pmctx))
 			goto unlock_continue;
 
@@ -2717,7 +2717,7 @@ static int br_ip4_multicast_igmp3_report(struct
net_bridge_mcast *brmctx,
 		if (changed)
 			br_mdb_notify(brmctx->br->dev, mdst, pg, RTM_NEWMDB);
 unlock_continue:
-		spin_unlock_bh(&brmctx->br->multicast_lock);
+		spin_unlock(&brmctx->br->multicast_lock);
 	}
 
 	return err;
@@ -2807,7 +2807,7 @@ static int br_ip6_multicast_mld2_report(struct
net_bridge_mcast *brmctx,
 		if (!pmctx || mldv1)
 			continue;
 
-		spin_lock_bh(&brmctx->br->multicast_lock);
+		spin_lock(&brmctx->br->multicast_lock);
 		if (!br_multicast_ctx_should_use(brmctx, pmctx))
 			goto unlock_continue;
 
@@ -2859,7 +2859,7 @@ static int br_ip6_multicast_mld2_report(struct
net_bridge_mcast *brmctx,
 		if (changed)
 			br_mdb_notify(brmctx->br->dev, mdst, pg, RTM_NEWMDB);
 unlock_continue:
-		spin_unlock_bh(&brmctx->br->multicast_lock);
+		spin_unlock(&brmctx->br->multicast_lock);
 	}
 
 	return err;
-- 
2.37.3

Before creating a new MDB entry, br_multicast_new_group() will call
br_mdb_ip_get() to see if one exists and return it if so.

Therefore, simply call br_multicast_new_group() and omit the call to
br_mdb_ip_get().

Signed-off-by: Ido Schimmel <idosch at nvidia.com>
---
 net/bridge/br_mdb.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 589ff497d50c..321be94c445a 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -866,7 +866,6 @@ static int br_mdb_add_group(struct net_bridge *br, struct
net_bridge_port *port,
 	unsigned long now = jiffies;
 	unsigned char flags = 0;
 	u8 filter_mode;
-	int err;
 
 	__mdb_entry_to_br_ip(entry, &group, mdb_attrs);
 
@@ -892,13 +891,9 @@ static int br_mdb_add_group(struct net_bridge *br, struct
net_bridge_port *port,
 		return -EINVAL;
 	}
 
-	mp = br_mdb_ip_get(br, &group);
-	if (!mp) {
-		mp = br_multicast_new_group(br, &group);
-		err = PTR_ERR_OR_ZERO(mp);
-		if (err)
-			return err;
-	}
+	mp = br_multicast_new_group(br, &group);
+	if (IS_ERR(mp))
+		return PTR_ERR(mp);
 
 	/* host join */
 	if (!port) {
-- 
2.37.3

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem at davemloft.net>:

On Tue, 18 Oct 2022 09:39:57 +0300 you wrote:
> Clean up a few issues spotted while working on the bridge multicast code
> and running its selftests.
> 
> Ido Schimmel (4):
>   selftests: bridge_vlan_mcast: Delete qdiscs during cleanup
>   selftests: bridge_igmp: Remove unnecessary address deletion
>   bridge: mcast: Use spin_lock() instead of spin_lock_bh()
>   bridge: mcast: Simplify MDB entry creation
> 
> [...]
Here is the summary with links:
  - [net-next,1/4] selftests: bridge_vlan_mcast: Delete qdiscs during cleanup
    https://git.kernel.org/netdev/net-next/c/6fb1faa1b92b
  - [net-next,2/4] selftests: bridge_igmp: Remove unnecessary address deletion
    https://git.kernel.org/netdev/net-next/c/b526b2ea1454
  - [net-next,3/4] bridge: mcast: Use spin_lock() instead of spin_lock_bh()
    https://git.kernel.org/netdev/net-next/c/262985fad1bd
  - [net-next,4/4] bridge: mcast: Simplify MDB entry creation
    https://git.kernel.org/netdev/net-next/c/d1942cd47dbd

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html