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

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()?