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

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

[Jun Yang]

From: Jun Yang <junvyyang@tencent.com>

__br_multicast_disable_port_ctx() and br_multicast_del_port() walk
port->mglist with hlist_for_each_entry_safe(). However,
br_multicast_find_del_pg() can also delete other entries from the same
list through br_multicast_fwd_src_remove() or __fwd_del_star_excl().

If such an entry is the iterator's saved next node, hlist_del_init()
clears its ->next and terminates the walk early. The reproducer triggers
this in both teardown walks, leaving port groups in the bridge mdb with
a dangling ->key.port after del_nbp() frees the port:

  BUG: KASAN: slab-use-after-free in __mdb_fill_info+0x1191/0x1320
   __mdb_fill_info+0x1191/0x1320
   br_mdb_dump+0x594/0xe40
   rtnl_mdb_dump+0x1cf/0x5d0

Use hlist_del_init_rcu() to unlink the group while preserving ->next.
br_multicast_del_pg() and the teardown walks run under
br->multicast_lock. The GC worker must acquire the same lock before
detaching the group for destruction, so the node remains alive while
the walk uses the preserved pointer.

Fixes: b08123684bd5 ("net: bridge: mcast: install S,G entries automatically based on reports")
Cc: stable@vger.kernel.org
Suggested-by: Nikolay Aleksandrov <razor@blackwall.org>
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Assisted-by: tencentos-corvus-ai:kimi-k3
Signed-off-by: Jun Yang <junvyyang@tencent.com>
---
v2: delete with hlist_del_init_rcu() to preserve ->next for an in-progress
    teardown walk, per Nikolay's suggestion, instead of restarting the walks.
    Tested on 7.2-rc7 with KASAN: the original use-after-free is no longer
    reported.

v1: https://lore.kernel.org/all/20260812113435.1854275-1-junvyyang@tencent.com/

 net/bridge/br_multicast.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 00aa9b2879d6..8022c7f12169 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -808,7 +808,11 @@ 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);
+	/* Keep ->next (held under multicast_lock, freed later by the GC work):
+	 * a port->mglist teardown walk may have latched this node as its next,
+	 * and deleting other groups of the same port must not truncate it.
+	 */
+	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);
-- 
2.43.7

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

[Nikolay Aleksandrov]

On 26/08/2026 04:41, Jun Yang wrote:
> From: Jun Yang <junvyyang@tencent.com>
> 
> __br_multicast_disable_port_ctx() and br_multicast_del_port() walk
> port->mglist with hlist_for_each_entry_safe(). However,
> br_multicast_find_del_pg() can also delete other entries from the same
> list through br_multicast_fwd_src_remove() or __fwd_del_star_excl().
> 
> If such an entry is the iterator's saved next node, hlist_del_init()
> clears its ->next and terminates the walk early. The reproducer triggers
> this in both teardown walks, leaving port groups in the bridge mdb with
> a dangling ->key.port after del_nbp() frees the port:
> 
>    BUG: KASAN: slab-use-after-free in __mdb_fill_info+0x1191/0x1320
>     __mdb_fill_info+0x1191/0x1320
>     br_mdb_dump+0x594/0xe40
>     rtnl_mdb_dump+0x1cf/0x5d0
> 
> Use hlist_del_init_rcu() to unlink the group while preserving ->next.
> br_multicast_del_pg() and the teardown walks run under
> br->multicast_lock. The GC worker must acquire the same lock before
> detaching the group for destruction, so the node remains alive while
> the walk uses the preserved pointer.
> 
> Fixes: b08123684bd5 ("net: bridge: mcast: install S,G entries automatically based on reports")
> Cc: stable@vger.kernel.org
> Suggested-by: Nikolay Aleksandrov <razor@blackwall.org>
> Reported-by: TencentOS Corvus AI <corvus@tencent.com>
> Assisted-by: tencentos-corvus-ai:kimi-k3
> Signed-off-by: Jun Yang <junvyyang@tencent.com>
> ---
> v2: delete with hlist_del_init_rcu() to preserve ->next for an in-progress
>      teardown walk, per Nikolay's suggestion, instead of restarting the walks.
>      Tested on 7.2-rc7 with KASAN: the original use-after-free is no longer
>      reported.
> 
> v1: https://lore.kernel.org/all/20260812113435.1854275-1-junvyyang@tencent.com/
> 
>   net/bridge/br_multicast.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 00aa9b2879d6..8022c7f12169 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -808,7 +808,11 @@ 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);
> +	/* Keep ->next (held under multicast_lock, freed later by the GC work):
> +	 * a port->mglist teardown walk may have latched this node as its next,
> +	 * and deleting other groups of the same port must not truncate it.
> +	 */
> +	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);

Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: bridge: mcast: don't truncate the port group walk on teardown

