[PATCH net 0/2] bridge: mcast: Fix MDB exhaustion by rejected entries

[PATCH net 0/2] bridge: mcast: Fix MDB exhaustion by rejected entries

[Ido Schimmel]

When the bridge fails to create a port group for a new MDB entry, the
entry is left behind with no ports and is never deleted, but it is still
counted towards mcast_hash_max. Since the introduction of
mcast_max_groups, a host behind a port that reached its maximum can use
this to exhaust the MDB and disable multicast snooping.

Patch #1 fixes the problem by deleting the entry upon failure.

Patch #2 adds a selftest.

Ido Schimmel (2):
  bridge: mcast: Delete empty MDB entry when port group creation fails
  selftests: forwarding: bridge_mdb_max: Add a test for rejected MDB
    entries

 net/bridge/br_mdb.c                           |  16 +-
 net/bridge/br_multicast.c                     |   9 ++
 net/bridge/br_private.h                       |   1 +
 .../net/forwarding/bridge_mdb_max.sh          | 138 ++++++++++++++++++
 4 files changed, 161 insertions(+), 3 deletions(-)

-- 
2.56.0

[PATCH net 1/2] bridge: mcast: Delete empty MDB entry when port group creation fails

[Ido Schimmel]

When a port group cannot be created for a new group, the MDB entry that
was created for it is left with no ports and is not joined by the host.
Its timer is never armed, so the entry is never deleted, but it is
counted towards mcast_hash_max. When this maximum is reached, the bridge
disables multicast snooping.

