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

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