This changes br_multicast_del_pg() to unlink the port group from
port->mglist with hlist_del_init_rcu() instead of hlist_del_init(), so
that a nested deletion of another group of the same port does not clear
the ->next pointer that an in-progress teardown walk has already latched.
The intent is to stop br_multicast_del_port() and
__br_multicast_disable_port_ctx() from terminating early and leaving port
groups behind with a dangling ->key.port.

> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 75e1e2a8fc83a..452ac68c4a835 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -808,7 +808,11 @@ 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);
> +	/* Keep ->next (held under multicast_lock, freed later by the GC work):
> +	 * a port->mglist teardown walk may have latched this node as its next,
> +	 * and deleting other groups of the same port must not truncate it.
> +	 */
> +	hlist_del_init_rcu(&pg->mglist);

[Severity: High]

Preserving ->next keeps the walk going, but doesn't it now make the walk
visit a group that was already deleted as a side effect?

hlist_del_init_rcu() only clears ->pprev, so the latched next node is
still reachable by the iterator:

	include/linux/rculist.h:
	static inline void hlist_del_init_rcu(struct hlist_node *n)
	{
		if (!hlist_unhashed(n)) {
			__hlist_del(n);
			WRITE_ONCE(n->pprev, NULL);
		}
	}

Taking br_multicast_del_port() as the example, all under
br->multicast_lock:

br_multicast_del_port()
   hlist_for_each_entry_safe(pg, n, &port->mglist, mglist)  /* n = A->next = B */
      br_multicast_find_del_pg(br, A)
         br_multicast_del_pg(mp, A, pp)
            br_multicast_del_group_src() -> br_multicast_fwd_src_remove()
            or br_multicast_star_g_handle_mode() -> __fwd_del_star_excl()
            or br_multicast_sg_del_exclude_ports()
               -> deletes B (same port) via br_multicast_del_pg()
                  rcu_assign_pointer(*pp, B->next);   /* B off mp->ports */
                  hlist_del_init_rcu(&B->mglist);     /* B->next preserved */
   pg = n = B                                          /* loop continues into B */
      br_multicast_find_del_pg(br, B)

In br_multicast_find_del_pg() the mdb entry lookup still succeeds, since
mdb entries are only unhashed under br->multicast_lock, but the scan over
mp->ports can no longer find B because br_multicast_del_pg() already did
rcu_assign_pointer(*pp, pg->next) for it:

	for (pp = &mp->ports;
	     (p = mlock_dereference(*pp, br)) != NULL;
	     pp = &p->next) {
		if (p != pg)
			continue;

		br_multicast_del_pg(mp, pg, pp);
		return;
	}

	WARN_ON(1);

So control falls through to the trailing WARN_ON(1). br_multicast_del_port()
applies no filter at all before calling br_multicast_find_del_pg(), and the
filter in __br_multicast_disable_port_ctx():

	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);

doesn't exclude a kernel-installed, non-permanent S,G group either.

Does that mean the reproducer described in the commit message now produces
a WARN backtrace on the same path instead of the KASAN use-after-free, and
a panic when booted with panic_on_warn=1?

Would it make sense to also skip nodes that are already unlinked in both
teardown walks, e.g.:

	hlist_for_each_entry_safe(pg, n, &port->mglist, mglist) {
		if (hlist_unhashed(&pg->mglist))
			continue;
		br_multicast_find_del_pg(br, pg);
	}

or otherwise restart the walk, rather than relying on the preserved ->next
alone?

[Severity: High]