Since commit b57e8d870d52 ("net: bridge: Maintain number of MDB entries
in net_bridge_mcast_port"), port group creation also fails when the port
reached its mcast_max_groups limit. A host behind such a port can
therefore disable snooping by sending reports for new groups, which is
what the limit is meant to prevent. The RTM_NEWMDB path has the same
problem.

Fix by deleting the entry after a failure if it is unused. Delete it
immediately rather than arming its timer, so that it stops counting
towards the maximum right away.

Fixes: b57e8d870d52 ("net: bridge: Maintain number of MDB entries in net_bridge_mcast_port")
Reported-by: Yunlong Ma <yunlong_ma@buaa.edu.cn>
Closes: https://lore.kernel.org/netdev/2f5b61e2.1e71a.1a11fcb3809.Coremail.yunlong_ma@buaa.edu.cn/
Assisted-by: LLM
Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 net/bridge/br_mdb.c       | 16 +++++++++++++---
 net/bridge/br_multicast.c |  9 +++++++++
 net/bridge/br_private.h   |  1 +
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index a01bd280c722..0adcd1524ab2 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -819,6 +819,7 @@ static int br_mdb_add_group_src_fwd(const struct br_mdb_config *cfg,
 	struct br_mdb_config sg_cfg;
 	struct br_ip sg_ip;
 	u8 flags = 0;
+	int err;
 
 	sg_ip = cfg->group;
 	sg_ip.src = src_ip->src;
@@ -842,7 +843,11 @@ static int br_mdb_add_group_src_fwd(const struct br_mdb_config *cfg,
 	sg_cfg.filter_mode = MCAST_INCLUDE;
 	sg_cfg.rt_protocol = cfg->rt_protocol;
 	sg_cfg.nlflags = cfg->nlflags;
-	return br_mdb_add_group_sg(&sg_cfg, sgmp, brmctx, flags, extack);
+	err = br_mdb_add_group_sg(&sg_cfg, sgmp, brmctx, flags, extack);
+	if (err)
+		br_multicast_del_group(sgmp);
+
+	return err;
 }
 
 static int br_mdb_add_group_src(const struct br_mdb_config *cfg,
@@ -1047,6 +1052,7 @@ static int br_mdb_add_group(const struct br_mdb_config *cfg,
 	struct net_bridge_mcast *brmctx;
 	struct br_ip group = cfg->group;
 	unsigned char flags = 0;
+	int err;
 
 	brmctx = __br_mdb_choose_context(br, entry, extack);
 	if (!brmctx)
@@ -1073,9 +1079,13 @@ static int br_mdb_add_group(const struct br_mdb_config *cfg,
 		flags |= MDB_PG_FLAGS_PERMANENT;
 
 	if (br_multicast_is_star_g(&group))
-		return br_mdb_add_group_star_g(cfg, mp, brmctx, flags, extack);
+		err = br_mdb_add_group_star_g(cfg, mp, brmctx, flags, extack);
 	else
-		return br_mdb_add_group_sg(cfg, mp, brmctx, flags, extack);
+		err = br_mdb_add_group_sg(cfg, mp, brmctx, flags, extack);
+	if (err)
+		br_multicast_del_group(mp);
+
+	return err;
 }
 
 static int __br_mdb_add(const struct br_mdb_config *cfg,
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 3fbad7b59769..ecc74b73a3e1 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -647,6 +647,14 @@ static void br_multicast_del_mdb_entry(struct net_bridge_mdb_entry *mp)
 	queue_work(system_long_wq, &br->mcast_gc_work);
 }
 
+void br_multicast_del_group(struct net_bridge_mdb_entry *mp)
+{
+	if (mp->ports || mp->host_joined)
+		return;
+
+	br_multicast_del_mdb_entry(mp);
+}
+
 static void br_multicast_group_expired(struct timer_list *t)
 {
 	struct net_bridge_mdb_entry *mp = timer_container_of(mp, t, timer);
@@ -1554,6 +1562,7 @@ __br_multicast_add_group(struct net_bridge_mcast *brmctx,
 	p = br_multicast_new_port_group(pmctx->port, group, *pp, 0, src,
 					filter_mode, RTPROT_KERNEL, NULL);
 	if (unlikely(!p)) {
+		br_multicast_del_group(mp);
 		p = ERR_PTR(-ENOMEM);
 		goto out;
 	}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index b01997ea9508..ff746f036f54 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -998,6 +998,7 @@ struct net_bridge_mdb_entry *
 br_mdb_ip_get(struct net_bridge *br, struct br_ip *dst);
 struct net_bridge_mdb_entry *
 br_multicast_new_group(struct net_bridge *br, struct br_ip *group);
+void br_multicast_del_group(struct net_bridge_mdb_entry *mp);
 struct net_bridge_port_group *
 br_multicast_new_port_group(struct net_bridge_port *port,
 			    const struct br_ip *group,
-- 
2.56.0

[PATCH net 2/2] selftests: forwarding: bridge_mdb_max: Add a test for rejected MDB entries

[Ido Schimmel]

Add a test that checks that MDB entries that are rejected because the
port reached its maximum number of MDB entries (mcast_max_groups) do not
count towards the maximum number of MDB entries in the bridge
(mcast_hash_max).

The test lowers mcast_hash_max so that only three more entries can be
added. It then configures the port's maximum so that new (*, G) entries
are rejected and tries to add four of them. Next, it raises the maximum
by one so that a new (*, G) entry is accepted, but its (S, G) entry is
rejected, and tries this with four groups. Finally, it checks that
multicast snooping was not disabled because the bridge ran out of MDB
entries.

Like the other tests, the test is run for entries added both by the
control path (IGMP / MLD reports) and by the configuration path
(RTM_NEWMDB), and for both the port and port-VLAN maximums.

Without "bridge: mcast: Delete empty MDB entry when port group creation
fails", the new tests fail (other tests omitted):

 # ./bridge_mdb_max.sh
 INFO: 802.1d tests
 TEST: cfg4: port maxgroups: rejected entries do not exhaust MDB     [FAIL]
         Multicast snooping disabled by rejected MDB entries
 TEST: ctl4: port maxgroups: rejected entries do not exhaust MDB     [FAIL]
         Multicast snooping disabled by rejected MDB entries
 TEST: cfg6: port maxgroups: rejected entries do not exhaust MDB     [FAIL]
         Multicast snooping disabled by rejected MDB entries
 TEST: ctl6: port maxgroups: rejected entries do not exhaust MDB     [FAIL]
         Multicast snooping disabled by rejected MDB entries
 INFO: 802.1q tests
 TEST: cfg4: port maxgroups: rejected entries do not exhaust MDB     [FAIL]
         Multicast snooping disabled by rejected MDB entries
 TEST: ctl4: port maxgroups: rejected entries do not exhaust MDB     [FAIL]
         Multicast snooping disabled by rejected MDB entries
 TEST: cfg6: port maxgroups: rejected entries do not exhaust MDB     [FAIL]
         Multicast snooping disabled by rejected MDB entries
 TEST: ctl6: port maxgroups: rejected entries do not exhaust MDB     [FAIL]
         Multicast snooping disabled by rejected MDB entries
 INFO: 802.1q mcast_vlan_snooping 1 tests
 TEST: cfg4: port_vlan maxgroups: rejected entries do not exhaust MDB   [FAIL]
         Multicast snooping disabled by rejected MDB entries
 TEST: ctl4: port_vlan maxgroups: rejected entries do not exhaust MDB   [FAIL]
         Multicast snooping disabled by rejected MDB entries
 TEST: cfg6: port_vlan maxgroups: rejected entries do not exhaust MDB   [FAIL]
         Multicast snooping disabled by rejected MDB entries
 TEST: ctl6: port_vlan maxgroups: rejected entries do not exhaust MDB   [FAIL]
         Multicast snooping disabled by rejected MDB entries

Assisted-by: LLM
Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../net/forwarding/bridge_mdb_max.sh          | 138 ++++++++++++++++++
 1 file changed, 138 insertions(+)

diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb_max.sh b/tools/testing/selftests/net/forwarding/bridge_mdb_max.sh
index 625162fd7e8b..4ebffe4588a2 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb_max.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb_max.sh
@@ -399,6 +399,24 @@ bridge_port_vlan_maxgroups_set()
 	bridge vlan set $locus mcast_max_groups $max
 }
 
+bridge_mcast_snooping_get()
+{
+	ip -j -d link show dev br0 |
+	    jq '.[].linkinfo.info_data.mcast_snooping'
+}
+
+bridge_mcast_hash_max_get()
+{
+	ip -j -d link show dev br0 |
+	    jq '.[].linkinfo.info_data.mcast_hash_max'
+}
+
+bridge_mdb_entries_count()
+{
+	bridge -j mdb show dev br0 |
+	    jq '[.[].mdb[] | [.grp, .src, .vid]] | unique | length'
+}
+
 test_ngroups_reporting()
 {
 	local CFG=$1; shift
@@ -938,6 +956,114 @@ test_8021qvs_maxgroups_too_many_entries_ctl6()
 	test_maxgroups_too_many_entries ctl6 port_vlan "dev $swp1 vid 10"
 }
 
+test_maxgroups_hash_max()
+{
+	local CFG=$1; shift
+	local context=$1; shift
+	local locus=$1; shift
+
+	local hash_max n i
+
+	RET=0
+
+	hash_max=$(bridge_mcast_hash_max_get)
+
+	# Make sure the port has entries, so that max == n is not 0 (no limit).
+	${CFG}_entries_add "$locus" temp 2 120
+	check_err $? "$locus: Couldn't add MDB entries"
+	n=$(bridge_${context}_ngroups_get "$locus")
+
+	# Leave room for three more MDB entries.
+	ip link set dev br0 type bridge \
+		mcast_hash_max $(($(bridge_mdb_entries_count) + 3))
+	check_err $? "Couldn't set mcast_hash_max"
+
+	# Port at its maximum: (*, G) entries are rejected.
+	bridge_${context}_maxgroups_set "$locus" "$n"
+	check_err $? "$locus: Couldn't set maximum"
+	for i in $(seq 121 124); do
+		${CFG}_entries_add "$locus" temp 2 "$i" &> /dev/null
+	done
+
+	# One more slot: (*, G) is accepted, (S, G) is rejected.
+	bridge_${context}_maxgroups_set "$locus" $((n + 1))
+	check_err $? "$locus: Couldn't set maximum"
+	for i in $(seq 125 128); do
+		${CFG}_entries_add "$locus" temp 2 "$i" &> /dev/null
+	done
+
+	[[ $(bridge_mcast_snooping_get) == 1 ]]
+	check_err $? "Multicast snooping disabled by rejected MDB entries"
+
+	bridge mdb flush dev br0
+	ip link set dev br0 type bridge mcast_snooping 1
+	ip link set dev br0 type bridge mcast_hash_max "$hash_max"
+	bridge_${context}_maxgroups_set "$locus" 0
+	check_err $? "$locus: Couldn't set maximum to 0"
+
+	log_test "$CFG: $context maxgroups: rejected entries do not exhaust MDB"
+}
+
+test_8021d_maxgroups_hash_max_cfg4()
+{
+	test_maxgroups_hash_max cfg4 port "dev $swp1"
+}
+
+test_8021d_maxgroups_hash_max_ctl4()
+{
+	test_maxgroups_hash_max ctl4 port "dev $swp1"
+}
+
+test_8021d_maxgroups_hash_max_cfg6()
+{
+	test_maxgroups_hash_max cfg6 port "dev $swp1"
+}
+
+test_8021d_maxgroups_hash_max_ctl6()
+{
+	test_maxgroups_hash_max ctl6 port "dev $swp1"
+}
+
+test_8021q_maxgroups_hash_max_cfg4()
+{
+	test_maxgroups_hash_max cfg4 port "dev $swp1 vid 10"
+}
+
+test_8021q_maxgroups_hash_max_ctl4()
+{
+	test_maxgroups_hash_max ctl4 port "dev $swp1 vid 10"
+}
+
+test_8021q_maxgroups_hash_max_cfg6()
+{
+	test_maxgroups_hash_max cfg6 port "dev $swp1 vid 10"
+}
+
+test_8021q_maxgroups_hash_max_ctl6()
+{
+	test_maxgroups_hash_max ctl6 port "dev $swp1 vid 10"
+}
+
+test_8021qvs_maxgroups_hash_max_cfg4()
+{
+	test_maxgroups_hash_max cfg4 port_vlan "dev $swp1 vid 10"
+}
+
+test_8021qvs_maxgroups_hash_max_ctl4()
+{
+	test_maxgroups_hash_max ctl4 port_vlan "dev $swp1 vid 10"
+}
+
+test_8021qvs_maxgroups_hash_max_cfg6()
+{
+	test_maxgroups_hash_max cfg6 port_vlan "dev $swp1 vid 10"
+}
+
+test_8021qvs_maxgroups_hash_max_ctl6()
+{
+	test_maxgroups_hash_max ctl6 port_vlan "dev $swp1 vid 10"
+}
+
 test_maxgroups_too_many_cross_vlan()
 {
 	local CFG=$1; shift
@@ -1177,6 +1303,7 @@ test_8021d_maxgroups_cfg4()
 	test_8021d_maxgroups_zero_cfg4
 	test_8021d_maxgroups_too_low_cfg4
 	test_8021d_maxgroups_too_many_entries_cfg4
+	test_8021d_maxgroups_hash_max_cfg4
 }
 
 test_8021d_maxgroups_ctl4()
@@ -1184,6 +1311,7 @@ test_8021d_maxgroups_ctl4()
 	test_8021d_maxgroups_zero_ctl4
 	test_8021d_maxgroups_too_low_ctl4
 	test_8021d_maxgroups_too_many_entries_ctl4
+	test_8021d_maxgroups_hash_max_ctl4
 }
 
 test_8021d_maxgroups_cfg6()
@@ -1191,6 +1319,7 @@ test_8021d_maxgroups_cfg6()
 	test_8021d_maxgroups_zero_cfg6
 	test_8021d_maxgroups_too_low_cfg6
 	test_8021d_maxgroups_too_many_entries_cfg6
+	test_8021d_maxgroups_hash_max_cfg6
 }
 
 test_8021d_maxgroups_ctl6()
@@ -1198,6 +1327,7 @@ test_8021d_maxgroups_ctl6()
 	test_8021d_maxgroups_zero_ctl6
 	test_8021d_maxgroups_too_low_ctl6
 	test_8021d_maxgroups_too_many_entries_ctl6
+	test_8021d_maxgroups_hash_max_ctl6
 }
 
 test_8021q_maxgroups_cfg4()
@@ -1205,6 +1335,7 @@ test_8021q_maxgroups_cfg4()
 	test_8021q_maxgroups_zero_cfg4
 	test_8021q_maxgroups_too_low_cfg4
 	test_8021q_maxgroups_too_many_entries_cfg4
+	test_8021q_maxgroups_hash_max_cfg4
 }
 
 test_8021q_maxgroups_ctl4()
@@ -1212,6 +1343,7 @@ test_8021q_maxgroups_ctl4()
 	test_8021q_maxgroups_zero_ctl4
 	test_8021q_maxgroups_too_low_ctl4
 	test_8021q_maxgroups_too_many_entries_ctl4
+	test_8021q_maxgroups_hash_max_ctl4
 }
 
 test_8021q_maxgroups_cfg6()
@@ -1219,6 +1351,7 @@ test_8021q_maxgroups_cfg6()
 	test_8021q_maxgroups_zero_cfg6
 	test_8021q_maxgroups_too_low_cfg6
 	test_8021q_maxgroups_too_many_entries_cfg6
+	test_8021q_maxgroups_hash_max_cfg6
 }
 
 test_8021q_maxgroups_ctl6()
