[PATCH] net: bridge: mcast: don't truncate the port group walk on teardown

[PATCH] net: bridge: mcast: don't truncate the port group walk on teardown

[Jun Yang]

__br_multicast_disable_port_ctx() and br_multicast_del_port() walk
port->mglist with hlist_for_each_entry_safe(), which only guarantees that
the *current* node may be removed by the loop body.

The body is br_multicast_find_del_pg() -> br_multicast_del_pg(), and that
deletes further port groups of the very same port. br_multicast_del_pg()
drops the group's sources, and br_multicast_fwd_src_remove()
(net/bridge/br_multicast.c:583) deletes the (S,G) port group installed on
that same port; br_multicast_star_g_handle_mode() -> __fwd_del_star_excl()
(net/bridge/br_multicast.c:330) deletes the automatically installed
MDB_PG_FLAGS_STAR_EXCL entries, again on that same port. All of those sit
on the same port->mglist.

When one of them happens to be the node the iterator already latched as
"next", hlist_del_init() clears its ->next, the walk sees NULL and stops.
Every port group after it is silently left on the port. port->mglist is
head-inserted, so this needs the cascade victim to be older than the (*,G)
entry owning the source - a user-added, non-permanent (S,G) MDB entry added
before the (*,G) join produces exactly that ordering.

Hitting it once truncates the disable walk in
__br_multicast_disable_port_ctx() and once more truncates the flush in
br_multicast_del_port(), so del_nbp() goes on to free the port with port
groups still on port->mglist - and still linked in the bridge's mdb, with a
dangling ->key.port. Any subsequent mdb dump reads the freed port:

  BUG: KASAN: slab-use-after-free in __mdb_fill_info+0x1191/0x1320
  Read of size 8 at addr ffff88803065d008 by task bridge/9527
   __mdb_fill_info+0x1191/0x1320
   br_mdb_dump+0x594/0xe40
   rtnl_mdb_dump+0x1cf/0x5d0
  Freed by task 0:
   kfree+0x265/0x740
   kobject_put+0x212/0x6a0
   rcu_core+0x5c6/0x1140
  Last potentially related work creation:
   __call_rcu_common.constprop.0+0xb7/0x9e0
   br_del_if+0xdd/0x260

Don't rely on the pre-latched next pointer. br_multicast_del_port() deletes
everything, so just take the current list head each round. The filtered
walk in __br_multicast_disable_port_ctx() keeps its iterator but restarts
whenever the latched node has left the list; port groups are only freed by
the multicast GC work, which takes br->multicast_lock, so the node is still
valid memory for that check.