This isn't a bug introduced by this patch, but the same list has a second
deletion site that still uses the non-RCU helper and frees immediately,
br_multicast_del_port_group():

	void br_multicast_del_port_group(struct net_bridge_port_group *p)
	{
		struct net_bridge_port *port = p->key.port;
		__u16 vid = p->key.addr.vid;

		hlist_del_init(&p->mglist);
		if (!br_multicast_is_star_g(&p->key.addr))
			rhashtable_remove_fast(&port->br->sg_port_tbl, &p->rhnode,
					       br_sg_port_rht_params);
		kfree(p);
		br_multicast_port_ngroups_dec(port, vid);
	}

port->mglist is also walked locklessly by br_multicast_list_adjacent(),
which only holds rcu_read_lock():

	hlist_for_each_entry_rcu(group, &port->mglist, mglist) {
		...
		entry->addr = group->key.addr;

That reader is exported and used from net/batman-adv/multicast.c. The group
is already published on port->mglist by br_multicast_new_port_group()
(hlist_add_head(&p->mglist, &port->mglist)) before the only caller of
br_multicast_del_port_group() runs, the err_del_port_group label in
br_mdb_add_group_star_g() in net/bridge/br_mdb.c, reached when
br_mdb_add_group_srcs() fails. br_mdb_config_src_list_init() does not
reject a duplicated source, so a MDBE_ATTR_SRC_LIST carrying the same
source twice without NLM_F_REPLACE makes br_mdb_add_group_src() return
-EEXIST; an allocation failure returning -ENOSPC is a second trigger.

spin_lock_bh(&br->multicast_lock) held by the deleting side does not
exclude an RCU reader on another CPU. Can br_multicast_list_adjacent()
then read group->key.addr and follow group->mglist.next after the plain
kfree(p), and does the hlist_del_init() there also truncate that RCU walk
by NULLing ->next?

Since this patch establishes that port->mglist deletions must preserve
->next and defer the free, would it be worth converting this site to
hlist_del_init_rcu() plus a deferred free, in the way
br_multicast_destroy_port_group() uses kfree_rcu()?

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

[Nikolay Aleksandrov]

On 27/08/2026 22:31, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: bridge: mcast: don't truncate the port group walk on teardown
> 
> This changes br_multicast_del_pg() to unlink the port group from
> port->mglist with hlist_del_init_rcu() instead of hlist_del_init(), so
> that a nested deletion of another group of the same port does not clear
> the ->next pointer that an in-progress teardown walk has already latched.
> The intent is to stop br_multicast_del_port() and
> __br_multicast_disable_port_ctx() from terminating early and leaving port
> groups behind with a dangling ->key.port.
> 
>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>> index 75e1e2a8fc83a..452ac68c4a835 100644
>> --- a/net/bridge/br_multicast.c
>> +++ b/net/bridge/br_multicast.c
>> @@ -808,7 +808,11 @@ 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);
>> +	/* Keep ->next (held under multicast_lock, freed later by the GC work):
>> +	 * a port->mglist teardown walk may have latched this node as its next,
>> +	 * and deleting other groups of the same port must not truncate it.
>> +	 */
>> +	hlist_del_init_rcu(&pg->mglist);
> 
> [Severity: High]
> 
> Preserving ->next keeps the walk going, but doesn't it now make the walk
> visit a group that was already deleted as a side effect?
> 
> hlist_del_init_rcu() only clears ->pprev, so the latched next node is
> still reachable by the iterator:
> 
> 	include/linux/rculist.h:
> 	static inline void hlist_del_init_rcu(struct hlist_node *n)
> 	{
> 		if (!hlist_unhashed(n)) {
> 			__hlist_del(n);
> 			WRITE_ONCE(n->pprev, NULL);
> 		}
> 	}
> 
> Taking br_multicast_del_port() as the example, all under
> br->multicast_lock:
> 
> br_multicast_del_port()
>     hlist_for_each_entry_safe(pg, n, &port->mglist, mglist)  /* n = A->next = B */
>        br_multicast_find_del_pg(br, A)
>           br_multicast_del_pg(mp, A, pp)
>              br_multicast_del_group_src() -> br_multicast_fwd_src_remove()
>              or br_multicast_star_g_handle_mode() -> __fwd_del_star_excl()
>              or br_multicast_sg_del_exclude_ports()
>                 -> deletes B (same port) via br_multicast_del_pg()
>                    rcu_assign_pointer(*pp, B->next);   /* B off mp->ports */
>                    hlist_del_init_rcu(&B->mglist);     /* B->next preserved */
>     pg = n = B                                          /* loop continues into B */
>        br_multicast_find_del_pg(br, B)
> 
> In br_multicast_find_del_pg() the mdb entry lookup still succeeds, since
> mdb entries are only unhashed under br->multicast_lock, but the scan over
> mp->ports can no longer find B because br_multicast_del_pg() already did
> rcu_assign_pointer(*pp, pg->next) for it:
> 
> 	for (pp = &mp->ports;
> 	     (p = mlock_dereference(*pp, br)) != NULL;
> 	     pp = &p->next) {
> 		if (p != pg)
> 			continue;
> 
> 		br_multicast_del_pg(mp, pg, pp);
> 		return;
> 	}
> 
> 	WARN_ON(1);
> 
> So control falls through to the trailing WARN_ON(1). br_multicast_del_port()
> applies no filter at all before calling br_multicast_find_del_pg(), and the
> filter in __br_multicast_disable_port_ctx():
> 
> 	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);
> 
> doesn't exclude a kernel-installed, non-permanent S,G group either.
> 
> Does that mean the reproducer described in the commit message now produces
> a WARN backtrace on the same path instead of the KASAN use-after-free, and
> a panic when booted with panic_on_warn=1?
> 
> Would it make sense to also skip nodes that are already unlinked in both
> teardown walks, e.g.:
> 
> 	hlist_for_each_entry_safe(pg, n, &port->mglist, mglist) {
> 		if (hlist_unhashed(&pg->mglist))
> 			continue;
> 		br_multicast_find_del_pg(br, pg);
> 	}
> 
> or otherwise restart the walk, rather than relying on the preserved ->next
> alone?
> 