@@ -1226,6 +1359,7 @@ test_8021q_maxgroups_ctl6()
 	test_8021q_maxgroups_zero_ctl6
 	test_8021q_maxgroups_too_low_ctl6
 	test_8021q_maxgroups_too_many_entries_ctl6
+	test_8021q_maxgroups_hash_max_ctl6
 }
 
 test_8021qvs_maxgroups_cfg4()
@@ -1234,6 +1368,7 @@ test_8021qvs_maxgroups_cfg4()
 	test_8021qvs_maxgroups_zero_cross_vlan_cfg4
 	test_8021qvs_maxgroups_too_low_cfg4
 	test_8021qvs_maxgroups_too_many_entries_cfg4
+	test_8021qvs_maxgroups_hash_max_cfg4
 	test_8021qvs_maxgroups_too_many_cross_vlan_cfg4
 }
 
@@ -1243,6 +1378,7 @@ test_8021qvs_maxgroups_ctl4()
 	test_8021qvs_maxgroups_zero_cross_vlan_ctl4
 	test_8021qvs_maxgroups_too_low_ctl4
 	test_8021qvs_maxgroups_too_many_entries_ctl4
+	test_8021qvs_maxgroups_hash_max_ctl4
 	test_8021qvs_maxgroups_too_many_cross_vlan_ctl4
 }
 
@@ -1252,6 +1388,7 @@ test_8021qvs_maxgroups_cfg6()
 	test_8021qvs_maxgroups_zero_cross_vlan_cfg6
 	test_8021qvs_maxgroups_too_low_cfg6
 	test_8021qvs_maxgroups_too_many_entries_cfg6
+	test_8021qvs_maxgroups_hash_max_cfg6
 	test_8021qvs_maxgroups_too_many_cross_vlan_cfg6
 }
 
@@ -1261,6 +1398,7 @@ test_8021qvs_maxgroups_ctl6()
 	test_8021qvs_maxgroups_zero_cross_vlan_ctl6
 	test_8021qvs_maxgroups_too_low_ctl6
 	test_8021qvs_maxgroups_too_many_entries_ctl6
+	test_8021qvs_maxgroups_hash_max_ctl6
 	test_8021qvs_maxgroups_too_many_cross_vlan_ctl6
 }
 
-- 
2.56.0