Fixes: b08123684bd5 ("net: bridge: mcast: install S,G entries automatically based on reports")
Cc: stable@vger.kernel.org
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Assisted-by: tencentos-corvus-ai:kimi-k3
Signed-off-by: Jun Yang <junvyyang@tencent.com>
---
A KASAN reproducer for this issue is available if requested.

 net/bridge/br_multicast.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 00aa9b2879d6..624dfca4066b 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -2065,12 +2065,17 @@ void br_multicast_del_port(struct net_bridge_port *port)
 {
 	struct net_bridge *br = port->br;
 	struct net_bridge_port_group *pg;
-	struct hlist_node *n;
 
-	/* Take care of the remaining groups, only perm ones should be left */
+	/* Take care of the remaining groups, only perm ones should be left.
+	 * Deleting one can delete others on this same port->mglist, so
+	 * always restart from the head.
+	 */
 	spin_lock_bh(&br->multicast_lock);
-	hlist_for_each_entry_safe(pg, n, &port->mglist, mglist)
+	while (!hlist_empty(&port->mglist)) {
+		pg = hlist_entry(port->mglist.first,
+				 struct net_bridge_port_group, mglist);
 		br_multicast_find_del_pg(br, pg);
+	}
 	spin_unlock_bh(&br->multicast_lock);
 	flush_work(&br->mcast_gc_work);
 	br_multicast_port_ctx_deinit(&port->multicast_ctx);
@@ -2126,11 +2132,23 @@ static void __br_multicast_disable_port_ctx(struct net_bridge_mcast_port *pmctx)
 	struct hlist_node *n;
 	bool del = false;
 
-	hlist_for_each_entry_safe(pg, n, &pmctx->port->mglist, mglist)
-		if (!(pg->flags & MDB_PG_FLAGS_PERMANENT) &&
-		    (!br_multicast_port_ctx_is_vlan(pmctx) ||
-		     pg->key.addr.vid == pmctx->vlan->vid))
-			br_multicast_find_del_pg(pmctx->port->br, pg);
+	/* br_multicast_find_del_pg() can delete further entries of this same
+	 * port->mglist, so the node latched in @n may be unlinked by the loop
+	 * body. Port groups are only freed by the GC work under multicast_lock,
+	 * so @n is still valid here; if it left the list, restart.
+	 */
+restart:
+	hlist_for_each_entry_safe(pg, n, &pmctx->port->mglist, mglist) {
+		if ((pg->flags & MDB_PG_FLAGS_PERMANENT) ||
+		    (br_multicast_port_ctx_is_vlan(pmctx) &&
+		     pg->key.addr.vid != pmctx->vlan->vid))
+			continue;
+
+		br_multicast_find_del_pg(pmctx->port->br, pg);
+
+		if (n && hlist_unhashed(n))
+			goto restart;
+	}
 
 	del |= br_ip4_multicast_rport_del(pmctx);
 	timer_delete(&pmctx->ip4_mc_router_timer);
-- 
2.55.0

Re: [PATCH] net: bridge: mcast: don't truncate the port group walk on teardown

[Nikolay Aleksandrov]

On 12/08/2026 14:34, Jun Yang wrote:
> __br_multicast_disable_port_ctx() and br_multicast_del_port() walk
> port->mglist with hlist_for_each_entry_safe(), which only guarantees that
> the *current* node may be removed by the loop body.
> 
> The body is br_multicast_find_del_pg() -> br_multicast_del_pg(), and that
> deletes further port groups of the very same port. br_multicast_del_pg()
> drops the group's sources, and br_multicast_fwd_src_remove()
> (net/bridge/br_multicast.c:583) deletes the (S,G) port group installed on
> that same port; br_multicast_star_g_handle_mode() -> __fwd_del_star_excl()
> (net/bridge/br_multicast.c:330) deletes the automatically installed
> MDB_PG_FLAGS_STAR_EXCL entries, again on that same port. All of those sit
> on the same port->mglist.
> 
> When one of them happens to be the node the iterator already latched as
> "next", hlist_del_init() clears its ->next, the walk sees NULL and stops.
> Every port group after it is silently left on the port. port->mglist is
> head-inserted, so this needs the cascade victim to be older than the (*,G)
> entry owning the source - a user-added, non-permanent (S,G) MDB entry added
> before the (*,G) join produces exactly that ordering.
> 
> Hitting it once truncates the disable walk in
> __br_multicast_disable_port_ctx() and once more truncates the flush in
> br_multicast_del_port(), so del_nbp() goes on to free the port with port
> groups still on port->mglist - and still linked in the bridge's mdb, with a
> dangling ->key.port. Any subsequent mdb dump reads the freed port:
> 
>    BUG: KASAN: slab-use-after-free in __mdb_fill_info+0x1191/0x1320
>    Read of size 8 at addr ffff88803065d008 by task bridge/9527
>     __mdb_fill_info+0x1191/0x1320
>     br_mdb_dump+0x594/0xe40
>     rtnl_mdb_dump+0x1cf/0x5d0
>    Freed by task 0:
>     kfree+0x265/0x740
>     kobject_put+0x212/0x6a0
>     rcu_core+0x5c6/0x1140
>    Last potentially related work creation:
>     __call_rcu_common.constprop.0+0xb7/0x9e0
>     br_del_if+0xdd/0x260
> 
> Don't rely on the pre-latched next pointer. br_multicast_del_port() deletes
> everything, so just take the current list head each round. The filtered
> walk in __br_multicast_disable_port_ctx() keeps its iterator but restarts
> whenever the latched node has left the list; port groups are only freed by
> the multicast GC work, which takes br->multicast_lock, so the node is still
> valid memory for that check.
> 
> Fixes: b08123684bd5 ("net: bridge: mcast: install S,G entries automatically based on reports")
> Cc: stable@vger.kernel.org
> Reported-by: TencentOS Corvus AI <corvus@tencent.com>
> Assisted-by: tencentos-corvus-ai:kimi-k3
> Signed-off-by: Jun Yang <junvyyang@tencent.com>
> ---
> A KASAN reproducer for this issue is available if requested.
> 
>   net/bridge/br_multicast.c | 33 +++++++++++++++++++++++++--------
>   1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 00aa9b2879d6..624dfca4066b 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -2065,12 +2065,17 @@ void br_multicast_del_port(struct net_bridge_port *port)
>   {
>   	struct net_bridge *br = port->br;
>   	struct net_bridge_port_group *pg;
> -	struct hlist_node *n;
>   
> -	/* Take care of the remaining groups, only perm ones should be left */
> +	/* Take care of the remaining groups, only perm ones should be left.
> +	 * Deleting one can delete others on this same port->mglist, so
> +	 * always restart from the head.
> +	 */
>   	spin_lock_bh(&br->multicast_lock);
> -	hlist_for_each_entry_safe(pg, n, &port->mglist, mglist)
> +	while (!hlist_empty(&port->mglist)) {
> +		pg = hlist_entry(port->mglist.first,
> +				 struct net_bridge_port_group, mglist);
>   		br_multicast_find_del_pg(br, pg);
> +	}
>   	spin_unlock_bh(&br->multicast_lock);
>   	flush_work(&br->mcast_gc_work);
>   	br_multicast_port_ctx_deinit(&port->multicast_ctx);
> @@ -2126,11 +2132,23 @@ static void __br_multicast_disable_port_ctx(struct net_bridge_mcast_port *pmctx)
>   	struct hlist_node *n;
>   	bool del = false;
>   
> -	hlist_for_each_entry_safe(pg, n, &pmctx->port->mglist, mglist)
> -		if (!(pg->flags & MDB_PG_FLAGS_PERMANENT) &&
> -		    (!br_multicast_port_ctx_is_vlan(pmctx) ||
> -		     pg->key.addr.vid == pmctx->vlan->vid))
> -			br_multicast_find_del_pg(pmctx->port->br, pg);
> +	/* br_multicast_find_del_pg() can delete further entries of this same
> +	 * port->mglist, so the node latched in @n may be unlinked by the loop
> +	 * body. Port groups are only freed by the GC work under multicast_lock,
> +	 * so @n is still valid here; if it left the list, restart.
> +	 */
> +restart:
> +	hlist_for_each_entry_safe(pg, n, &pmctx->port->mglist, mglist) {
> +		if ((pg->flags & MDB_PG_FLAGS_PERMANENT) ||
> +		    (br_multicast_port_ctx_is_vlan(pmctx) &&
> +		     pg->key.addr.vid != pmctx->vlan->vid))
> +			continue;
> +
> +		br_multicast_find_del_pg(pmctx->port->br, pg);
> +
> +		if (n && hlist_unhashed(n))
> +			goto restart;
> +	}
>   
>   	del |= br_ip4_multicast_rport_del(pmctx);
>   	timer_delete(&pmctx->ip4_mc_router_timer);

Thanks for the report, but instead of all these restarts and checks,
can't we just do:
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 75e1e2a8fc83..62c4008c5bb8 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -808,7 +808,8 @@ void br_multicast_del_pg(struct net_bridge_mdb_entry *mp,
         struct hlist_node *tmp;
  
         rcu_assign_pointer(*pp, pg->next);
-       hlist_del_init(&pg->mglist);
+       /* use _rcu to preserve the next pointer because it might be in use */
+       hlist_del_init_rcu(&pg->mglist);
         br_multicast_eht_clean_sets(pg);
         hlist_for_each_entry_safe(ent, tmp, &pg->src_list, node)
                 br_multicast_del_group_src(ent, false);


I have old patches that remove the mcast open-coded list implementations, I must
revive them and clean all of this up finally. :)

Cheers,
  Nik

Re: [PATCH] net: bridge: mcast: don't truncate the port group walk on teardown

[Nikolay Aleksandrov]

On 12/08/2026 15:17, Nikolay Aleksandrov wrote:
> On 12/08/2026 14:34, Jun Yang wrote:
>> __br_multicast_disable_port_ctx() and br_multicast_del_port() walk
>> port->mglist with hlist_for_each_entry_safe(), which only guarantees that
>> the *current* node may be removed by the loop body.
>>
>> The body is br_multicast_find_del_pg() -> br_multicast_del_pg(), and that
>> deletes further port groups of the very same port. br_multicast_del_pg()
>> drops the group's sources, and br_multicast_fwd_src_remove()
>> (net/bridge/br_multicast.c:583) deletes the (S,G) port group installed on
>> that same port; br_multicast_star_g_handle_mode() -> __fwd_del_star_excl()
>> (net/bridge/br_multicast.c:330) deletes the automatically installed
>> MDB_PG_FLAGS_STAR_EXCL entries, again on that same port. All of those sit
>> on the same port->mglist.
>>
>> When one of them happens to be the node the iterator already latched as
>> "next", hlist_del_init() clears its ->next, the walk sees NULL and stops.
>> Every port group after it is silently left on the port. port->mglist is
>> head-inserted, so this needs the cascade victim to be older than the (*,G)
>> entry owning the source - a user-added, non-permanent (S,G) MDB entry added
>> before the (*,G) join produces exactly that ordering.
>>
>> Hitting it once truncates the disable walk in
>> __br_multicast_disable_port_ctx() and once more truncates the flush in
>> br_multicast_del_port(), so del_nbp() goes on to free the port with port
>> groups still on port->mglist - and still linked in the bridge's mdb, with a
>> dangling ->key.port. Any subsequent mdb dump reads the freed port:
>>
>>    BUG: KASAN: slab-use-after-free in __mdb_fill_info+0x1191/0x1320
>>    Read of size 8 at addr ffff88803065d008 by task bridge/9527
>>     __mdb_fill_info+0x1191/0x1320
>>     br_mdb_dump+0x594/0xe40
>>     rtnl_mdb_dump+0x1cf/0x5d0
>>    Freed by task 0:
>>     kfree+0x265/0x740
>>     kobject_put+0x212/0x6a0
>>     rcu_core+0x5c6/0x1140
>>    Last potentially related work creation:
>>     __call_rcu_common.constprop.0+0xb7/0x9e0
>>     br_del_if+0xdd/0x260
>>
>> Don't rely on the pre-latched next pointer. br_multicast_del_port() deletes
>> everything, so just take the current list head each round. The filtered
>> walk in __br_multicast_disable_port_ctx() keeps its iterator but restarts
>> whenever the latched node has left the list; port groups are only freed by
>> the multicast GC work, which takes br->multicast_lock, so the node is still
>> valid memory for that check.
>>
>> Fixes: b08123684bd5 ("net: bridge: mcast: install S,G entries automatically based on reports")
>> Cc: stable@vger.kernel.org
>> Reported-by: TencentOS Corvus AI <corvus@tencent.com>
>> Assisted-by: tencentos-corvus-ai:kimi-k3
>> Signed-off-by: Jun Yang <junvyyang@tencent.com>
>> ---
>> A KASAN reproducer for this issue is available if requested.
>>
>>   net/bridge/br_multicast.c | 33 +++++++++++++++++++++++++--------
>>   1 file changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>> index 00aa9b2879d6..624dfca4066b 100644
>> --- a/net/bridge/br_multicast.c
>> +++ b/net/bridge/br_multicast.c
>> @@ -2065,12 +2065,17 @@ void br_multicast_del_port(struct net_bridge_port *port)
>>   {
>>       struct net_bridge *br = port->br;
>>       struct net_bridge_port_group *pg;
>> -    struct hlist_node *n;
>> -    /* Take care of the remaining groups, only perm ones should be left */
>> +    /* Take care of the remaining groups, only perm ones should be left.
>> +     * Deleting one can delete others on this same port->mglist, so
>> +     * always restart from the head.
>> +     */
>>       spin_lock_bh(&br->multicast_lock);
>> -    hlist_for_each_entry_safe(pg, n, &port->mglist, mglist)
>> +    while (!hlist_empty(&port->mglist)) {
>> +        pg = hlist_entry(port->mglist.first,
>> +                 struct net_bridge_port_group, mglist);
>>           br_multicast_find_del_pg(br, pg);
>> +    }
>>       spin_unlock_bh(&br->multicast_lock);
>>       flush_work(&br->mcast_gc_work);
>>       br_multicast_port_ctx_deinit(&port->multicast_ctx);
>> @@ -2126,11 +2132,23 @@ static void __br_multicast_disable_port_ctx(struct net_bridge_mcast_port *pmctx)
>>       struct hlist_node *n;
>>       bool del = false;
>> -    hlist_for_each_entry_safe(pg, n, &pmctx->port->mglist, mglist)
>> -        if (!(pg->flags & MDB_PG_FLAGS_PERMANENT) &&
>> -            (!br_multicast_port_ctx_is_vlan(pmctx) ||
>> -             pg->key.addr.vid == pmctx->vlan->vid))
>> -            br_multicast_find_del_pg(pmctx->port->br, pg);
>> +    /* br_multicast_find_del_pg() can delete further entries of this same
>> +     * port->mglist, so the node latched in @n may be unlinked by the loop
>> +     * body. Port groups are only freed by the GC work under multicast_lock,
>> +     * so @n is still valid here; if it left the list, restart.
>> +     */
>> +restart:
>> +    hlist_for_each_entry_safe(pg, n, &pmctx->port->mglist, mglist) {
>> +        if ((pg->flags & MDB_PG_FLAGS_PERMANENT) ||
>> +            (br_multicast_port_ctx_is_vlan(pmctx) &&
>> +             pg->key.addr.vid != pmctx->vlan->vid))
>> +            continue;
>> +
>> +        br_multicast_find_del_pg(pmctx->port->br, pg);
>> +
>> +        if (n && hlist_unhashed(n))
>> +            goto restart;
>> +    }
>>       del |= br_ip4_multicast_rport_del(pmctx);
>>       timer_delete(&pmctx->ip4_mc_router_timer);
> 
> Thanks for the report, but instead of all these restarts and checks,
> can't we just do:
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 75e1e2a8fc83..62c4008c5bb8 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -808,7 +808,8 @@ void br_multicast_del_pg(struct net_bridge_mdb_entry *mp,
>          struct hlist_node *tmp;
> 
>          rcu_assign_pointer(*pp, pg->next);
> -       hlist_del_init(&pg->mglist);
> +       /* use _rcu to preserve the next pointer because it might be in use */

Just to be clear - I'd expand the comment to include why it is safe to do so and under
what conditions (multicast_lock held)

> +       hlist_del_init_rcu(&pg->mglist);
>          br_multicast_eht_clean_sets(pg);
>          hlist_for_each_entry_safe(ent, tmp, &pg->src_list, node)
>                  br_multicast_del_group_src(ent, false);
> 
> 
> I have old patches that remove the mcast open-coded list implementations, I must
> revive them and clean all of this up finally. :)
> 
> Cheers,
>   Nik
>