Oh, right, I missed that WARN_ON() and it will fire, yes. But instead of
sprinkling these hlist_unhashed checks around, you can add 1 to the beginning
of br_multicast_find_del_pg() with a comment why it exists

> [Severity: High]
> 
> This isn't a bug introduced by this patch, but the same list has a second
> deletion site that still uses the non-RCU helper and frees immediately,
> br_multicast_del_port_group():
> 
> 	void br_multicast_del_port_group(struct net_bridge_port_group *p)
> 	{
> 		struct net_bridge_port *port = p->key.port;
> 		__u16 vid = p->key.addr.vid;
> 
> 		hlist_del_init(&p->mglist);
> 		if (!br_multicast_is_star_g(&p->key.addr))
> 			rhashtable_remove_fast(&port->br->sg_port_tbl, &p->rhnode,
> 					       br_sg_port_rht_params);
> 		kfree(p);
> 		br_multicast_port_ngroups_dec(port, vid);
> 	}
> 
> port->mglist is also walked locklessly by br_multicast_list_adjacent(),
> which only holds rcu_read_lock():
> 
> 	hlist_for_each_entry_rcu(group, &port->mglist, mglist) {
> 		...
> 		entry->addr = group->key.addr;
> 
> That reader is exported and used from net/batman-adv/multicast.c. The group
> is already published on port->mglist by br_multicast_new_port_group()
> (hlist_add_head(&p->mglist, &port->mglist)) before the only caller of
> br_multicast_del_port_group() runs, the err_del_port_group label in
> br_mdb_add_group_star_g() in net/bridge/br_mdb.c, reached when
> br_mdb_add_group_srcs() fails. br_mdb_config_src_list_init() does not
> reject a duplicated source, so a MDBE_ATTR_SRC_LIST carrying the same
> source twice without NLM_F_REPLACE makes br_mdb_add_group_src() return
> -EEXIST; an allocation failure returning -ENOSPC is a second trigger.
> 
> spin_lock_bh(&br->multicast_lock) held by the deleting side does not
> exclude an RCU reader on another CPU. Can br_multicast_list_adjacent()
> then read group->key.addr and follow group->mglist.next after the plain
> kfree(p), and does the hlist_del_init() there also truncate that RCU walk
> by NULLing ->next?
> 
> Since this patch establishes that port->mglist deletions must preserve
> ->next and defer the free, would it be worth converting this site to
> hlist_del_init_rcu() plus a deferred free, in the way
> br_multicast_destroy_port_group() uses kfree_rcu()?

Ouch, that is actually a separate and valid bug, the cleanup shouldn't be
using hlist_del_init + immediate kfree(). I will take care of this one.

Cheers,
  Nik